Nikolay Aleksandrov
2021-Apr-25 16:45 UTC
[Bridge] [PATCH net 2/2] net: bridge: fix lockdep multicast_lock false positive splat
On 25/04/2021 18:57, Taehee Yoo wrote:> multicast_lock is a per-interface(bridge) lock. > This lock can be used recursively because interfaces can be used > recursively. So, it should use spin_lock_nested() but it doesn't. > So lockdep false positive splat occurred. > > Some inline helper functions are added. > These functions internally get 'subclass' variable, which is used as > parameter of spin_lock_nested() and use spin_lock_nested() with a > subclass parameter. > > Test commands: > ip link add br0 type bridge > ip link add bond0 type bond > ip link add br1 type bridge > ip link set br0 master bond0 > ip link set bond0 up > ip link set bond0 master br1 > ip link set br0 up > ip link set br1 up > ip link set br0 type bridge mcast_router 1 mcast_querier 1 > ip link set br1 type bridge mcast_querier 1 mcast_router 1 > > Splat looks like: > ===========================================> WARNING: possible recursive locking detected > 5.12.0-rc7+ #855 Not tainted > -------------------------------------------- > kworker/5:1/56 is trying to acquire lock: > ffff88810f833000 (&br->multicast_lock){+.-.}-{2:2}, at: > br_multicast_rcv+0x1484/0x5280 [bridge] >[snip]> > Fixes: eb1d16414339 ("bridge: Add core IGMP snooping support") > Signed-off-by: Taehee Yoo <ap420073 at gmail.com> > --- > > v2: > - No change > > net/bridge/br_mdb.c | 12 +-- > net/bridge/br_multicast.c | 146 +++++++++++++++++++++------------- > net/bridge/br_multicast_eht.c | 18 +++-- > net/bridge/br_private.h | 48 +++++++++++ > 4 files changed, 158 insertions(+), 66 deletions(-) >Hi Taehee, Ugh.. that's just very ugly. :) The setup you've described above is by all means invalid, but possible unfortunately. The bridge already checks if it's being added as a port to another bridge, but not through multiple levels of indirection. These locks are completely unrelated as they're in very different contexts (different devices). At the very least please push the rcu_read_lock() calls in br_multicast_lock_rcu/_bh() as they're needed only to get the nest level for netdev_get_nest_level_rcu(), we don't need them for the whole code paths (right ?), we could save a few lines in the process and avoid confusion about the locking rules for those code paths. I wish there was a better solution. Thanks, Nik
Herbert Xu
2021-Apr-26 12:48 UTC
[Bridge] [PATCH net 2/2] net: bridge: fix lockdep multicast_lock false positive splat
On Sun, Apr 25, 2021 at 07:45:27PM +0300, Nikolay Aleksandrov wrote:> > Ugh.. that's just very ugly. :) The setup you've described above is by all means invalid, but > possible unfortunately. The bridge already checks if it's being added as a port to another > bridge, but not through multiple levels of indirection. These locks are completely unrelated > as they're in very different contexts (different devices).Surely we should forbid this? Otherwise what's to stop someone from creating a loop? Cheers, -- Email: Herbert Xu <herbert at gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt