Petr Machata
2018-May-28 15:44 UTC
[Bridge] [PATCH net-next] net: bridge: Lock before br_fdb_find()
Callers of br_fdb_find() need to hold the hash lock, which br_fdb_find_port() doesn't do. Add the missing lock/unlock pair. Signed-off-by: Petr Machata <petrm at mellanox.com> --- net/bridge/br_fdb.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c index b19e310..3f5691a 100644 --- a/net/bridge/br_fdb.c +++ b/net/bridge/br_fdb.c @@ -135,9 +135,11 @@ struct net_device *br_fdb_find_port(const struct net_device *br_dev, return NULL; br = netdev_priv(br_dev); + spin_lock_bh(&br->hash_lock); f = br_fdb_find(br, addr, vid); if (f && f->dst) dev = f->dst->dev; + spin_unlock_bh(&br->hash_lock); return dev; } -- 2.4.11
Nikolay Aleksandrov
2018-May-28 15:52 UTC
[Bridge] [PATCH net-next] net: bridge: Lock before br_fdb_find()
On 28/05/18 18:44, Petr Machata wrote:> Callers of br_fdb_find() need to hold the hash lock, which > br_fdb_find_port() doesn't do. Add the missing lock/unlock > pair. > > Signed-off-by: Petr Machata <petrm at mellanox.com> > --- > net/bridge/br_fdb.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c > index b19e310..3f5691a 100644 > --- a/net/bridge/br_fdb.c > +++ b/net/bridge/br_fdb.c > @@ -135,9 +135,11 @@ struct net_device *br_fdb_find_port(const struct net_device *br_dev, > return NULL; > > br = netdev_priv(br_dev); > + spin_lock_bh(&br->hash_lock); > f = br_fdb_find(br, addr, vid); > if (f && f->dst) > dev = f->dst->dev; > + spin_unlock_bh(&br->hash_lock); > > return dev; > } >There's also a lockdep assert for hash_lock in br_find_fdb(). Acked-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
Stephen Hemminger
2018-May-28 17:42 UTC
[Bridge] [PATCH net-next] net: bridge: Lock before br_fdb_find()
On Mon, 28 May 2018 17:44:16 +0200 Petr Machata <petrm at mellanox.com> wrote:> Callers of br_fdb_find() need to hold the hash lock, which > br_fdb_find_port() doesn't do. Add the missing lock/unlock > pair. > > Signed-off-by: Petr Machata <petrm at mellanox.com> > --- > net/bridge/br_fdb.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c > index b19e310..3f5691a 100644 > --- a/net/bridge/br_fdb.c > +++ b/net/bridge/br_fdb.c > @@ -135,9 +135,11 @@ struct net_device *br_fdb_find_port(const struct net_device *br_dev, > return NULL; > > br = netdev_priv(br_dev); > + spin_lock_bh(&br->hash_lock); > f = br_fdb_find(br, addr, vid); > if (f && f->dst) > dev = f->dst->dev; > + spin_unlock_bh(&br->hash_lock); > > return dev; > }Sigh. when did br_fdb_find start needing hash_lock? What is the point of RCU then?
David Miller
2018-May-30 16:42 UTC
[Bridge] [PATCH net-next] net: bridge: Lock before br_fdb_find()
From: Petr Machata <petrm at mellanox.com> Date: Mon, 28 May 2018 17:44:16 +0200> Callers of br_fdb_find() need to hold the hash lock, which > br_fdb_find_port() doesn't do. Add the missing lock/unlock > pair. > > Signed-off-by: Petr Machata <petrm at mellanox.com>If all of the these uses of br_fdb_find_port() are safe, then it should use the RCU fdb lookup variant. So I basically agree with Stephen that this locking doesn't make any sense. The lock is needed when you are going to add or delete an FDB entry. Here we are doing a lookup and returning a device pointer via the FDB entry found in the lookup. The RTNL assertion assures that the device returned won't disappear. If the device can disappear, the spinlock added by this patch doesn't change that at all.