From ed56f8bd6a668df3664f68b00c5396c268aa1821 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Mon, 20 Aug 2018 20:25:51 -0700 Subject: [PATCH] ofproto-dpif-xlate: Avoid deadlock on multicast snooping recursion. Until now, OVS did multicast snooping outputs holding the read-lock on the mcast_snooping object. This could recurse via a patch port to try to take the write-lock on the same object, which deadlocked. This patch fixes the problem, by releasing the read-lock before doing any outputs. It would probably be better to use RCU for mcast_snooping. That would be a bigger patch and less suitable for backporting. Reported-by: Sameh Elsharkawy Reported-at: https://github.com/openvswitch/ovs-issues/issues/153 Signed-off-by: Ben Pfaff --- ofproto/ofproto-dpif-xlate.c | 102 ++++++++++++++++++++++++++++------- 1 file changed, 84 insertions(+), 18 deletions(-) diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 77c08aa6551..de0b56b6147 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -518,6 +518,8 @@ static bool may_receive(const struct xport *, struct xlate_ctx *); static void do_xlate_actions(const struct ofpact *, size_t ofpacts_len, struct xlate_ctx *); static void xlate_normal(struct xlate_ctx *); +static void xlate_normal_flood(struct xlate_ctx *ct, + struct xbundle *in_xbundle, struct xvlan *); static void xlate_table_action(struct xlate_ctx *, ofp_port_t in_port, uint8_t table_id, bool may_packet_in, bool honor_table_miss, bool with_ct_orig); @@ -2549,6 +2551,53 @@ update_mcast_snooping_table(const struct xlate_ctx *ctx, } ovs_rwlock_unlock(&ms->rwlock); } + +/* A list of multicast output ports. + * + * We accumulate output ports and then do all the outputs afterward. It would + * be more natural to do the outputs one at a time as we discover the need for + * each one, but this can cause a deadlock because we need to take the + * mcast_snooping's rwlock for reading to iterate through the port lists and + * doing an output, if it goes to a patch port, can eventually come back to the + * same mcast_snooping and attempt to take the write lock (see + * https://github.com/openvswitch/ovs-issues/issues/153). */ +struct mcast_output { + /* Discrete ports. */ + struct xbundle **xbundles; + size_t n, allocated; + + /* If set, flood to all ports. */ + bool flood; +}; +#define MCAST_OUTPUT_INIT { NULL, 0, 0, false } + +/* Add 'mcast_bundle' to 'out'. */ +static void +mcast_output_add(struct mcast_output *out, struct xbundle *mcast_xbundle) +{ + if (out->n >= out->allocated) { + out->xbundles = x2nrealloc(out->xbundles, &out->allocated, + sizeof *out->xbundles); + } + out->xbundles[out->n++] = mcast_xbundle; +} + +/* Outputs the packet in 'ctx' to all of the output ports in 'out', given input + * bundle 'in_xbundle' and the current 'xvlan'. */ +static void +mcast_output_finish(struct xlate_ctx *ctx, struct mcast_output *out, + struct xbundle *in_xbundle, struct xvlan *xvlan) +{ + if (out->flood) { + xlate_normal_flood(ctx, in_xbundle, xvlan); + } else { + for (size_t i = 0; i < out->n; i++) { + output_normal(ctx, out->xbundles[i], xvlan); + } + } + + free(out->xbundles); +} /* send the packet to ports having the multicast group learned */ static void @@ -2556,7 +2605,7 @@ xlate_normal_mcast_send_group(struct xlate_ctx *ctx, struct mcast_snooping *ms OVS_UNUSED, struct mcast_group *grp, struct xbundle *in_xbundle, - const struct xvlan *xvlan) + struct mcast_output *out) OVS_REQ_RDLOCK(ms->rwlock) { struct xlate_cfg *xcfg; @@ -2568,7 +2617,7 @@ xlate_normal_mcast_send_group(struct xlate_ctx *ctx, mcast_xbundle = xbundle_lookup(xcfg, b->port); if (mcast_xbundle && mcast_xbundle != in_xbundle) { xlate_report(ctx, OFT_DETAIL, "forwarding to mcast group port"); - output_normal(ctx, mcast_xbundle, xvlan); + mcast_output_add(out, mcast_xbundle); } else if (!mcast_xbundle) { xlate_report(ctx, OFT_WARN, "mcast group port is unknown, dropping"); @@ -2584,7 +2633,8 @@ static void xlate_normal_mcast_send_mrouters(struct xlate_ctx *ctx, struct mcast_snooping *ms, struct xbundle *in_xbundle, - const struct xvlan *xvlan) + const struct xvlan *xvlan, + struct mcast_output *out) OVS_REQ_RDLOCK(ms->rwlock) { struct xlate_cfg *xcfg; @@ -2597,7 +2647,7 @@ xlate_normal_mcast_send_mrouters(struct xlate_ctx *ctx, if (mcast_xbundle && mcast_xbundle != in_xbundle && mrouter->vlan == xvlan->v[0].vid) { xlate_report(ctx, OFT_DETAIL, "forwarding to mcast router port"); - output_normal(ctx, mcast_xbundle, xvlan); + mcast_output_add(out, mcast_xbundle); } else if (!mcast_xbundle) { xlate_report(ctx, OFT_WARN, "mcast router port is unknown, dropping"); @@ -2616,7 +2666,7 @@ static void xlate_normal_mcast_send_fports(struct xlate_ctx *ctx, struct mcast_snooping *ms, struct xbundle *in_xbundle, - const struct xvlan *xvlan) + struct mcast_output *out) OVS_REQ_RDLOCK(ms->rwlock) { struct xlate_cfg *xcfg; @@ -2628,7 +2678,7 @@ xlate_normal_mcast_send_fports(struct xlate_ctx *ctx, mcast_xbundle = xbundle_lookup(xcfg, fport->port); if (mcast_xbundle && mcast_xbundle != in_xbundle) { xlate_report(ctx, OFT_DETAIL, "forwarding to mcast flood port"); - output_normal(ctx, mcast_xbundle, xvlan); + mcast_output_add(out, mcast_xbundle); } else if (!mcast_xbundle) { xlate_report(ctx, OFT_WARN, "mcast flood port is unknown, dropping"); @@ -2644,7 +2694,7 @@ static void xlate_normal_mcast_send_rports(struct xlate_ctx *ctx, struct mcast_snooping *ms, struct xbundle *in_xbundle, - const struct xvlan *xvlan) + struct mcast_output *out) OVS_REQ_RDLOCK(ms->rwlock) { struct xlate_cfg *xcfg; @@ -2659,7 +2709,7 @@ xlate_normal_mcast_send_rports(struct xlate_ctx *ctx, && mcast_xbundle->ofbundle != in_xbundle->ofbundle) { xlate_report(ctx, OFT_DETAIL, "forwarding report to mcast flagged port"); - output_normal(ctx, mcast_xbundle, xvlan); + mcast_output_add(out, mcast_xbundle); } else if (!mcast_xbundle) { xlate_report(ctx, OFT_WARN, "mcast port is unknown, dropping the report"); @@ -2811,8 +2861,11 @@ xlate_normal(struct xlate_ctx *ctx) } if (mcast_snooping_is_membership(flow->tp_src)) { + struct mcast_output out = MCAST_OUTPUT_INIT; + ovs_rwlock_rdlock(&ms->rwlock); - xlate_normal_mcast_send_mrouters(ctx, ms, in_xbundle, &xvlan); + xlate_normal_mcast_send_mrouters(ctx, ms, in_xbundle, &xvlan, + &out); /* RFC4541: section 2.1.1, item 1: A snooping switch should * forward IGMP Membership Reports only to those ports where * multicast routers are attached. Alternatively stated: a @@ -2821,8 +2874,10 @@ xlate_normal(struct xlate_ctx *ctx) * An administrative control may be provided to override this * restriction, allowing the report messages to be flooded to * other ports. */ - xlate_normal_mcast_send_rports(ctx, ms, in_xbundle, &xvlan); + xlate_normal_mcast_send_rports(ctx, ms, in_xbundle, &out); ovs_rwlock_unlock(&ms->rwlock); + + mcast_output_finish(ctx, &out, in_xbundle, &xvlan); } else { xlate_report(ctx, OFT_DETAIL, "multicast traffic, flooding"); xlate_normal_flood(ctx, in_xbundle, &xvlan); @@ -2835,10 +2890,15 @@ xlate_normal(struct xlate_ctx *ctx) in_xbundle, ctx->xin->packet); } if (is_mld_report(flow, wc)) { + struct mcast_output out = MCAST_OUTPUT_INIT; + ovs_rwlock_rdlock(&ms->rwlock); - xlate_normal_mcast_send_mrouters(ctx, ms, in_xbundle, &xvlan); - xlate_normal_mcast_send_rports(ctx, ms, in_xbundle, &xvlan); + xlate_normal_mcast_send_mrouters(ctx, ms, in_xbundle, &xvlan, + &out); + xlate_normal_mcast_send_rports(ctx, ms, in_xbundle, &out); ovs_rwlock_unlock(&ms->rwlock); + + mcast_output_finish(ctx, &out, in_xbundle, &xvlan); } else { xlate_report(ctx, OFT_DETAIL, "MLD query, flooding"); xlate_normal_flood(ctx, in_xbundle, &xvlan); @@ -2856,6 +2916,8 @@ xlate_normal(struct xlate_ctx *ctx) } /* forwarding to group base ports */ + struct mcast_output out = MCAST_OUTPUT_INIT; + ovs_rwlock_rdlock(&ms->rwlock); if (flow->dl_type == htons(ETH_TYPE_IP)) { grp = mcast_snooping_lookup4(ms, flow->nw_dst, vlan); @@ -2863,20 +2925,24 @@ xlate_normal(struct xlate_ctx *ctx) grp = mcast_snooping_lookup(ms, &flow->ipv6_dst, vlan); } if (grp) { - xlate_normal_mcast_send_group(ctx, ms, grp, in_xbundle, &xvlan); - xlate_normal_mcast_send_fports(ctx, ms, in_xbundle, &xvlan); - xlate_normal_mcast_send_mrouters(ctx, ms, in_xbundle, &xvlan); + xlate_normal_mcast_send_group(ctx, ms, grp, in_xbundle, &out); + xlate_normal_mcast_send_fports(ctx, ms, in_xbundle, &out); + xlate_normal_mcast_send_mrouters(ctx, ms, in_xbundle, &xvlan, + &out); } else { if (mcast_snooping_flood_unreg(ms)) { xlate_report(ctx, OFT_DETAIL, "unregistered multicast, flooding"); - xlate_normal_flood(ctx, in_xbundle, &xvlan); + out.flood = true; } else { - xlate_normal_mcast_send_mrouters(ctx, ms, in_xbundle, &xvlan); - xlate_normal_mcast_send_fports(ctx, ms, in_xbundle, &xvlan); + xlate_normal_mcast_send_mrouters(ctx, ms, in_xbundle, &xvlan, + &out); + xlate_normal_mcast_send_fports(ctx, ms, in_xbundle, &out); } } ovs_rwlock_unlock(&ms->rwlock); + + mcast_output_finish(ctx, &out, in_xbundle, &xvlan); } else { ovs_rwlock_rdlock(&ctx->xbridge->ml->rwlock); mac = mac_learning_lookup(ctx->xbridge->ml, flow->dl_dst, vlan);