| Message ID | 20100428010336.237294971@vyatta.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Apr 27, 2010 at 06:01:06PM -0700, Stephen Hemminger wrote: > The recently introduced bridge mulitcast port group list was only > partially using RCU correctly. It was missing rcu_dereference() > and missing the necessary barrier on deletion. > > The code should have used one of the standard list methods (list or hlist) > instead of open coding a RCU based link list. > > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> > > --- a/net/bridge/br_forward.c 2010-04-27 17:51:27.909588950 -0700 > +++ b/net/bridge/br_forward.c 2010-04-27 17:53:18.790721091 -0700 > @@ -217,7 +217,7 @@ static void br_multicast_flood(struct ne > prev = NULL; > > rp = rcu_dereference(br->router_list.first); > - p = mdst ? mdst->ports : NULL; > + p = mdst ? rcu_dereference(mdst->ports) : NULL; > while (p || rp) { > lport = p ? p->port : NULL; > rport = rp ? hlist_entry(rp, struct net_bridge_port, rlist) : > @@ -231,7 +231,7 @@ static void br_multicast_flood(struct ne > goto out; > > if ((unsigned long)lport >= (unsigned long)port) > - p = p->next; > + p = rcu_dereference(p->next); > if ((unsigned long)rport >= (unsigned long)port) > rp = rcu_dereference(rp->next); > } Thanks for catching this! > --- a/net/bridge/br_multicast.c 2010-04-27 17:51:31.509593914 -0700 > +++ b/net/bridge/br_multicast.c 2010-04-27 17:52:48.209243982 -0700 > @@ -259,7 +259,7 @@ static void br_multicast_del_pg(struct n > if (p != pg) > continue; > > - *pp = p->next; > + rcu_assign_pointer(*pp, p->next); But this is bogus. br_multicast_del_pg is removing an entry from the RCU list. You only need write barriers when you're putting a new entry in it, and only if there is no other barrier between the code filling in the new entry and the line adding it to the RCU list. Cheers,
On Wed, 28 Apr 2010 11:07:09 +0800 Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Tue, Apr 27, 2010 at 06:01:06PM -0700, Stephen Hemminger wrote: > > The recently introduced bridge mulitcast port group list was only > > partially using RCU correctly. It was missing rcu_dereference() > > and missing the necessary barrier on deletion. > > > > The code should have used one of the standard list methods (list or hlist) > > instead of open coding a RCU based link list. > > > > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> > > > > --- a/net/bridge/br_forward.c 2010-04-27 17:51:27.909588950 -0700 > > +++ b/net/bridge/br_forward.c 2010-04-27 17:53:18.790721091 -0700 > > @@ -217,7 +217,7 @@ static void br_multicast_flood(struct ne > > prev = NULL; > > > > rp = rcu_dereference(br->router_list.first); > > - p = mdst ? mdst->ports : NULL; > > + p = mdst ? rcu_dereference(mdst->ports) : NULL; > > while (p || rp) { > > lport = p ? p->port : NULL; > > rport = rp ? hlist_entry(rp, struct net_bridge_port, rlist) : > > @@ -231,7 +231,7 @@ static void br_multicast_flood(struct ne > > goto out; > > > > if ((unsigned long)lport >= (unsigned long)port) > > - p = p->next; > > + p = rcu_dereference(p->next); > > if ((unsigned long)rport >= (unsigned long)port) > > rp = rcu_dereference(rp->next); > > } > > Thanks for catching this! > > > --- a/net/bridge/br_multicast.c 2010-04-27 17:51:31.509593914 -0700 > > +++ b/net/bridge/br_multicast.c 2010-04-27 17:52:48.209243982 -0700 > > @@ -259,7 +259,7 @@ static void br_multicast_del_pg(struct n > > if (p != pg) > > continue; > > > > - *pp = p->next; > > + rcu_assign_pointer(*pp, p->next); > > But this is bogus. br_multicast_del_pg is removing an entry from > the RCU list. > > You only need write barriers when you're putting a new entry in > it, and only if there is no other barrier between the code filling > in the new entry and the line adding it to the RCU list. Yeah, it is extra barrier (one more reason to stick to hlist_del_rcu)
--- a/net/bridge/br_forward.c 2010-04-27 17:51:27.909588950 -0700 +++ b/net/bridge/br_forward.c 2010-04-27 17:53:18.790721091 -0700 @@ -217,7 +217,7 @@ static void br_multicast_flood(struct ne prev = NULL; rp = rcu_dereference(br->router_list.first); - p = mdst ? mdst->ports : NULL; + p = mdst ? rcu_dereference(mdst->ports) : NULL; while (p || rp) { lport = p ? p->port : NULL; rport = rp ? hlist_entry(rp, struct net_bridge_port, rlist) : @@ -231,7 +231,7 @@ static void br_multicast_flood(struct ne goto out; if ((unsigned long)lport >= (unsigned long)port) - p = p->next; + p = rcu_dereference(p->next); if ((unsigned long)rport >= (unsigned long)port) rp = rcu_dereference(rp->next); } --- a/net/bridge/br_multicast.c 2010-04-27 17:51:31.509593914 -0700 +++ b/net/bridge/br_multicast.c 2010-04-27 17:52:48.209243982 -0700 @@ -259,7 +259,7 @@ static void br_multicast_del_pg(struct n if (p != pg) continue; - *pp = p->next; + rcu_assign_pointer(*pp, p->next); hlist_del_init(&p->mglist); del_timer(&p->timer); del_timer(&p->query_timer);
The recently introduced bridge mulitcast port group list was only partially using RCU correctly. It was missing rcu_dereference() and missing the necessary barrier on deletion. The code should have used one of the standard list methods (list or hlist) instead of open coding a RCU based link list. Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>