netdev - Re: [PATCH net] net: switchdev: Convert blocking notification chain to a raw one

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250310061737.GA4159220@kernel.org>
Date: Mon, 10 Mar 2025 06:17:37 +0000
From: Simon Horman <horms@...nel.org>
To: Amit Cohen <amcohen@...dia.com>
Cc: netdev@...r.kernel.org, idosch@...dia.com, petrm@...dia.com,
	jiri@...nulli.us, ivecera@...hat.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	olteanv@...il.com, tobias@...dekranz.com
Subject: Re: [PATCH net] net: switchdev: Convert blocking notification chain
 to a raw one

On Wed, Mar 05, 2025 at 02:15:09PM +0200, Amit Cohen wrote:
> A blocking notification chain uses a read-write semaphore to protect the
> integrity of the chain. The semaphore is acquired for writing when
> adding / removing notifiers to / from the chain and acquired for reading
> when traversing the chain and informing notifiers about an event.
> 
> In case of the blocking switchdev notification chain, recursive
> notifications are possible which leads to the semaphore being acquired
> twice for reading and to lockdep warnings being generated [1].
> 
> Specifically, this can happen when the bridge driver processes a
> SWITCHDEV_BRPORT_UNOFFLOADED event which causes it to emit notifications
> about deferred events when calling switchdev_deferred_process().
> 
> Fix this by converting the notification chain to a raw notification
> chain in a similar fashion to the netdev notification chain. Protect
> the chain using the RTNL mutex by acquiring it when modifying the chain.
> Events are always informed under the RTNL mutex, but add an assertion in
> call_switchdev_blocking_notifiers() to make sure this is not violated in
> the future.

Hi Amit,

As you may be aware there is quite some activity to reduce the reliance on
RTNL. However, as the events in question are already protected by RTNL
I think the approach you have taken here is entirely reasonable.

> 
> Maintain the "blocking" prefix as events are always emitted from process
> context and listeners are allowed to block.
> 
> [1]:
> WARNING: possible recursive locking detected
> 6.14.0-rc4-custom-g079270089484 #1 Not tainted
> --------------------------------------------
> ip/52731 is trying to acquire lock:
> ffffffff850918d8 ((switchdev_blocking_notif_chain).rwsem){++++}-{4:4}, at: blocking_notifier_call_chain+0x58/0xa0
> 
> but task is already holding lock:
> ffffffff850918d8 ((switchdev_blocking_notif_chain).rwsem){++++}-{4:4}, at: blocking_notifier_call_chain+0x58/0xa0
> 
> other info that might help us debug this:
> Possible unsafe locking scenario:
> CPU0
> ----
> lock((switchdev_blocking_notif_chain).rwsem);
> lock((switchdev_blocking_notif_chain).rwsem);
> 
> *** DEADLOCK ***
> May be due to missing lock nesting notation
> 3 locks held by ip/52731:
>  #0: ffffffff84f795b0 (rtnl_mutex){+.+.}-{4:4}, at: rtnl_newlink+0x727/0x1dc0
>  #1: ffffffff8731f628 (&net->rtnl_mutex){+.+.}-{4:4}, at: rtnl_newlink+0x790/0x1dc0
>  #2: ffffffff850918d8 ((switchdev_blocking_notif_chain).rwsem){++++}-{4:4}, at: blocking_notifier_call_chain+0x58/0xa0
> 
> stack backtrace:
> ...
> ? __pfx_down_read+0x10/0x10
> ? __pfx_mark_lock+0x10/0x10
> ? __pfx_switchdev_port_attr_set_deferred+0x10/0x10
> blocking_notifier_call_chain+0x58/0xa0
> switchdev_port_attr_notify.constprop.0+0xb3/0x1b0
> ? __pfx_switchdev_port_attr_notify.constprop.0+0x10/0x10
> ? mark_held_locks+0x94/0xe0
> ? switchdev_deferred_process+0x11a/0x340
> switchdev_port_attr_set_deferred+0x27/0xd0
> switchdev_deferred_process+0x164/0x340
> br_switchdev_port_unoffload+0xc8/0x100 [bridge]
> br_switchdev_blocking_event+0x29f/0x580 [bridge]
> notifier_call_chain+0xa2/0x440
> blocking_notifier_call_chain+0x6e/0xa0
> switchdev_bridge_port_unoffload+0xde/0x1a0
> ...
> 
> Fixes: f7a70d650b0b6 ("net: bridge: switchdev: Ensure deferred event delivery on unoffload")
> Signed-off-by: Amit Cohen <amcohen@...dia.com>
> Reviewed-by: Ido Schimmel <idosch@...dia.com>

Reviewed-by: Simon Horman <horms@...nel.org>

...