[PATCH net-next 0/3] netconsole: Fix potential race condition and improve code clarity

[PATCH net-next 0/3] netconsole: Fix potential race condition and improve code clarity

[Breno Leitao]

This patchset addresses a potential bug in netconsole where the netconsole
target is cleaned up before it is disabled. This sequence could lead to a
situation where an enabled target has an uninitialized netpoll structure,
potentially causing undefined behavior.

The main goals of this patchset are:

1. Correct the order of operations:
   - First, disable the netconsole target
   - Then, clean up the netpoll structure

2. Improve code readability:
   - Remove unnecessary casts
   - Eliminate redundant operations

These changes aim to enhance the reliability of netconsole by
eliminating the potential race condition and improve maintainability by
making the code more straightforward to understand and modify.

Breno Leitao (3):
  net: netconsole: Remove unnecessary cast from bool
  net: netconsole: Eliminate redundant setting of enabled field
  net: netconsole: Disable target before netpoll cleanup

 drivers/net/netconsole.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH net-next 1/3] net: netconsole: Remove unnecessary cast from bool

[Breno Leitao]

The 'enabled' variable is already a bool, so casting it to its value
is redundant.

Remove the superfluous cast, improving code clarity without changing
functionality.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index ab8a0623b1a1..de0d89e4e4e2 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -344,7 +344,7 @@ static ssize_t enabled_store(struct config_item *item,
 		goto out_unlock;
 
 	err = -EINVAL;
-	if ((bool)enabled == nt->enabled) {
+	if (enabled == nt->enabled) {
 		pr_info("network logging has already %s\n",
 			nt->enabled ? "started" : "stopped");
 		goto out_unlock;
-- 
2.43.0

[PATCH net-next 2/3] net: netconsole: Eliminate redundant setting of enabled field

[Breno Leitao]

When disabling a netconsole target, enabled_store() is called with
enabled=false. Currently, this results in updating the nt->enabled
field twice:

1. Inside the if/else block, with the target_list_lock spinlock held
2. Later, without the target_list_lock

This patch eliminates the redundancy by setting the field only once,
improving efficiency and reducing potential race conditions.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index de0d89e4e4e2..5ef680cf994a 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -369,6 +369,7 @@ static ssize_t enabled_store(struct config_item *item,
 		if (err)
 			goto out_unlock;
 
+		nt->enabled = true;
 		pr_info("network logging started\n");
 	} else {	/* false */
 		/* We need to disable the netconsole before cleaning it up
@@ -381,8 +382,6 @@ static ssize_t enabled_store(struct config_item *item,
 		netpoll_cleanup(&nt->np);
 	}
 
-	nt->enabled = enabled;
-
 	mutex_unlock(&dynamic_netconsole_mutex);
 	return strnlen(buf, count);
 out_unlock:
-- 
2.43.0

[PATCH net-next 3/3] net: netconsole: Disable target before netpoll cleanup

[Breno Leitao]

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.

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.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 5ef680cf994a..9c09293b5258 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -973,6 +973,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);
@@ -980,7 +981,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;
-- 
2.43.0

Re: [PATCH net-next 3/3] net: netconsole: Disable target before netpoll cleanup

[Jakub Kicinski]

On Tue,  9 Jul 2024 07:44:01 -0700 Breno Leitao 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.
> 
> 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.

I think this is a legit fix, please add a Fixes tag and resend for net.

Re: [PATCH net-next 0/3] netconsole: Fix potential race condition and improve code clarity

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  9 Jul 2024 07:43:58 -0700 you wrote:
> This patchset addresses a potential bug in netconsole where the netconsole
> target is cleaned up before it is disabled. This sequence could lead to a
> situation where an enabled target has an uninitialized netpoll structure,
> potentially causing undefined behavior.
> 
> The main goals of this patchset are:
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] net: netconsole: Remove unnecessary cast from bool
    https://git.kernel.org/netdev/net-next/c/a9359e8b0065
  - [net-next,2/3] net: netconsole: Eliminate redundant setting of enabled field
    https://git.kernel.org/netdev/net-next/c/0066623d4008
  - [net-next,3/3] net: netconsole: Disable target before netpoll cleanup
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html