Re: [PATCH net-next] net: bgmac: use the NAPI skb cache

[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

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] In bgmac_dma_rx_read() (drivers/net/ethernet/broadcom/bgmac.c),
  the patch replaces build_skb() with napi_build_skb().

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but bgmac_poll()
  has no zero-budget guard.

--

net: bgmac: use the NAPI skb cache

This commit changes bgmac to allocate RX skb heads with napi_build_skb()
and to free completed TX skbs with napi_consume_skb(). The skb heads are
then recycled through the per-CPU NAPI cache instead of the slab
allocator.

> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 3e9c57196a397..63a01accd7803 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c

[ ... ]

> @@ -466,9 +467,9 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
>  			/* Omit CRC. */
>  			len -= ETH_FCS_LEN;
>  
> -			skb = build_skb(buf, BGMAC_RX_ALLOC_SIZE);
> +			skb = napi_build_skb(buf, BGMAC_RX_ALLOC_SIZE);

[Severity: High]
Can netpoll's zero-budget poll reach this call outside softirq context?

poll_one_napi() in net/core/netpoll.c calls napi->poll(napi, 0). A budget
of 0 means only the TX path should be cleaned. The TX side of bgmac_poll()
handles this, because napi_consume_skb() falls back to
dev_consume_skb_any() when budget is 0.

However, bgmac_poll() still calls bgmac_dma_rx_read() with weight 0. The
budget check there only runs after a descriptor has been handled:

bgmac_dma_rx_read() {
    ...
		if (handled >= weight) /* Should never be greater */
			break;
    ...
}

So one ready RX frame always reaches napi_build_skb():

netpoll_send_skb() [local_irq_save]
  __netpoll_send_skb()
    netpoll_poll_dev()
      poll_one_napi()
        bgmac_poll(napi, 0)
          bgmac_dma_rx_read(weight=0)
            napi_build_skb()
              __napi_build_skb()
                napi_skb_cache_get()

napi_skb_cache_get() modifies the per-CPU napi_alloc_cache
(nc->skb_cache[--nc->skb_count]) under local_lock_nested_bh(). On !RT
kernels, that lock is only a lockdep_assert_in_softirq() plus a lockdep
annotation.

Netpoll can be entered from hardirq context, for example by a printk
through netconsole from an IRQ handler. Suppose that hardirq interrupts a
NET_RX softirq on the same CPU while the softirq is inside
napi_skb_cache_get() or napi_skb_cache_put().

Could both contexts then take the same skb head, or corrupt
nc->skb_count?

On PREEMPT_RT, local_lock_nested_bh() is a sleeping per-CPU lock. Here it
would be taken with IRQs disabled. With lockdep enabled,
lockdep_assert_in_softirq() would also warn every time netpoll handles an
RX frame.

The old build_skb() used kmem_cache_alloc(), which is safe in this
context. Zero-budget RX could already reach napi_skb_cache_put() through
the GRO_MERGED_FREE path in gro_skb_finish(). This change makes the cache
access unconditional for every frame handled at budget 0.

Would it make sense to skip RX processing when weight is 0, either in
bgmac_poll() or at the top of bgmac_dma_rx_read()?

[ ... ]

> @@ -1153,7 +1154,7 @@ static int bgmac_poll(struct napi_struct *napi, int weight)
>  	/* Ack */
>  	bgmac_write(bgmac, BGMAC_INT_STATUS, ~0);
>  
> -	bgmac_dma_tx_free(bgmac, &bgmac->tx_ring[0]);
> +	bgmac_dma_tx_free(bgmac, &bgmac->tx_ring[0], weight);
>  	handled += bgmac_dma_rx_read(bgmac, &bgmac->rx_ring[0], weight);

[Severity: Medium]
This isn't a bug introduced by this patch, but bgmac_poll() has no guard
for a zero budget. With weight 0, bgmac_dma_rx_read() handles one RX
descriptor and returns 1. bgmac_poll() then skips the handled < weight
branch and also returns 1.

Doesn't that trigger the WARN_ONCE() in poll_one_napi()?

net/core/netpoll.c:poll_one_napi() {
    ...
	work = napi->poll(napi, 0);
	WARN_ONCE(work, "%pS exceeded budget in poll\n", napi->poll);
    ...
}

This missing guard is also what allows the napi_build_skb() issue above.
The same fix, returning before RX processing when weight is 0, would
cover both.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010070635.97759-1-rosenp%40gmail.com