| Message ID | 20240712143415.1141039-1-leitao@debian.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 97d9fba9a812cada5484667a46e14a4c976ca330 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] net: netconsole: Disable target before netpoll cleanup | expand |
On Fri, Jul 12, 2024 at 7:34 AM Breno Leitao <leitao@debian.org> wrote: > > Currently, netconsole cleans up the netpoll structure before disabling > the target. This approach can lead to race conditions, as message > senders (write_ext_msg() and write_msg()) check if the target is > enabled before using netpoll. The sender can validate that the target is > enabled, but, the netpoll might be de-allocated already, causing > undesired behaviours. > > This patch reverses the order of operations: > 1. Disable the target > 2. Clean up the netpoll structure > > This change eliminates the potential race condition, ensuring that > no messages are sent through a partially cleaned-up netpoll structure. > > Fixes: 2382b15bcc39 ("netconsole: take care of NETDEV_UNREGISTER event") > Cc: stable@vger.kernel.org > Signed-off-by: Breno Leitao <leitao@debian.org> Goof catch, thanks. Reviewed-by: Eric Dumazet <edumazet@google.com>
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Fri, 12 Jul 2024 07:34:15 -0700 you wrote: > Currently, netconsole cleans up the netpoll structure before disabling > the target. This approach can lead to race conditions, as message > senders (write_ext_msg() and write_msg()) check if the target is > enabled before using netpoll. The sender can validate that the target is > enabled, but, the netpoll might be de-allocated already, causing > undesired behaviours. > > [...] Here is the summary with links: - [net,v2] net: netconsole: Disable target before netpoll cleanup https://git.kernel.org/netdev/net/c/97d9fba9a812 You are awesome, thank you!
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index d7070dd4fe73..aa66c923790f 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -974,6 +974,7 @@ static int netconsole_netdev_event(struct notifier_block *this, /* rtnl_lock already held * we might sleep in __netpoll_cleanup() */ + nt->enabled = false; spin_unlock_irqrestore(&target_list_lock, flags); __netpoll_cleanup(&nt->np); @@ -981,7 +982,6 @@ static int netconsole_netdev_event(struct notifier_block *this, spin_lock_irqsave(&target_list_lock, flags); netdev_put(nt->np.dev, &nt->np.dev_tracker); nt->np.dev = NULL; - nt->enabled = false; stopped = true; netconsole_target_put(nt); goto restart;
Currently, netconsole cleans up the netpoll structure before disabling the target. This approach can lead to race conditions, as message senders (write_ext_msg() and write_msg()) check if the target is enabled before using netpoll. The sender can validate that the target is enabled, but, the netpoll might be de-allocated already, causing undesired behaviours. This patch reverses the order of operations: 1. Disable the target 2. Clean up the netpoll structure This change eliminates the potential race condition, ensuring that no messages are sent through a partially cleaned-up netpoll structure. Fixes: 2382b15bcc39 ("netconsole: take care of NETDEV_UNREGISTER event") Cc: stable@vger.kernel.org Signed-off-by: Breno Leitao <leitao@debian.org> --- Changelog: v2: * Targeting "net" instead of "net-dev" (Jakub) v1: * https://lore.kernel.org/all/20240709144403.544099-4-leitao@debian.org/ drivers/net/netconsole.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)