[ovs-dev] [rwlock 3/5] ofproto-dpif-xlate: Avoid recursively taking read side of ofgroup rwlock.

Ben Pfaff blp at nicira.com
Thu Jan 16 18:07:31 UTC 2014


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 <blp at 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




More information about the dev mailing list