Thanks. I applied this and patch 5 (the only other remaining patch).
On Fri, Feb 21, 2014 at 03:16:28PM -0800, Joe Stringer wrote: > Looks good to me. > > Acked-by: Joe Stringer <joestrin...@nicira.com> > > > On 21 February 2014 10:46, Ben Pfaff <b...@nicira.com> wrote: > > > On Wed, Feb 19, 2014 at 02:53:11PM -0800, Joe Stringer wrote: > > > This looks good to me, although aren't we meant to report something at > > the > > > OpenFlow layer? > > > > > > Is this equivalent to the OpenFlow "chaining" description? So if we don't > > > support chaining groups together, we should return an error message with > > > reason OFPGMFC_CHAINING_UNSUPPORTED, rather than accepting the rules then > > > misbehaving? > > > > You're right. Here's a version that does that, and adds a test. > > > > --8<--------------------------cut here-------------------------->8-- > > > > From: Ben Pfaff <b...@nicira.com> > > Date: Fri, 21 Feb 2014 10:45:00 -0800 > > Subject: [PATCH] ofproto-dpif-xlate: Avoid recursively taking read side of > > ofgroup rwlock. > > > > With glibc, rwlocks by default allow recursive read-locking even if a > > thread is blocked waiting for the write-lock. POSIX allows such attempts > > to deadlock, and it appears that the libc used in NetBSD, at least, does > > deadlock. ofproto-dpif-xlate could take the ofgroup rwlock recursively if > > an ofgroup's actions caused the ofgroup to be executed again. This commit > > avoids that issue by preventing recursive translation of groups (the same > > group or another group). This is not the most user friendly solution, > > but OpenFlow allows this restriction, and we can always remove the > > restriction later (probably requiring more complicated code) if it > > proves to be a real problem to real users. > > > > Found by inspection. > > > > Signed-off-by: Ben Pfaff <b...@nicira.com> > > --- > > ofproto/ofproto-dpif-xlate.c | 32 +++++++++++++++++++++++++++++++- > > ofproto/ofproto-dpif.c | 14 ++++++++++++++ > > tests/ofproto-dpif.at | 11 +++++++++++ > > 3 files changed, 56 insertions(+), 1 deletion(-) > > > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > > index ccf0b75..89d92af 100644 > > --- a/ofproto/ofproto-dpif-xlate.c > > +++ b/ofproto/ofproto-dpif-xlate.c > > @@ -178,6 +178,7 @@ struct xlate_ctx { > > /* Resubmit statistics, via xlate_table_action(). */ > > int recurse; /* Current resubmit nesting depth. */ > > int resubmits; /* Total number of resubmits. */ > > + bool in_group; /* Currently translating ofgroup, if > > true. */ > > > > uint32_t orig_skb_priority; /* Priority when packet arrived. */ > > uint8_t table_id; /* OpenFlow table ID where flow was > > found. */ > > @@ -1980,6 +1981,8 @@ xlate_select_group(struct xlate_ctx *ctx, struct > > group_dpif *group) > > static void > > xlate_group_action__(struct xlate_ctx *ctx, struct group_dpif *group) > > { > > + ctx->in_group = true; > > + > > switch (group_dpif_get_type(group)) { > > case OFPGT11_ALL: > > case OFPGT11_INDIRECT: > > @@ -1995,12 +1998,38 @@ xlate_group_action__(struct xlate_ctx *ctx, struct > > group_dpif *group) > > OVS_NOT_REACHED(); > > } > > group_dpif_release(group); > > + > > + ctx->in_group = false; > > +} > > + > > +static bool > > +xlate_group_resource_check(struct xlate_ctx *ctx) > > +{ > > + if (!xlate_resubmit_resource_check(ctx)) { > > + return false; > > + } else if (ctx->in_group) { > > + /* Prevent nested translation of OpenFlow groups. > > + * > > + * OpenFlow allows this restriction. We enforce this restriction > > only > > + * because, with the current architecture, we would otherwise > > have to > > + * take a possibly recursive read lock on the ofgroup rwlock, > > which is > > + * unsafe given that POSIX allows taking a read lock to block if > > there > > + * is a thread blocked on taking the write lock. Other solutions > > + * without this restriction are also possible, but seem > > unwarranted > > + * given the current limited use of groups. */ > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > + > > + VLOG_ERR_RL(&rl, "cannot recursively translate OpenFlow group"); > > + return false; > > + } else { > > + return true; > > + } > > } > > > > static bool > > xlate_group_action(struct xlate_ctx *ctx, uint32_t group_id) > > { > > - if (xlate_resubmit_resource_check(ctx)) { > > + if (xlate_group_resource_check(ctx)) { > > struct group_dpif *group; > > bool got_group; > > > > @@ -2974,6 +3003,7 @@ xlate_actions__(struct xlate_in *xin, struct > > xlate_out *xout) > > > > ctx.recurse = 0; > > ctx.resubmits = 0; > > + ctx.in_group = false; > > ctx.orig_skb_priority = flow->skb_priority; > > ctx.table_id = 0; > > ctx.exit = false; > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index 328b215..7c5b941 100644 > > --- a/ofproto/ofproto-dpif.c > > +++ b/ofproto/ofproto-dpif.c > > @@ -3291,6 +3291,20 @@ static enum ofperr > > group_construct(struct ofgroup *group_) > > { > > struct group_dpif *group = group_dpif_cast(group_); > > + const struct ofputil_bucket *bucket; > > + > > + /* Prevent group chaining because our locking structure makes it hard > > to > > + * implement deadlock-free. (See xlate_group_resource_check().) */ > > + LIST_FOR_EACH (bucket, list_node, &group->up.buckets) { > > + const struct ofpact *a; > > + > > + OFPACT_FOR_EACH (a, bucket->ofpacts, bucket->ofpacts_len) { > > + if (a->type == OFPACT_GROUP) { > > + return OFPERR_OFPGMFC_CHAINING_UNSUPPORTED; > > + } > > + } > > + } > > + > > ovs_mutex_init_adaptive(&group->stats_mutex); > > ovs_mutex_lock(&group->stats_mutex); > > group_construct_stats(group); > > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > > index 06c4046..6d48e5a 100644 > > --- a/tests/ofproto-dpif.at > > +++ b/tests/ofproto-dpif.at > > @@ -112,6 +112,17 @@ AT_CHECK([tail -1 stdout], [0], > > OVS_VSWITCHD_STOP > > AT_CLEANUP > > > > +AT_SETUP([ofproto-dpif - group chaining not supported]) > > +OVS_VSWITCHD_START > > +ADD_OF_PORTS([br0], [1], [10], [11]) > > +AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0 > > 'group_id=1234,type=all,bucket=output:10,set_field:192.168.3.90->ip_src,group:123,bucket=output:11'], > > + [1], [], [stderr]) > > +AT_CHECK([STRIP_XIDS stderr | sed 1q], [0], > > + [OFPT_ERROR (OF1.2): OFPGMFC_CHAINING_UNSUPPORTED > > +]) > > +OVS_VSWITCHD_STOP > > +AT_CLEANUP > > + > > AT_SETUP([ofproto-dpif - all group in action list]) > > OVS_VSWITCHD_START > > ADD_OF_PORTS([br0], [1], [10], [11]) > > -- > > 1.7.10.4 > > > > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev