[PATCH net v3] net: do not let defer_count outlive an empty defer_list

[PATCH net v3] net: do not let defer_count outlive an empty defer_list

[Joseph Kain]

__skb_defer_free_flush() can leave defer_count at or above skb_defer_max
with an empty defer_list. That state is absorbing: every producer then
takes the nodefer path without queueing, so the list never refills.
Deferred freeing stays off for that (cpu, node) until reboot, and
cross-CPU frees fall back to the page allocator, which is the contention
deferral exists to avoid.

skb_attempt_defer_free() increments before testing the limit and does not
undo the increment when it rejects, so a rejected free leaves the counter
raised with nothing queued.

Nothing brings it back down. The reject path returns before
kick_defer_list_purge(), so once the counter is stuck no producer
schedules a flush on the owner CPU. A CPU that never raises NET_RX itself
-- an application CPU doing TX while NIC IRQs are pinned elsewhere and
RPS is off -- then never runs the flush at all, so clearing the counter
inside the flush cannot help it either. proc_do_skb_defer_max() neither
resets the counters nor schedules a flush when the limit changes.

It is reachable with no race at all:

  sysctl -w net.core.skb_defer_max=1    # frees increment, then reject
  sysctl -w net.core.skb_defer_max=128  # does not recover

skb_defer_max has .extra1 = SYSCTL_ZERO and no .extra2, so 1 is settable.
From v7.1 on, 0 is safe because proc_do_skb_defer_max() flips
skb_defer_disable_key and the producer returns before incrementing. On
6.18 through 7.0, which carry the llist conversion but not commit
08dc30de1a40 ("net: add skb_defer_disable_key static key"), setting 0 and
restoring the previous value strands the counter the same way -- and 0 is
the documented way to turn the feature off.

Undo the increment when the limit test rejects. The counter then only
accounts for skbs that are actually queued, so it cannot outlive them,
and the absorbing state is unreachable without depending on a flush ever
running on the owner CPU.

While here, drain before clearing in the flush. A producer that queues
between the clear and the drain currently has its skb drained but its
increment kept, which over-counts and costs deferral until the next
flush. Draining first matches the ordering __dev_xmit_skb() already uses
for the qdisc defer_list.

Found by manual code inspection, LLM-assisted, while backporting the
llist conversion to 6.1 after hitting the sd->defer_lock contention it
removes. The latch itself was not hit in production; it was reproduced by
lowering net.core.skb_defer_max.

Fixes: 844c9db7f7f5 ("net: use llist for sd->defer_list")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Joseph Kain <jkain@nuro.ai>
---
v3:
 - Undo the increment on the reject path. This is now the actual fix:
   the reject path returns before kick_defer_list_purge(), so a CPU that
   never raises NET_RX itself never runs the flush, and clearing the
   counter inside the flush cannot reach it (netdev-bot sashiko).
 - Dropped the empty-list clear from v2. With the increment undone the
   counter cannot outlive the skbs it counts, so the clear could no
   longer fire usefully and sat on the common path of the flush.
 - Added the comment on the deliberate clear/drain race, mirroring the
   one in __dev_xmit_skb() (netdev-bot sashiko).
 - Reworded accordingly: the reorder is now an accuracy improvement that
   avoids a transient over-count, not the thing preventing the latch.
 - Moved the discovery provenance into the commit message (netdev-bot
   sinfo).

Built for x86_64 and arm64, with and without CONFIG_PREEMPT_RT. On
v7.3-rc6 with skb_defer_max=1 and a cross-CPU TCP load, defer_count
reached 1918118 unpatched and did not recover when the default was
restored; with this patch it stayed at 0.

v2: https://lore.kernel.org/all/20261008220256.1844072-1-jkain@nuro.ai/
v1: https://lore.kernel.org/all/20261007180056.1394868-1-jkain@nuro.ai/
 net/core/dev.c    | 5 ++++-
 net/core/skbuff.c | 7 ++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 18dc88990510..60d86bc21632 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6940,8 +6940,11 @@ static void __skb_defer_free_flush(struct skb_defer_node *sdn, int budget)
 
 	if (llist_empty(&sdn->defer_list))
 		return;
