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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 (1) · 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 reject-path undo in skb_attempt_defer_free()
  (net/core/skbuff.c:7399) brings in a new absorbing state that leaves…

Pre-existing issues:
- [Medium] In skb_attempt_defer_free() the reject path jumps to nodefer
  without calling kick_defer_list_purge().
- [Low] Documentation/admin-guide/sysctl/net.rst (line 365) describes
  skb_defer_max as the "Max size (in skbs) of the per-cpu list".

--

net: do not let defer_count outlive an empty defer_list

When the skb_defer_max limit rejects an skb, skb_attempt_defer_free() now
undoes its defer_count increment, so the counter only counts skbs that are
actually queued. In __skb_defer_free_flush() the counter reset now comes
after llist_del_all(), which avoids a transient over-count.

> 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);

[ ... ]

> 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) {

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. The >=
test means at most defer_max - 1 skbs are queued. However,
Documentation/admin-guide/sysctl/net.rst describes skb_defer_max as:

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

With skb_defer_max=1, nothing is deferred at all. The list is also now per
(cpu, node), not per-cpu.

Should the documentation or the comparison be changed so they agree?

> +		/* 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);

[Severity: High]
Can this decrement push defer_count below zero if it races with the flush
on the owner CPU?

The increment and the undo are two separate atomic operations. The
atomic_long_set() in __skb_defer_free_flush() can run between them. With
defer_max = M and M-1 skbs queued:

P1, P2 (remote producers)            owner CPU
atomic_long_inc_return() -> M
atomic_long_inc_return() -> M+1
both take the reject branch
                                     llist_del_all()
                                     atomic_long_set(&sdn->defer_count, 0)
atomic_long_dec() -> -1
atomic_long_dec() -> -2

This leaves the counter at -2 with an empty list.

defer_count is an unsigned long, so the next producer's
atomic_long_inc_return() returns -1, which reads as ULONG_MAX. The
defer_count >= defer_max test is true, so that producer rejects and
decrements back to -2. Every later producer that runs alone does the same.

__skb_defer_free_flush() then returns early on the empty list and never
resets the counter:

    if (llist_empty(&sdn->defer_list))
        return;

dev_cpu_dead() and sysctl writes don't reset it either.

Wouldn't this switch off deferral for that (cpu, node) slot indefinitely,
even at the default skb_defer_max?

Several things can widen the inc-to-dec window: hardirqs, preemption in
process context (the TCP/UDP recvmsg paths), and softirq preemption on
PREEMPT_RT. Meanwhile the flush runs from net_rx_action, busy poll and
threaded NAPI on the owner CPU.

This seems to contradict the commit message:

    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.

The new comment in __skb_defer_free_flush() cites __dev_xmit_skb() as
precedent. That path increments q->defer_count and drops without ever
decrementing, so its reset can only under-count and can never go negative:

    defer_count = atomic_long_inc_return(&q->defer_count);
    if (unlikely(defer_count > READ_ONCE(net_hotdata.qdisc_max_burst))) {
        kfree_skb_reason(skb, SKB_DROP_REASON_QDISC_BURST_DROP);
        return NET_XMIT_DROP;

The new comment also only covers producers that get accepted. It does not
cover rejected producers whose increment is erased by the store before
their decrement runs.

Would an approach that never increments past the limit avoid this? Some
options:

- atomic_long_add_unless() or a cmpxchg loop
- a signed comparison, so a negative count heals
- having the flush subtract the number of drained skbs instead of storing 0

>  		goto nodefer;
> +	}

[Severity: Medium]
This isn't a bug introduced by this patch. The reject path still jumps to
nodefer without calling kick_defer_list_purge(). The kick further down in
skb_attempt_defer_free() only fires on exact equality:

    kick = (defer_count - 1) == (defer_max >> 1);

Can that threshold become unreachable, even with an accurate counter?

With skb_defer_max=2, the only accepted value is inc_return == 1, and
(1 - 1) == 1 is false. Every later producer increments to 2, rejects, and
decrements back to 1.

The same happens if the limit is lowered below the current count before
the old kick fired. For example, with 127 skbs queued under a limit of 256,
set the limit to 128. Every producer then goes 127 -> 128, rejects, and goes
back to 127. The new threshold of 65 has already been passed.

proc_do_skb_defer_max() only toggles skb_defer_disable_key on changes to or
from 0. It doesn't reset the counters or schedule a flush:

    if (nval != oval) {
        if (nval)
            static_branch_enable(&skb_defer_disable_key);
        else
            static_branch_disable(&skb_defer_disable_key);
    }

Consider an owner CPU that never runs NET_RX, such as the TX-only
application CPU from the commit message, whose skbs are completed with
napi_consume_skb() on another CPU. Wouldn't up to defer_max - 1 skbs stay
pinned on that list, with deferral off for that (cpu, node), for as long as
the sysctl keeps that value?

Before this patch, the counter also climbed past the threshold without
kicking. With this patch, it can recover once the limit is raised again.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013205416.3417296-1-jkain%40nuro.ai