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@xxxxxxx>

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@xxxxxxx?part=1