[PATCH] net: netpoll: ensure skb_pool list is always initialized

[PATCH] net: netpoll: ensure skb_pool list is always initialized

[John Sperbeck]

When __netpoll_setup() is called directly, instead of through
netpoll_setup(), the np->skb_pool list head isn't initialized.
If skb_pool_flush() is later called, then we hit a NULL pointer
in skb_queue_purge_reason().  This can be seen with this repro,
when CONFIG_NETCONSOLE is enabled as a module:

    ip tuntap add mode tap tap0
    ip link add name br0 type bridge
    ip link set dev tap0 master br0
    modprobe netconsole netconsole=4444@10.0.0.1/br0,9353@10.0.0.2/
    rmmod netconsole

The backtrace is:

    BUG: kernel NULL pointer dereference, address: 0000000000000008
    #PF: supervisor write access in kernel mode
    #PF: error_code(0x0002) - not-present page
    ... ... ...
    Call Trace:
     <TASK>
     __netpoll_free+0xa5/0xf0
     br_netpoll_cleanup+0x43/0x50 [bridge]
     do_netpoll_cleanup+0x43/0xc0
     netconsole_netdev_event+0x1e3/0x300 [netconsole]
     unregister_netdevice_notifier+0xd9/0x150
     cleanup_module+0x45/0x920 [netconsole]
     __se_sys_delete_module+0x205/0x290
     do_syscall_64+0x70/0x150
     entry_SYSCALL_64_after_hwframe+0x76/0x7e

Move the skb_pool list initialization into __netpoll_setup().  Also,
have netpoll_setup() call this before allocating its initial pool of
packets.

Fixes: 6c59f16f1770 ("net: netpoll: flush skb pool during cleanup")
Signed-off-by: John Sperbeck <jsperbeck@google.com>
---
 net/core/netpoll.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 2e459b9d88eb..61662390414e 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -627,6 +627,8 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 	const struct net_device_ops *ops;
 	int err;
 
+	skb_queue_head_init(&np->skb_pool);
+
 	if (ndev->priv_flags & IFF_DISABLE_NETPOLL) {
 		np_err(np, "%s doesn't support polling, aborting\n",
 		       ndev->name);
@@ -681,8 +683,6 @@ int netpoll_setup(struct netpoll *np)
 	struct in_device *in_dev;
 	int err;
 
-	skb_queue_head_init(&np->skb_pool);
-
 	rtnl_lock();
 	if (np->dev_name[0]) {
 		struct net *net = current->nsproxy->net_ns;
@@ -782,17 +782,16 @@ int netpoll_setup(struct netpoll *np)
 		}
 	}
 
+	err = __netpoll_setup(np, ndev);
+	if (err)
+		goto put;
+
 	/* fill up the skb queue */
 	refill_skbs(np);
 
-	err = __netpoll_setup(np, ndev);
-	if (err)
-		goto flush;
 	rtnl_unlock();
 	return 0;
 
-flush:
-	skb_pool_flush(np);
 put:
 	DEBUG_NET_WARN_ON_ONCE(np->dev);
 	if (ip_overwritten)
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH] net: netpoll: ensure skb_pool list is always initialized

[Jakub Kicinski]

On Sat, 21 Dec 2024 17:23:34 -0800 John Sperbeck wrote:
> Move the skb_pool list initialization into __netpoll_setup().  Also,
> have netpoll_setup() call this before allocating its initial pool of
> packets.
> 
> Fixes: 6c59f16f1770 ("net: netpoll: flush skb pool during cleanup")

The fixes tag seems to be off by one? Wasn't the problem was introduced
by commit 221a9c1df790 ("net: netpoll: Individualize the skb pool") ?

Since __netpoll_setup() can be called by other drivers, shouldn't 
we move refill in there? Since the pool is per np?

Optionally, could you extend the netcons tests to exercise netcons over
vlan? I think it should be able to trigger the crash you're fixing?
-- 
pw-bot: cr

Re: [PATCH] net: netpoll: ensure skb_pool list is always initialized

[Breno Leitao]

On Mon, Dec 30, 2024 at 05:57:07PM -0800, Jakub Kicinski wrote:
> On Sat, 21 Dec 2024 17:23:34 -0800 John Sperbeck wrote:
> > Move the skb_pool list initialization into __netpoll_setup().  Also,
> > have netpoll_setup() call this before allocating its initial pool of
> > packets.
> > 
> > Fixes: 6c59f16f1770 ("net: netpoll: flush skb pool during cleanup")
> 
> The fixes tag seems to be off by one? Wasn't the problem was introduced
> by commit 221a9c1df790 ("net: netpoll: Individualize the skb pool") ?

You are correct. The regression was caused by 221a9c1df790 ("net:
netpoll: Individualize the skb pool"), when I mistakenly moved
skb_queue_head_init() out of netpoll_init() into netpoll_setup().

> Since __netpoll_setup() can be called by other drivers, shouldn't 
> we move refill in there? Since the pool is per np?

I'd say so.

It is not a big deal to have it in netpoll_setup(), since find_skb()
will refill the SKBs in the very first message being transmitted
(which will undesirably delay the very first netconsole TX).

On the other side, having it in refill_skbs() in the __netpoll_setup(),
as Jakub suggested, will avoid this extra delay in the very first TX,
and make the workflow cohesive.

Anyway, thanks John for spotting this regression and working in the fix,
--breno

Re: [PATCH] net: netpoll: ensure skb_pool list is always initialized

[Breno Leitao]

Hello John,

On Mon, Dec 30, 2024 at 05:57:07PM -0800, Jakub Kicinski wrote:
> On Sat, 21 Dec 2024 17:23:34 -0800 John Sperbeck wrote:
> > Move the skb_pool list initialization into __netpoll_setup().  Also,
> > have netpoll_setup() call this before allocating its initial pool of
> > packets.
> > 
> > Fixes: 6c59f16f1770 ("net: netpoll: flush skb pool during cleanup")
> 
> The fixes tag seems to be off by one? Wasn't the problem was introduced
> by commit 221a9c1df790 ("net: netpoll: Individualize the skb pool") ?
> 
> Since __netpoll_setup() can be called by other drivers, shouldn't 
> we move refill in there? Since the pool is per np?
> 
> Optionally, could you extend the netcons tests to exercise netcons over
> vlan? I think it should be able to trigger the crash you're fixing?

Are you planning to resend this fix with Jakub's suggestion?

Thanks
--breno

[PATCH v2] net: netpoll: ensure skb_pool list is always initialized

[John Sperbeck]

When __netpoll_setup() is called directly, instead of through
netpoll_setup(), the np->skb_pool list head isn't initialized.
If skb_pool_flush() is later called, then we hit a NULL pointer
in skb_queue_purge_reason().  This can be seen with this repro,
when CONFIG_NETCONSOLE is enabled as a module:

    ip tuntap add mode tap tap0
    ip link add name br0 type bridge
    ip link set dev tap0 master br0
    modprobe netconsole netconsole=4444@10.0.0.1/br0,9353@10.0.0.2/
    rmmod netconsole

The backtrace is:

    BUG: kernel NULL pointer dereference, address: 0000000000000008
    #PF: supervisor write access in kernel mode
    #PF: error_code(0x0002) - not-present page
    ... ... ...
    Call Trace:
     <TASK>
     __netpoll_free+0xa5/0xf0
     br_netpoll_cleanup+0x43/0x50 [bridge]
     do_netpoll_cleanup+0x43/0xc0
     netconsole_netdev_event+0x1e3/0x300 [netconsole]
     unregister_netdevice_notifier+0xd9/0x150
     cleanup_module+0x45/0x920 [netconsole]
     __se_sys_delete_module+0x205/0x290
     do_syscall_64+0x70/0x150
     entry_SYSCALL_64_after_hwframe+0x76/0x7e

Move the skb_pool list setup and initial skb fill into __netpoll_setup().

Fixes: 221a9c1df790 ("net: netpoll: Individualize the skb pool")
Signed-off-by: John Sperbeck <jsperbeck@google.com>
---
 net/core/netpoll.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 2e459b9d88eb..96a6ed37d4cc 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -627,6 +627,8 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 	const struct net_device_ops *ops;
 	int err;
 
+	skb_queue_head_init(&np->skb_pool);
+
 	if (ndev->priv_flags & IFF_DISABLE_NETPOLL) {
 		np_err(np, "%s doesn't support polling, aborting\n",
 		       ndev->name);
@@ -662,6 +664,9 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 	strscpy(np->dev_name, ndev->name, IFNAMSIZ);
 	npinfo->netpoll = np;
 
+	/* fill up the skb queue */
+	refill_skbs(np);
+
 	/* last thing to do is link it to the net device structure */
 	rcu_assign_pointer(ndev->npinfo, npinfo);
 
@@ -681,8 +686,6 @@ int netpoll_setup(struct netpoll *np)
 	struct in_device *in_dev;
 	int err;
 
-	skb_queue_head_init(&np->skb_pool);
-
 	rtnl_lock();
 	if (np->dev_name[0]) {
 		struct net *net = current->nsproxy->net_ns;
@@ -782,9 +785,6 @@ int netpoll_setup(struct netpoll *np)
 		}
 	}
 
-	/* fill up the skb queue */
-	refill_skbs(np);
-
 	err = __netpoll_setup(np, ndev);
 	if (err)
 		goto flush;
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v2] net: netpoll: ensure skb_pool list is always initialized

[Breno Leitao]

On Fri, Jan 10, 2025 at 04:32:38PM -0800, John Sperbeck wrote:
> When __netpoll_setup() is called directly, instead of through
> netpoll_setup(), the np->skb_pool list head isn't initialized.
> If skb_pool_flush() is later called, then we hit a NULL pointer
> in skb_queue_purge_reason().  This can be seen with this repro,
> when CONFIG_NETCONSOLE is enabled as a module:
> 
>     ip tuntap add mode tap tap0
>     ip link add name br0 type bridge
>     ip link set dev tap0 master br0
>     modprobe netconsole netconsole=4444@10.0.0.1/br0,9353@10.0.0.2/
>     rmmod netconsole
> 
> The backtrace is:
> 
>     BUG: kernel NULL pointer dereference, address: 0000000000000008
>     #PF: supervisor write access in kernel mode
>     #PF: error_code(0x0002) - not-present page
>     ... ... ...
>     Call Trace:
>      <TASK>
>      __netpoll_free+0xa5/0xf0
>      br_netpoll_cleanup+0x43/0x50 [bridge]
>      do_netpoll_cleanup+0x43/0xc0
>      netconsole_netdev_event+0x1e3/0x300 [netconsole]
>      unregister_netdevice_notifier+0xd9/0x150
>      cleanup_module+0x45/0x920 [netconsole]
>      __se_sys_delete_module+0x205/0x290
>      do_syscall_64+0x70/0x150
>      entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 
> Move the skb_pool list setup and initial skb fill into __netpoll_setup().
> 
> Fixes: 221a9c1df790 ("net: netpoll: Individualize the skb pool")
> Signed-off-by: John Sperbeck <jsperbeck@google.com>

Reviewed-by: Breno Leitao <leitao@debian.org>

