[PATCH net] net: flush skb_defer_nodes in dev_cpu_dead()

[PATCH net] net: flush skb_defer_nodes in dev_cpu_dead()

[Eric Dumazet]

When a CPU goes offline, dev_cpu_dead() drains its softnet queues
(completion_queue, output_queue, poll_list, process_queue, and
input_pkt_queue), but leaves net_hotdata.skb_defer_nodes untouched.

If oldcpu goes offline while holding pending skbs in its
skb_defer_nodes lists (e.g. below the sysctl_skb_defer_max >> 1 IPI
threshold, or if the IPI races with CPU teardown), those skbs remain
stranded until oldcpu is brought back online. If any of these skbs
hold page_pool fragments, page_pool_destroy() will stall indefinitely
waiting for inflight pages to be returned when a netdev or driver is
torn down while oldcpu is offline.

Additionally, if smp_call_function_single_async() fails in
kick_defer_list_purge() because the target CPU went offline, reset
defer_ipi_scheduled to 0 so future IPI kicks are not blocked when the
CPU comes back online.

Also, if oldcpu was the last online CPU on its NUMA node, drain that
node's slot across all CPUs so no skbs deferred from that node remain
stranded on idle remote CPUs (or if the node itself is subsequently
offlined).

Finally, in skb_attempt_defer_free(), re-check cpu_online(cpu) and
whether the caller migrated CPUs after llist_add(), flushing the node
list if so, to close the preemption TOCTOU race against CPU/node
teardown.

Fixes: 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu lists")
Fixes: 5628f3fe3b16 ("net: add NUMA awareness to skb_attempt_defer_free()")
Closes: https://lore.kernel.org/netdev/20260916003430.3612956-1-kris.pan@intel.com/
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Kris Pan <kris.pan@intel.com>
---
 net/core/dev.c    | 48 +++++++++++++++++++++++++++++++++++------------
 net/core/dev.h    |  2 ++
 net/core/skbuff.c | 12 +++++++++---
 3 files changed, 47 insertions(+), 15 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index c67900354fa64b73de0623112366f485fedeb87b..c4151393cc819617fcf0d6b5abba09d4f6946ce6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5376,7 +5376,8 @@ void kick_defer_list_purge(unsigned int cpu)
 		backlog_unlock_irq_restore(sd, flags);
 
 	} else if (!cmpxchg(&sd->defer_ipi_scheduled, 0, 1)) {
-		smp_call_function_single_async(cpu, &sd->defer_csd);
+		if (smp_call_function_single_async(cpu, &sd->defer_csd))
+			WRITE_ONCE(sd->defer_ipi_scheduled, 0);
 	}
 }
 
@@ -6900,25 +6901,35 @@ bool napi_complete_done(struct napi_struct *n, int work_done)
 }
 EXPORT_SYMBOL(napi_complete_done);
 
-static void skb_defer_free_flush(void)
+static void __skb_defer_free_flush(struct skb_defer_node *sdn, int budget)
 {
 	struct llist_node *free_list;
 	struct sk_buff *skb, *next;
+
+	if (llist_empty(&sdn->defer_list))
+		return;
+	atomic_long_set(&sdn->defer_count, 0);
+	free_list = llist_del_all(&sdn->defer_list);
+
+	llist_for_each_entry_safe(skb, next, free_list, ll_node) {
+		prefetch(next);
+		napi_consume_skb(skb, budget);
+	}
+}
+
+void skb_defer_node_flush(struct skb_defer_node *sdn)
+{
+	__skb_defer_free_flush(sdn, 0);
+}
+
+static void skb_defer_free_flush(void)
+{
 	struct skb_defer_node *sdn;
 	int node;
 
 	for_each_node(node) {
 		sdn = this_cpu_ptr(net_hotdata.skb_defer_nodes) + node;
-
-		if (llist_empty(&sdn->defer_list))
-			continue;
-		atomic_long_set(&sdn->defer_count, 0);
-		free_list = llist_del_all(&sdn->defer_list);
-
-		llist_for_each_entry_safe(skb, next, free_list, ll_node) {
-			prefetch(next);
-			napi_consume_skb(skb, 1);
-		}
+		__skb_defer_free_flush(sdn, 1);
 	}
 }
 
@@ -12897,6 +12908,7 @@ static int dev_cpu_dead(unsigned int oldcpu)
 	struct sk_buff **list_skb;
 	struct sk_buff *skb;
 	unsigned int cpu;
+	int node;
 	struct softnet_data *sd, *oldsd, *remsd = NULL;
 
 	local_irq_disable();
@@ -12957,6 +12969,18 @@ static int dev_cpu_dead(unsigned int oldcpu)
 		rps_input_queue_head_incr(oldsd);
 	}
 
+	WRITE_ONCE(oldsd->defer_ipi_scheduled, 0);
+	for_each_node(node)
+		skb_defer_node_flush(per_cpu_ptr(net_hotdata.skb_defer_nodes,
+						 oldcpu) + node);
+	node = cpu_to_node(oldcpu);
+	if (node_possible(node) &&
+	    !cpumask_intersects(cpumask_of_node(node), cpu_online_mask)) {
+		for_each_possible_cpu(cpu)
+			skb_defer_node_flush(per_cpu_ptr(net_hotdata.skb_defer_nodes,
+							 cpu) + node);
+	}
+
 	return 0;
 }
 
diff --git a/net/core/dev.h b/net/core/dev.h
index b757faead4d1a3e445e54d2f468c38c9e09b6762..04fb0e9a571e03f41cac110a0d1dd864d110b339 100644
--- a/net/core/dev.h
+++ b/net/core/dev.h
@@ -399,6 +399,8 @@ static inline void napi_assert_will_not_race(const struct napi_struct *napi)
 	WARN_ON(READ_ONCE(napi->list_owner) != -1);
 }
 
+struct skb_defer_node;
+void skb_defer_node_flush(struct skb_defer_node *sdn);
 void kick_defer_list_purge(unsigned int cpu);
 
 int dev_set_hwtstamp_phylib(struct net_device *dev,
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index b4edbd06655e2ac46fdccc74979295f4a938078f..c3042d822afa7aaaae7cad6aba4d647af0732aeb 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -7360,8 +7360,8 @@ void skb_attempt_defer_free(struct sk_buff *skb)
 	struct skb_defer_node *sdn;
 	unsigned long defer_count;
 	unsigned int defer_max;
+	int cpu, my_cpu;
 	bool kick;
-	int cpu;
 
 	if (static_branch_unlikely(&skb_defer_disable_key))
 		goto nodefer;
@@ -7371,7 +7371,8 @@ void skb_attempt_defer_free(struct sk_buff *skb)
 		goto nodefer;
 
 	cpu = skb->alloc_cpu;
-	if (cpu == raw_smp_processor_id() ||
+	my_cpu = raw_smp_processor_id();
+	if (cpu == my_cpu ||
 	    WARN_ON_ONCE(cpu >= nr_cpu_ids) ||
 	    !cpu_online(cpu)) {
 nodefer:	kfree_skb_napi_cache(skb);
@@ -7382,7 +7383,7 @@ nodefer:	kfree_skb_napi_cache(skb);
 	DEBUG_NET_WARN_ON_ONCE(skb->destructor);
 	DEBUG_NET_WARN_ON_ONCE(skb_nfct(skb));
 
-	sdn = per_cpu_ptr(net_hotdata.skb_defer_nodes, cpu) + numa_node_id();
+	sdn = per_cpu_ptr(net_hotdata.skb_defer_nodes, cpu) + cpu_to_node(my_cpu);
 
 	defer_max = READ_ONCE(net_hotdata.sysctl_skb_defer_max);
 	defer_count = atomic_long_inc_return(&sdn->defer_count);
@@ -7392,6 +7393,11 @@ nodefer:	kfree_skb_napi_cache(skb);
 
 	llist_add(&skb->ll_node, &sdn->defer_list);
 
+	if (unlikely(!cpu_online(cpu) || my_cpu != raw_smp_processor_id())) {
+		skb_defer_node_flush(sdn);
+		return;
+	}
+
 	/* Send an IPI every time queue reaches half capacity. */
 	kick = (defer_count - 1) == (defer_max >> 1);
 
-- 
2.55.0.1082.g2b9226bbc0-goog

Re: [PATCH net] net: flush skb_defer_nodes in dev_cpu_dead()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 (3) · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new WRITE_ONCE(oldsd->defer_ipi_scheduled, 0) in
  dev_cpu_dead() revokes CSD ownership that another task may still hold,…

Pre-existing issues:
- [Medium] Residual page_pool_destroy() stall with all CPUs online: skbs
  sitting in the skb_defer_node list of an *online* CPU below the…
- [Medium] __skb_defer_free_flush() returns early when the llist is empty
  and therefore never executes atomic_long_set(&sdn->defer_count, 0) -…
- [Medium] dev_cpu_dead() is extended to drain
  net_hotdata.skb_defer_nodes, but the neighbouring per-CPU skb queue…
- [Low] Documentation/admin-guide/sysctl/net.rst describes skb_defer_max
  as the 'Max size (in skbs) of the per-cpu list of skbs being freed by…

--

net: flush skb_defer_nodes in dev_cpu_dead()

This adds a drain of net_hotdata.skb_defer_nodes to dev_cpu_dead() so skbs
deferred to a CPU that goes offline are not stranded, extends that drain to
every CPU's slot for a NUMA node whose last CPU just went away, and resets
defer_ipi_scheduled when smp_call_function_single_async() fails.  It also
re-checks cpu_online(cpu) and the current CPU after llist_add() in
skb_attempt_defer_free().

Closes: https://lore.kernel.org/netdev/20260916003430.3612956-1-kris.pan@intel.com/

> diff --git a/net/core/dev.c b/net/core/dev.c
> index c67900354fa64..c4151393cc819 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5376,7 +5376,8 @@ void kick_defer_list_purge(unsigned int cpu)
>  		backlog_unlock_irq_restore(sd, flags);
>  
>  	} else if (!cmpxchg(&sd->defer_ipi_scheduled, 0, 1)) {
> -		smp_call_function_single_async(cpu, &sd->defer_csd);
> +		if (smp_call_function_single_async(cpu, &sd->defer_csd))
> +			WRITE_ONCE(sd->defer_ipi_scheduled, 0);
>  	}
>  }
>  
> @@ -6900,25 +6901,35 @@ bool napi_complete_done(struct napi_struct *n, int work_done)
>  }
>  EXPORT_SYMBOL(napi_complete_done);
>  
> -static void skb_defer_free_flush(void)
> +static void __skb_defer_free_flush(struct skb_defer_node *sdn, int budget)
>  {
>  	struct llist_node *free_list;
>  	struct sk_buff *skb, *next;
> +
> +	if (llist_empty(&sdn->defer_list))
> +		return;
> +	atomic_long_set(&sdn->defer_count, 0);
> +	free_list = llist_del_all(&sdn->defer_list);

[Severity: Medium]
This isn't a bug introduced by this patch, but can the early return on an
empty list latch defer_count permanently above the limit?

The atomic_long_set(&sdn->defer_count, 0) here is the only reset of that
counter in the tree, and it is skipped when the list is empty.
skb_attempt_defer_free() increments before it tests, and does not decrement
on the bail-out path:

net/core/skbuff.c:skb_attempt_defer_free() {
	defer_count = atomic_long_inc_return(&sdn->defer_count);

	if (defer_count >= defer_max)
		goto nodefer;
}

Two ways to reach counter >= sysctl_skb_defer_max with an empty list:

 - an admin sets net.core.skb_defer_max to 1 (the sysctl has only
   SYSCTL_ZERO as a lower bound, and only the value 0 flips
   skb_defer_disable_key) and then restores 128, leaving the counter
   already above the restored limit;

 - a flusher delayed between atomic_long_set(0) and llist_del_all() lets
   producers push the counter up to defer_max while the following
   llist_del_all() captures all their skbs.

>From then on every deferral for that (cpu, node) slot increments and takes
the nodefer path, the list stays empty, and every flush - including the new
skb_defer_node_flush() calls in dev_cpu_dead() - returns immediately, so
the slot never recovers.  Would resetting defer_count unconditionally
before the llist_empty() check address that?

>  
> -		if (llist_empty(&sdn->defer_list))
> -			continue;
> -		atomic_long_set(&sdn->defer_count, 0);
> -		free_list = llist_del_all(&sdn->defer_list);
> -
> -		llist_for_each_entry_safe(skb, next, free_list, ll_node) {
> -			prefetch(next);
> -			napi_consume_skb(skb, 1);
> -		}
> +		__skb_defer_free_flush(sdn, 1);
>  	}
>  }
>  

