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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250311001103.wkbk6y3b736kcf2k@skbuf>
Date: Tue, 11 Mar 2025 02:11:03 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Amit Cohen <amcohen@...dia.com>, Kuniyuki Iwashima <kuniyu@...zon.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,
	horms@...nel.org, tobias@...dekranz.com
Subject: Re: [PATCH net] net: switchdev: Convert blocking notification chain
 to a raw one

Hi Amit,

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.
> 
> 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: Vladimir Oltean <olteanv@...il.com>
Tested-by: Vladimir Oltean <olteanv@...il.com>