You didn't specify the network tree you are sending this patch for.
Since this patch fixes a commit in 'net' tree (also on Linus' tree), you
want to send it against 'net'.

Something as:
	# git format-patch --subject-prefix='PATCH net'

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#indicating-target-tree

Thanks for the fix.
--breno

[PATCH net v3] net: netpoll: ensure skb_pool list is always initialized

[John Sperbeck]

When __netpoll_setup() is called directly, instead of through
netpoll_setup(), the np->skb_pool list head isn't initialized.
If skb_pool_flush() is later called, then we hit a NULL pointer
in skb_queue_purge_reason().  This can be seen with this repro,
when CONFIG_NETCONSOLE is enabled as a module:

    ip tuntap add mode tap tap0
    ip link add name br0 type bridge
    ip link set dev tap0 master br0
    modprobe netconsole netconsole=4444@10.0.0.1/br0,9353@10.0.0.2/
    rmmod netconsole

The backtrace is:

    BUG: kernel NULL pointer dereference, address: 0000000000000008
    #PF: supervisor write access in kernel mode
    #PF: error_code(0x0002) - not-present page
    ... ... ...
    Call Trace:
     <TASK>
     __netpoll_free+0xa5/0xf0
     br_netpoll_cleanup+0x43/0x50 [bridge]
     do_netpoll_cleanup+0x43/0xc0
     netconsole_netdev_event+0x1e3/0x300 [netconsole]
     unregister_netdevice_notifier+0xd9/0x150
     cleanup_module+0x45/0x920 [netconsole]
     __se_sys_delete_module+0x205/0x290
     do_syscall_64+0x70/0x150
     entry_SYSCALL_64_after_hwframe+0x76/0x7e

Move the skb_pool list setup and initial skb fill into __netpoll_setup().

Fixes: 221a9c1df790 ("net: netpoll: Individualize the skb pool")
Signed-off-by: John Sperbeck <jsperbeck@google.com>
---

Updated mail message subject to target the 'net' tree.

 net/core/netpoll.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 2e459b9d88eb..96a6ed37d4cc 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -627,6 +627,8 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 	const struct net_device_ops *ops;
 	int err;
 
+	skb_queue_head_init(&np->skb_pool);
+
 	if (ndev->priv_flags & IFF_DISABLE_NETPOLL) {
 		np_err(np, "%s doesn't support polling, aborting\n",
 		       ndev->name);
@@ -662,6 +664,9 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 	strscpy(np->dev_name, ndev->name, IFNAMSIZ);
 	npinfo->netpoll = np;
 
+	/* fill up the skb queue */
+	refill_skbs(np);
+
 	/* last thing to do is link it to the net device structure */
 	rcu_assign_pointer(ndev->npinfo, npinfo);
 
@@ -681,8 +686,6 @@ int netpoll_setup(struct netpoll *np)
 	struct in_device *in_dev;
 	int err;
 
-	skb_queue_head_init(&np->skb_pool);
-
 	rtnl_lock();
 	if (np->dev_name[0]) {
 		struct net *net = current->nsproxy->net_ns;
@@ -782,9 +785,6 @@ int netpoll_setup(struct netpoll *np)
 		}
 	}
 
-	/* fill up the skb queue */
-	refill_skbs(np);
-
 	err = __netpoll_setup(np, ndev);
 	if (err)
 		goto flush;
-- 
2.47.1.688.g23fc6f90ad-goog

Re: [PATCH net v3] net: netpoll: ensure skb_pool list is always initialized

[Breno Leitao]

On Mon, Jan 13, 2025 at 05:13:54PM -0800, John Sperbeck wrote:
> When __netpoll_setup() is called directly, instead of through
> netpoll_setup(), the np->skb_pool list head isn't initialized.
> If skb_pool_flush() is later called, then we hit a NULL pointer
> in skb_queue_purge_reason().  This can be seen with this repro,
> when CONFIG_NETCONSOLE is enabled as a module:
> 
>     ip tuntap add mode tap tap0
>     ip link add name br0 type bridge
>     ip link set dev tap0 master br0
>     modprobe netconsole netconsole=4444@10.0.0.1/br0,9353@10.0.0.2/
>     rmmod netconsole
> 
> The backtrace is:
> 
>     BUG: kernel NULL pointer dereference, address: 0000000000000008
>     #PF: supervisor write access in kernel mode
>     #PF: error_code(0x0002) - not-present page
>     ... ... ...
>     Call Trace:
>      <TASK>
>      __netpoll_free+0xa5/0xf0
>      br_netpoll_cleanup+0x43/0x50 [bridge]
>      do_netpoll_cleanup+0x43/0xc0
>      netconsole_netdev_event+0x1e3/0x300 [netconsole]
>      unregister_netdevice_notifier+0xd9/0x150
>      cleanup_module+0x45/0x920 [netconsole]
>      __se_sys_delete_module+0x205/0x290
>      do_syscall_64+0x70/0x150
>      entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 
> Move the skb_pool list setup and initial skb fill into __netpoll_setup().
> 
> Fixes: 221a9c1df790 ("net: netpoll: Individualize the skb pool")
> Signed-off-by: John Sperbeck <jsperbeck@google.com>

Reviewed-by: Breno Leitao <leitao@debian.org>

Re: [PATCH net v3] net: netpoll: ensure skb_pool list is always initialized

[patchwork-bot+netdevbpf]

Hello:

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

On Mon, 13 Jan 2025 17:13:54 -0800 you wrote:
> When __netpoll_setup() is called directly, instead of through
> netpoll_setup(), the np->skb_pool list head isn't initialized.
> If skb_pool_flush() is later called, then we hit a NULL pointer
> in skb_queue_purge_reason().  This can be seen with this repro,
> when CONFIG_NETCONSOLE is enabled as a module:
> 
>     ip tuntap add mode tap tap0
>     ip link add name br0 type bridge
>     ip link set dev tap0 master br0
>     modprobe netconsole netconsole=4444@10.0.0.1/br0,9353@10.0.0.2/
>     rmmod netconsole
> 
> [...]

Here is the summary with links:
  - [net,v3] net: netpoll: ensure skb_pool list is always initialized
    https://git.kernel.org/netdev/net/c/f0d0277796db

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