| Message ID | 1441916335-22413-2-git-send-email-blp@nicira.com |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Sep 10, 2015 at 01:18:55PM -0700, Ben Pfaff wrote: > Otherwise duplicate bucket IDs cause linked list loops and other nastiness > because the ofputil_bucket_find() in the OFPG15_BUCKET_LAST case later in > copy_buckets_for_insert_bucket() will find the new bucket instead of the > old one and the list_splice() call becomes nonsensical. > > Reported-by: Ray Li <rayli1107@gmail.com> > Reported-at: http://openvswitch.org/pipermail/discuss/2015-September/018731.html > Signed-off-by: Ben Pfaff <blp@nicira.com> Reviewed-by: Simon Horman <simon.horman@netronome.com> > --- > AUTHORS | 1 + > ofproto/ofproto.c | 2 +- > tests/ofproto.at | 8 ++++++++ > 3 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/AUTHORS b/AUTHORS > index a7f40bb..75ca32a 100644 > --- a/AUTHORS > +++ b/AUTHORS > @@ -339,6 +339,7 @@ Pratap Reddy preddy@nicira.com > Ralf Heiringhoff ralf@frosty-geek.net > Ram Jothikumar rjothikumar@nicira.com > Ramana Reddy gtvrreddy@gmail.com > +Ray Li rayli1107@gmail.com > RishiRaj Maulick rishi.raj2509@gmail.com > Rob Sherwood rob.sherwood@bigswitch.com > Robert Strickler anomalyst@gmail.com > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index c7dd8a2..c884948 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -6368,7 +6368,7 @@ copy_buckets_for_insert_bucket(const struct ofgroup *ofgroup, > > ofputil_bucket_clone_list(&new_ofgroup->buckets, &ofgroup->buckets, NULL); > > - if (ofputil_bucket_check_duplicate_id(&ofgroup->buckets)) { > + if (ofputil_bucket_check_duplicate_id(&new_ofgroup->buckets)) { > VLOG_INFO_RL(&rl, "Duplicate bucket id"); > return OFPERR_OFPGMFC_BUCKET_EXISTS; > } > diff --git a/tests/ofproto.at b/tests/ofproto.at > index 7e80293..0f3a55e 100644 > --- a/tests/ofproto.at > +++ b/tests/ofproto.at > @@ -438,6 +438,14 @@ AT_CHECK([STRIP_XIDS stdout], [0], [dnl > OFPST_GROUP_DESC reply (OF1.5): > group_id=1234,type=all,bucket=bucket_id:0,actions=output:0,bucket=bucket_id:1,actions=output:1,bucket=bucket_id:10,actions=output:10,bucket=bucket_id:11,actions=output:11,bucket=bucket_id:14,actions=output:14,bucket=bucket_id:15,actions=output:15 > ]) > + > +# Verify that duplicate bucket IDs are rejected. > +AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=last,bucket=bucket_id:14,actions=output:14,bucket=bucket_id:15,actions=output:15], [1], [], [stderr]) > +AT_CHECK([STRIP_XIDS stderr | sed '/truncated/,$d'], [0], [dnl > +OFPT_ERROR (OF1.5): OFPGMFC_BUCKET_EXISTS > +OFPT_GROUP_MOD (OF1.5): > +]) > + > AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=15]) > AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout]) > AT_CHECK([STRIP_XIDS stdout], [0], [dnl > -- > 2.1.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On Wed, Nov 25, 2015 at 03:47:18PM +0900, Simon Horman wrote: > On Thu, Sep 10, 2015 at 01:18:55PM -0700, Ben Pfaff wrote: > > Otherwise duplicate bucket IDs cause linked list loops and other nastiness > > because the ofputil_bucket_find() in the OFPG15_BUCKET_LAST case later in > > copy_buckets_for_insert_bucket() will find the new bucket instead of the > > old one and the list_splice() call becomes nonsensical. > > > > Reported-by: Ray Li <rayli1107@gmail.com> > > Reported-at: http://openvswitch.org/pipermail/discuss/2015-September/018731.html > > Signed-off-by: Ben Pfaff <blp@nicira.com> > > Reviewed-by: Simon Horman <simon.horman@netronome.com> Thanks for the review! Applied to master and branch-2.4.
diff --git a/AUTHORS b/AUTHORS index a7f40bb..75ca32a 100644 --- a/AUTHORS +++ b/AUTHORS @@ -339,6 +339,7 @@ Pratap Reddy preddy@nicira.com Ralf Heiringhoff ralf@frosty-geek.net Ram Jothikumar rjothikumar@nicira.com Ramana Reddy gtvrreddy@gmail.com +Ray Li rayli1107@gmail.com RishiRaj Maulick rishi.raj2509@gmail.com Rob Sherwood rob.sherwood@bigswitch.com Robert Strickler anomalyst@gmail.com diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index c7dd8a2..c884948 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -6368,7 +6368,7 @@ copy_buckets_for_insert_bucket(const struct ofgroup *ofgroup, ofputil_bucket_clone_list(&new_ofgroup->buckets, &ofgroup->buckets, NULL); - if (ofputil_bucket_check_duplicate_id(&ofgroup->buckets)) { + if (ofputil_bucket_check_duplicate_id(&new_ofgroup->buckets)) { VLOG_INFO_RL(&rl, "Duplicate bucket id"); return OFPERR_OFPGMFC_BUCKET_EXISTS; } diff --git a/tests/ofproto.at b/tests/ofproto.at index 7e80293..0f3a55e 100644 --- a/tests/ofproto.at +++ b/tests/ofproto.at @@ -438,6 +438,14 @@ AT_CHECK([STRIP_XIDS stdout], [0], [dnl OFPST_GROUP_DESC reply (OF1.5): group_id=1234,type=all,bucket=bucket_id:0,actions=output:0,bucket=bucket_id:1,actions=output:1,bucket=bucket_id:10,actions=output:10,bucket=bucket_id:11,actions=output:11,bucket=bucket_id:14,actions=output:14,bucket=bucket_id:15,actions=output:15 ]) + +# Verify that duplicate bucket IDs are rejected. +AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=last,bucket=bucket_id:14,actions=output:14,bucket=bucket_id:15,actions=output:15], [1], [], [stderr]) +AT_CHECK([STRIP_XIDS stderr | sed '/truncated/,$d'], [0], [dnl +OFPT_ERROR (OF1.5): OFPGMFC_BUCKET_EXISTS +OFPT_GROUP_MOD (OF1.5): +]) + AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=15]) AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout]) AT_CHECK([STRIP_XIDS stdout], [0], [dnl
Otherwise duplicate bucket IDs cause linked list loops and other nastiness because the ofputil_bucket_find() in the OFPG15_BUCKET_LAST case later in copy_buckets_for_insert_bucket() will find the new bucket instead of the old one and the list_splice() call becomes nonsensical. Reported-by: Ray Li <rayli1107@gmail.com> Reported-at: http://openvswitch.org/pipermail/discuss/2015-September/018731.html Signed-off-by: Ben Pfaff <blp@nicira.com> --- AUTHORS | 1 + ofproto/ofproto.c | 2 +- tests/ofproto.at | 8 ++++++++ 3 files changed, 10 insertions(+), 1 deletion(-)