Skip to content

Commit

Permalink
ofproto-dpif-xlate: Avoid deadlock on multicast snooping recursion.
Browse files Browse the repository at this point in the history 
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: openvswitch/ovs-issues#153
Signed-off-by: Ben Pfaff <blp@ovn.org>
  • Loading branch information
@blp
blp committed Oct 15, 2018
1 parent 801be1c commit ed56f8b
Showing 1 changed file with 84 additions and 18 deletions.
102 changes: 84 additions & 18 deletions ofproto/ofproto-dpif-xlate.c
View file
Expand Up @@ -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);
Expand Down Expand Up @@ -2549,14 +2551,61 @@ 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
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;
Expand All @@ -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");
Expand All @@ -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;
Expand All @@ -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");
Expand All @@ -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;
Expand All @@ -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");
Expand All @@ -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;
Expand All @@ -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");
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -2856,27 +2916,33 @@ 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);
} else if (flow->dl_type == htons(ETH_TYPE_IPV6)) {
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);
Expand Down

0 comments on commit ed56f8b

Please sign in to comment.