[ ... ]

> @@ -12957,6 +12969,18 @@ static int dev_cpu_dead(unsigned int oldcpu)
>  		rps_input_queue_head_incr(oldsd);
>  	}
>  
> +	WRITE_ONCE(oldsd->defer_ipi_scheduled, 0);

[Severity: High]
Can this store revoke CSD ownership that another task still holds?

defer_ipi_scheduled acts as the ownership token for &sd->defer_csd: the
winner of the cmpxchg in kick_defer_list_purge() is the only task allowed
to submit that csd, which is what satisfies the serialization requirement
documented above smp_call_function_single_async() in kernel/smp.c.

kick_defer_list_purge() is reachable from preemptible process context:

tcp_recvmsg()
  tcp_eat_recv_skb()
    skb_attempt_defer_free()
      kick_defer_list_purge()

so a task can win the cmpxchg and be preempted before it ever enters
smp_call_function_single_async(), which only disables preemption
internally.  If oldcpu is offlined in that window, this WRITE_ONCE() clears
the token while that task still owns it.  After the CPU comes back online a
second task can win the cmpxchg, and both can then be inside
smp_call_function_single_async() for the same csd:

kernel/smp.c:smp_call_function_single_async() {
	if (csd->node.u_flags & CSD_FLAG_LOCK) {
		err = -EBUSY;
		goto out;
	}

	csd->node.u_flags = CSD_FLAG_LOCK;
}

That test-then-set is not atomic across CPUs, so both can pass it and both
call __smp_call_single_queue(), doing llist_add() twice with the same node.
The second adder can observe head->first == node and store
node->next = node, which is a self-loop in the target CPU's
call_single_queue and makes the traversal in
__flush_smp_call_function_queue() spin in hardirq context.

Is the store needed at all?  The new error handling added to
kick_defer_list_purge() in this patch already has the owner release the
token when submission is refused, and a csd that was successfully queued is
unlocked and executed by smpcfd_dying_cpu() at CPUHP_AP_SMPCFD_DYING, which
runs before CPUHP_NET_DEV_DEAD.  Would dropping this line be the simpler
fix?

> +	for_each_node(node)
> +		skb_defer_node_flush(per_cpu_ptr(net_hotdata.skb_defer_nodes,
> +						 oldcpu) + node);

[Severity: Medium]
This is a pre-existing gap rather than something this patch introduces, but
while dev_cpu_dead() is being extended to release per-CPU skb references,
should oldsd->xfrm_backlog be drained here too?

With CONFIG_XFRM_OFFLOAD the producer is strictly local:

net/xfrm/xfrm_device.c:xfrm_dev_resume() {
		sd = this_cpu_ptr(&softnet_data);
		skb_queue_tail(&sd->xfrm_backlog, skb);
		raise_softirq_irqoff(NET_TX_SOFTIRQ);
}

and the only consumer is xfrm_dev_backlog(sd) called from net_tx_action()
with sd = this_cpu_ptr(&softnet_data), so anything left on
oldsd->xfrm_backlog when the CPU dies stays pinned until that CPU is
brought back online, which is the same stranded-skb and page_pool-inflight
condition this patch targets.

The commit message says dev_cpu_dead() "drains its softnet queues
(completion_queue, output_queue, poll_list, process_queue, and
input_pkt_queue)" - is that list complete given xfrm_backlog is also a
softnet_data queue that is never drained there?

