Re: [PATCH v4 net 7/8] net: enetc: fix leak of pending XDP_TX frames when going down

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · 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] The new XDP_TX branch in enetc_free_tx_frame()
  (drivers/net/ethernet/freescale/enetc/enetc.c:171-178) decrements…

--

net: enetc: fix leak of pending XDP_TX frames when going down

This patch changes enetc_free_tx_frame() so that XDP_TX buffers still in
the TX ring at teardown are handled. It decrements the RX ring's
xdp_tx_in_flight counter and frees the page, so the RX ring is no longer
left waiting for buffers that will never come back.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index d456b9b488acf..218065b9a3777 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -168,6 +168,14 @@ static void enetc_free_tx_frame(struct enetc_bdr *tx_ring,
>  
>  	if (tx_swbd->dma)
>  		enetc_unmap_tx_buff(tx_ring, tx_swbd);
> +	if (tx_swbd->is_xdp_tx) {
> +		struct enetc_ndev_priv *priv = netdev_priv(tx_ring->ndev);
> +		struct enetc_bdr *rx_ring;
> +
> +		rx_ring = enetc_rx_ring_from_xdp_tx_ring(priv, tx_ring);
> +		rx_ring->xdp.xdp_tx_in_flight--;
> +		__free_page(tx_swbd->page);
> +	}

[Severity: High]
Should tx_swbd->is_xdp_tx and tx_swbd->page be cleared here, or the whole
swbd memset, after the page is freed?

As written, the swbd still has is_xdp_tx set and still points at the page
that was just freed. enetc_unmap_tx_buff() clears only ->dma, and the code
after this block clears only ->xdp_frame or ->skb. Before this patch,
calling enetc_free_tx_frame() twice on the same swbd did no harm. With
this patch, the second call frees the same page again.

The other release paths do scrub their descriptors. enetc_clean_tx_ring()
does this after enetc_recycle_xdp_tx_buff():

    memset(tx_swbd, 0, sizeof(*tx_swbd));

and enetc_free_rx_ring() sets rx_swbd->page = NULL.

The tx_swbd arrays can be reused after enetc_free_tx_ring() on the error
path of enetc_reconfigure():

    enetc_stop(priv->ndev);
    enetc_free_rxtx_rings(priv);

    /* Interface is down, run optional callback now */
    if (cb) {
        err = cb(priv, ctx);
        if (err)
            goto out_restart;
    }
    [ ... ]
out_restart:
    enetc_setup_bdrs(priv, extended);
    enetc_start(priv->ndev);

Here is one way to reach that path. Detaching an XDP program goes through
enetc_setup_xdp_prog()->enetc_reconfigure()->enetc_reconfigure_xdp_cb().
netif_set_real_num_tx_queues() can fail there with -ENOMEM when the stack
TX queue count grows. The callback then restores the old program, and
enetc_reconfigure() takes out_restart.

The interface then restarts on the old tx_swbd arrays. Any XDP_TX slot
that hardware had sent but NAPI never cleaned before enetc_stop() still
has is_xdp_tx=1 and a pointer to a freed page.

enetc_setup_txbdr() reloads next_to_use and next_to_clean from
TBPIR/TBCIR, so these stale slots can end up outside the active window.
They are only overwritten when enetc_xdp_map_tx_buff() copies a whole new
swbd into them.

At the next enetc_close() or enetc_reconfigure(), enetc_free_tx_ring()
walks every slot:

    for (i = 0; i < tx_ring->bd_count; i++) {
        struct enetc_tx_swbd *tx_swbd = &tx_ring->tx_swbd[i];

        enetc_free_tx_frame(tx_ring, tx_swbd);
    }

Would that call __free_page() a second time on the already freed page,
and decrement xdp_tx_in_flight a second time?

Nothing resets xdp_tx_in_flight. Once it goes negative, this check in
enetc_clean_rx_ring_xdp() is always true:

    if (cleaned_cnt > rx_ring->xdp.xdp_tx_in_flight)
        enetc_refill_rx_ring(rx_ring, enetc_bd_unused(rx_ring) -
                             rx_ring->xdp.xdp_tx_in_flight);

Can that ask for more buffers than there are free RX BDs, and overwrite
BDs the hardware still owns?

There is a second variant. If a short XDP frame's pad BD lands in a stale
slot, enetc_map_tx_pad() sets only len, is_eof, do_twostep_tstamp and
check_wb. In that case is_xdp_tx and page are left as they were. When
that BD completes, enetc_clean_tx_ring() would call
enetc_recycle_xdp_tx_buff() on the freed page with dma == 0.

Could that put a freed page back into the RX ring for device DMA, and
decrement xdp_tx_in_flight once more?

Also, on rev1, if PI != CI at restart, the hardware would resume the
stale BDs, which now point at unmapped DMA addresses.

>  
>  	if (xdp_frame) {
>  		xdp_return_frame(tx_swbd->xdp_frame);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015105536.1692348-1-vladimir.oltean%40nxp.com