Re: [PATCH net v3 3/4] net: mvpp2: refill RX buffers before XDP or skb use

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incomplete DMA sync when recycling an XDP-modified page during SKB allocation failure.

Pre-existing issues:
- [High] Variable swap leading to invalid memory access in mvpp2_buff_hdr_pool_put().
- [High] Acquiring a spinlock with preemption disabled causes a sleep-in-atomic bug on PREEMPT_RT.
--

commit 9aeeec1a49aec085a29bfd1f85e72ffb42da9a5c
Author: Til Kaiser <mail@tk154.de>

net: mvpp2: refill RX buffers before XDP or skb use

This patch adjusts the RX error path to prevent returning a buffer to the
hardware BM pool if it has already been handed to XDP or attached to an skb.
It refills the BM pool earlier so that if allocation fails, the original
still-owned buffer can safely be returned to the hardware pool.

> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 3372ed27cc8d6..481daafdc1cbc 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c

[ ... ]

> @@ -4008,8 +4008,21 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
>  			skb = slab_build_skb(data);
>  		if (!skb) {
>  			netdev_warn(port->dev, "skb build failed\n");
> -			goto err_drop_frame;
> +			if (pp) {
> +				page_pool_put_page(pp, virt_to_head_page(data),
> +						   rx_bytes + MVPP2_MH_SIZE,
> +						   true);

[Severity: High]
Does this truncate the DMA sync size if the packet length was modified by XDP?

Earlier in mvpp2_rx(), an XDP program might have been executed which could
reduce the packet length using helpers like bpf_xdp_adjust_tail, updating
rx_bytes to a smaller value.

If build_skb() subsequently fails here, we use this reduced rx_bytes for the
page pool DMA sync. Because the sync does not cover the tail of the buffer
that the CPU may have fetched into its cache, can this lead to memory
corruption when the hardware DMA writes a new packet and the dirty CPU cache
lines are evicted over it?

In mvpp2_run_xdp(), this appears to be handled properly by calculating
max(sync, len).

> +			} else {
> +				dma_unmap_single_attrs(dev->dev.parent, dma_addr,
> +						       bm_pool->buf_size,
> +						       DMA_FROM_DEVICE,
> +						       DMA_ATTR_SKIP_CPU_SYNC);
> +				mvpp2_frag_free(bm_pool, pp, data);
> +			}
> +			goto err_drop_frame_retired;
>  		}

[ ... ]

> @@ -4048,13 +4052,14 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
>  		continue;
>  
>  err_drop_frame:
> -		dev->stats.rx_errors++;
> -		mvpp2_rx_error(port, rx_desc);
>  		/* Return the buffer to the pool */
>  		if (rx_status & MVPP2_RXD_BUF_HDR)
>  			mvpp2_buff_hdr_pool_put(port, rx_desc, pool, rx_status);

[Severity: High]
This is a pre-existing issue, but does mvpp2_buff_hdr_pool_put() swap the
physical and DMA addresses?

Looking at mvpp2_buff_hdr_pool_put():

static void mvpp2_buff_hdr_pool_put(...)
{
        ...
        phys_addr = mvpp2_rxdesc_dma_addr_get(port, rx_desc);
        dma_addr = mvpp2_rxdesc_cookie_get(port, rx_desc);

        do {
                buff_hdr = (struct mvpp2_buff_hdr *)phys_to_virt(phys_addr);

It appears phys_addr is initialized with the DMA address, and dma_addr with
the physical address. Because of this swap, we call phys_to_virt() on a DMA
address. On systems where the DMA address and physical address differ (like
when an IOMMU is active), could this cause an invalid memory access?

>  		else
>  			mvpp2_bm_pool_put(port, pool, dma_addr, phys_addr);

[Severity: High]
This is also a pre-existing issue, but does mvpp2_bm_pool_put() acquire a
spinlock while preemption is disabled?

Looking at mvpp2_bm_pool_put():

static inline void mvpp2_bm_pool_put(...)
{
        unsigned int thread = mvpp2_cpu_to_thread(port->priv, get_cpu());
        unsigned long flags = 0;

        if (test_bit(thread, &port->priv->lock_map))
                spin_lock_irqsave(&port->bm_lock[thread], flags);

Since get_cpu() executes preempt_disable(), we conditionally attempt to
acquire the bm_lock spinlock immediately after. On PREEMPT_RT kernels, where
standard spinlocks are mapped to sleepable rt_mutexes, won't this cause a
sleep-in-atomic bug when this code path is hit?

> +err_drop_frame_retired:
> +		dev->stats.rx_errors++;
> +		mvpp2_rx_error(port, rx_desc);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605121332.147397-1-mail@tk154.de?part=3