> +	node = cpu_to_node(oldcpu);
> +	if (node_possible(node) &&
> +	    !cpumask_intersects(cpumask_of_node(node), cpu_online_mask)) {
> +		for_each_possible_cpu(cpu)
> +			skb_defer_node_flush(per_cpu_ptr(net_hotdata.skb_defer_nodes,
> +							 cpu) + node);
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 609f2c7f4a47a..14a9ee1344890 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c

[ ... ]

> @@ -7379,7 +7380,7 @@ nodefer:	kfree_skb_napi_cache(skb);
>  	DEBUG_NET_WARN_ON_ONCE(skb->destructor);
>  	DEBUG_NET_WARN_ON_ONCE(skb_nfct(skb));
>  
> -	sdn = per_cpu_ptr(net_hotdata.skb_defer_nodes, cpu) + numa_node_id();
> +	sdn = per_cpu_ptr(net_hotdata.skb_defer_nodes, cpu) + cpu_to_node(my_cpu);

[Severity: Low]
This isn't a bug introduced here, but the new per-node flush loops make it
visible that the skb_defer_max documentation no longer matches the layout.

Documentation/admin-guide/sysctl/net.rst still describes it as:

	Max size (in skbs) of the per-cpu list of skbs being freed
	by the cpu which allocated them.

	Default: 128

net_dev_init() allocates nr_node_ids slots per CPU:

net/core/dev.c:net_dev_init() {
	net_hotdata.skb_defer_nodes =
		 __alloc_percpu(sizeof(struct skb_defer_node) * nr_node_ids,
}

and the limit is compared against the counter of the single node-indexed
slot selected above, so a target CPU can hold roughly
nr_node_ids * skb_defer_max skbs.  Should the documentation be updated to
say the limit is per (cpu, node) list?

>  
>  	defer_max = READ_ONCE(net_hotdata.sysctl_skb_defer_max);
>  	defer_count = atomic_long_inc_return(&sdn->defer_count);
> @@ -7389,6 +7390,11 @@ nodefer:	kfree_skb_napi_cache(skb);
>  
>  	llist_add(&skb->ll_node, &sdn->defer_list);
>  
> +	if (unlikely(!cpu_online(cpu) || my_cpu != raw_smp_processor_id())) {
> +		skb_defer_node_flush(sdn);
> +		return;
> +	}
> +
>  	/* Send an IPI every time queue reaches half capacity. */
>  	kick = (defer_count - 1) == (defer_max >> 1);

[Severity: Medium]
This is a pre-existing property of the deferral design rather than
something this patch adds, but does it also leave a page_pool_destroy()
stall reachable with all CPUs online?

This equality test is the only producer of an IPI kick, and
kick_defer_list_purge() has just this one caller.  Below the
(defer_max >> 1) edge nothing asks the owning CPU to flush, and the only
periodic drain is skb_defer_free_flush(), which runs solely on the owning
CPU from net_rx_action(), napi_threaded_poll() and the busy-poll path.

So if CPU A consumes a handful of skbs allocated by CPU B, no IPI is sent,
and CPU B then stays idle (no further NET_RX_SOFTIRQ), those skbs and any
page_pool fragments they hold are pinned for as long as B stays idle.  If
the NIC owning the pool is removed in that window,
page_pool_release_retry() keeps requeueing itself:

net/core/page_pool.c:page_pool_release_retry() {
	schedule_delayed_work(&pool->release_dw, DEFER_TIME);
}

with a warning every DEFER_WARN_INTERVAL and the pool never freed.  The new
dev_cpu_dead() drain covers the offline case described in the commit
message; is anything planned for the idle-but-online alloc_cpu case, for
instance kicking the defer lists from the page_pool teardown path?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921182700.1796214-1-edumazet%40google.com

Re: [PATCH net] net: flush skb_defer_nodes in dev_cpu_dead()

[Eric Dumazet]

On Wed, Sep 23, 2026 at 11:28 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 0 (3) · Low: 0 (1)
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] The new WRITE_ONCE(oldsd->defer_ipi_scheduled, 0) in
>   dev_cpu_dead() revokes CSD ownership that another task may still hold,…
>

Right, I will fix this issue, and leave pre-existing issues for followups.

> Pre-existing issues:
> - [Medium] Residual page_pool_destroy() stall with all CPUs online: skbs
>   sitting in the skb_defer_node list of an *online* CPU below the…
> - [Medium] __skb_defer_free_flush() returns early when the llist is empty
>   and therefore never executes atomic_long_set(&sdn->defer_count, 0) -…
> - [Medium] dev_cpu_dead() is extended to drain
>   net_hotdata.skb_defer_nodes, but the neighbouring per-CPU skb queue…
> - [Low] Documentation/admin-guide/sysctl/net.rst describes skb_defer_max
>   as the 'Max size (in skbs) of the per-cpu list of skbs being freed by…

pw-bot: cr