-	atomic_long_set(&sdn->defer_count, 0);
 	free_list = llist_del_all(&sdn->defer_list);
+	/* Racy on purpose, as in __dev_xmit_skb(): a producer landing between
+	 * the del_all and this store stays queued but uncounted.
+	 */
+	atomic_long_set(&sdn->defer_count, 0);
 
 	llist_for_each_entry_safe(skb, next, free_list, ll_node) {
 		prefetch(next);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 4aea06d5167d..6e0872569ccf 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -7388,8 +7388,13 @@ nodefer:	kfree_skb_napi_cache(skb);
 	defer_max = READ_ONCE(net_hotdata.sysctl_skb_defer_max);
 	defer_count = atomic_long_inc_return(&sdn->defer_count);
 
-	if (defer_count >= defer_max)
+	if (defer_count >= defer_max) {
+		/* Undo, or the count outlives the skbs it was counting: the
+		 * reject path never reaches kick_defer_list_purge() below.
+		 */
+		atomic_long_dec(&sdn->defer_count);
 		goto nodefer;
+	}
 
 	llist_add(&skb->ll_node, &sdn->defer_list);
 
-- 
2.55.0

Re: [PATCH net v3] net: do not let defer_count outlive an empty defer_list

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: skb_attempt_defer_free() counter underflow permanently disables deferral
--

commit df0f58555dfa5a46b25a9ac548d9384905fe6da3
Author: Joseph Kain <jkain@nuro.ai>

net: do not let defer_count outlive an empty defer_list

This commit addresses an issue where the skb deferral counter could outlive
the items in the defer_list. It undoes the increment on the reject path
and reorders the clear and drain operations during a flush to prevent
transient over-counts.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index e76762e29360e..a5912b728ddd7 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c

[ ... ]

> @@ -6940,8 +6940,11 @@ static void __skb_defer_free_flush(struct skb_defer_node *sdn, int budget)
>  
>  	if (llist_empty(&sdn->defer_list))
>  		return;
> -	atomic_long_set(&sdn->defer_count, 0);
>  	free_list = llist_del_all(&sdn->defer_list);
> +	/* Racy on purpose, as in __dev_xmit_skb(): a producer landing between
> +	 * the del_all and this store stays queued but uncounted.
> +	 */
> +	atomic_long_set(&sdn->defer_count, 0);

[Severity: High]
Could this unconditional zeroing wipe out the increments of producers that 
are currently delayed on the reject path?

>  
>  	llist_for_each_entry_safe(skb, next, free_list, ll_node) {
>  		prefetch(next);

[ ... ]

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 41beaf625421a..e45e15506283b 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c

[ ... ]

> @@ -7392,8 +7392,13 @@ nodefer:	kfree_skb_napi_cache(skb);
>  	defer_max = READ_ONCE(net_hotdata.sysctl_skb_defer_max);
>  	defer_count = atomic_long_inc_return(&sdn->defer_count);
>  
> -	if (defer_count >= defer_max)
> +	if (defer_count >= defer_max) {
> +		/* Undo, or the count outlives the skbs it was counting: the
> +		 * reject path never reaches kick_defer_list_purge() below.
> +		 */
> +		atomic_long_dec(&sdn->defer_count);
>  		goto nodefer;
> +	}

[Severity: High]
Can this decrement race with the flush and permanently disable deferral due 
to a counter underflow?

If multiple producers concurrently increment the counter past the maximum, 
they will all prepare to decrement. If a flush operation then runs on the 
target CPU, empties the list, and unconditionally sets the counter to zero 
in __skb_defer_free_flush(), the delayed producers will then execute this 
decrement.

Because the counter is an unsigned long, this drops the value below zero 
and wraps it to a very large number. The next time a producer attempts to 
defer an SKB, the incremented count will still be above the maximum, so it 
will be rejected without queuing. Since no SKBs are ever queued again, the 
empty list check in __skb_defer_free_flush() remains true, causing future 
flushes to return early and never recover the wrapped counter.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013205416.3417296-1-jkain@nuro.ai?part=1