<div dir="ltr">This looks good to me, although aren't we meant to report something at the OpenFlow layer?<div><br><div>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?<br>
</div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On 16 January 2014 10:07, Ben Pfaff <span dir="ltr"><<a href="mailto:blp@nicira.com" target="_blank">blp@nicira.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">With glibc, rwlocks by default allow recursive read-locking even if a<br>
thread is blocked waiting for the write-lock. POSIX allows such attempts<br>
to deadlock, and it appears that the libc used in NetBSD, at least, does<br>
deadlock. ofproto-dpif-xlate could take the ofgroup rwlock recursively if<br>
an ofgroup's actions caused the ofgroup to be executed again. This commit<br>
avoids that issue by preventing recursive translation of groups (the same<br>
group or another group). This is not the most user friendly solution,<br>
but OpenFlow allows this restriction, and we can always remove the<br>
restriction later (probably requiring more complicated code) if it<br>
proves to be a real problem to real users.<br>
<br>
Found by inspection.<br>
<br>
Signed-off-by: Ben Pfaff <<a href="mailto:blp@nicira.com">blp@nicira.com</a>><br>
---<br>
ofproto/ofproto-dpif-xlate.c | 32 +++++++++++++++++++++++++++++++-<br>
1 file changed, 31 insertions(+), 1 deletion(-)<br>
<br>
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c<br>
index 4747ea7..e0b1bc6 100644<br>
--- a/ofproto/ofproto-dpif-xlate.c<br>
+++ b/ofproto/ofproto-dpif-xlate.c<br>
@@ -180,6 +180,7 @@ struct xlate_ctx {<br>
/* Resubmit statistics, via xlate_table_action(). */<br>
int recurse; /* Current resubmit nesting depth. */<br>
int resubmits; /* Total number of resubmits. */<br>
+ bool in_group; /* Currently translating ofgroup, if true. */<br>
<br>
uint32_t orig_skb_priority; /* Priority when packet arrived. */<br>
uint8_t table_id; /* OpenFlow table ID where flow was found. */<br>
@@ -1992,6 +1993,8 @@ xlate_select_group(struct xlate_ctx *ctx, struct group_dpif *group)<br>
static void<br>
xlate_group_action__(struct xlate_ctx *ctx, struct group_dpif *group)<br>
{<br>
+ ctx->in_group = true;<br>
+<br>
switch (group_dpif_get_type(group)) {<br>
case OFPGT11_ALL:<br>
case OFPGT11_INDIRECT:<br>
@@ -2007,12 +2010,38 @@ xlate_group_action__(struct xlate_ctx *ctx, struct group_dpif *group)<br>
OVS_NOT_REACHED();<br>
}<br>
group_dpif_release(group);<br>
+<br>
+ ctx->in_group = false;<br>
+}<br>
+<br>
+static bool<br>
+xlate_group_resource_check(struct xlate_ctx *ctx)<br>
+{<br>
+ if (!xlate_resubmit_resource_check(ctx)) {<br>
+ return false;<br>
+ } else if (ctx->in_group) {<br>
+ /* Prevent nested translation of OpenFlow groups.<br>
+ *<br>
+ * OpenFlow allows this restriction. We enforce this restriction only<br>
+ * because, with the current architecture, we would otherwise have to<br>
+ * take a possibly recursive read lock on the ofgroup rwlock, which is<br>
+ * unsafe given that POSIX allows taking a read lock to block if there<br>
+ * is a thread blocked on taking the write lock. Other solutions<br>
+ * without this restriction are also possible, but seem unwarranted<br>
+ * given the current limited use of groups. */<br>
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);<br>
+<br>
+ VLOG_ERR_RL(&rl, "cannot recursively translate OpenFlow group");<br>
+ return false;<br>
+ } else {<br>
+ return true;<br>
+ }<br>
}<br>
<br>
static bool<br>
xlate_group_action(struct xlate_ctx *ctx, uint32_t group_id)<br>
{<br>
- if (xlate_resubmit_resource_check(ctx)) {<br>
+ if (xlate_group_resource_check(ctx)) {<br>
struct group_dpif *group;<br>
bool got_group;<br>
<br>
@@ -3088,6 +3117,7 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out *xout)<br>
<br>
ctx.recurse = 0;<br>
ctx.resubmits = 0;<br>
+ ctx.in_group = false;<br>
ctx.orig_skb_priority = flow->skb_priority;<br>
ctx.table_id = 0;<br>
ctx.exit = false;<br>
<span class="HOEnZb"><font color="#888888">--<br>
1.7.10.4<br>
<br>
_______________________________________________<br>
dev mailing list<br>
<a href="mailto:dev@openvswitch.org">dev@openvswitch.org</a><br>
<a href="http://openvswitch.org/mailman/listinfo/dev" target="_blank">http://openvswitch.org/mailman/listinfo/dev</a><br>
</font></span></blockquote></div><br></div>