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?
On 16 January 2014 10:07, Ben Pfaff <b...@nicira.com> wrote:
> 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 +++++++++++++++++++++++++++++++-
> 1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 4747ea7..e0b1bc6 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -180,6 +180,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. */
> @@ -1992,6 +1993,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:
> @@ -2007,12 +2010,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;
>
> @@ -3088,6 +3117,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;
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
