Re: [PATCH v4 net 1/8] net: enetc: consistently track dropped frames in enetc_xdp_xmit()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] enetc: Use-After-Free in enetc_xdp_xmit() racing with enetc_close() teardown
- [High] enetc: OOB array access if CPUs exceed hardware TX rings
- [High] enetc: TX ring corruption when XDP_REDIRECT overlaps with network stack
--

commit 805311ddad67da52149365adfc7772abd1f8ca5b
Author: vladimir.oltean@nxp.com <vladimir.oltean@nxp.com>

net: enetc: consistently track dropped frames in enetc_xdp_xmit()
    
This patch updates enetc_xdp_xmit() to consistently increment the xdp_tx_drops
counter for all silently dropped frames. It routes early failures and batch
failures to a common exit path to ensure statistics are updated accurately.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 80f0082f6c633..26abc5fafd188 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1814,15 +1814,18 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
>  	struct enetc_bdr *tx_ring;
>  	int xdp_tx_bd_cnt, i, k;
>  	int xdp_tx_frm_cnt = 0;
> +	int err = 0;
> +
> +	tx_ring = priv->xdp_tx_ring[smp_processor_id()];

[Severity: High]
Can tx_ring point to out-of-bounds memory here if the system has more
possible CPUs than hardware TX rings?

During initialization, the driver configures:

    first_xdp_tx_ring = priv->num_tx_rings - num_possible_cpus();
    priv->xdp_tx_ring = &priv->tx_ring[first_xdp_tx_ring];

If num_possible_cpus() is greater than priv->num_tx_rings,
first_xdp_tx_ring becomes negative, leaving priv->xdp_tx_ring pointing
out-of-bounds.

Because this patch moves the tx_ring assignment before the ENETDOWN check
and adds an unconditional dereference of tx_ring->stats in the goto out
path, an XDP_REDIRECT to this interface will now dereference this invalid
pointer and crash, whereas previously it would safely return -ENETDOWN.

[Severity: High]
Does this setup cause concurrent, lockless modifications of a TX ring
that is already in use by the network stack?

When no local XDP program is loaded, enetc_num_stack_tx_queues() allocates
all priv->num_tx_rings to the network stack. However, the driver also
unconditionally assigns priv->xdp_tx_ring to point to the uppermost
hardware queues.

If another interface uses XDP_REDIRECT to send frames to this device,
enetc_xdp_xmit() executes locklessly here, concurrently modifying the
same TX ring that the standard network stack is using under the Qdisc
__netif_tx_lock, leading to queue corruption.

>  
>  	if (unlikely(test_bit(ENETC_TX_DOWN, &priv->flags) ||
> -		     !netif_carrier_ok(ndev)))
> -		return -ENETDOWN;
> +		     !netif_carrier_ok(ndev))) {
> +		err = -ENETDOWN;
> +		goto out;
> +	}
>  
>  	enetc_lock_mdio();
>  
> -	tx_ring = priv->xdp_tx_ring[smp_processor_id()];
> -
>  	prefetchw(ENETC_TXBD(*tx_ring, tx_ring->next_to_use));

[ ... ]

> @@ -1853,11 +1855,13 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
>  	if (unlikely((flags & XDP_XMIT_FLUSH) || k != xdp_tx_frm_cnt))
>  		enetc_update_tx_ring_tail(tx_ring);
>  
> -	tx_ring->stats.xdp_tx += xdp_tx_frm_cnt;
> -
>  	enetc_unlock_mdio();
>  
> -	return xdp_tx_frm_cnt;
> +out:
> +	tx_ring->stats.xdp_tx += xdp_tx_frm_cnt;

[Severity: High]
Will this result in a use-after-free if it races with the interface teardown?

In the teardown path, enetc_close() does not wait for RCU or hold a
synchronization lock against remote XDP_REDIRECT threads before freeing
the TX rings:

enetc_close()
    enetc_stop()
        set_bit(ENETC_TX_DOWN, &priv->flags);
    ...
    enetc_assign_tx_resources(priv, NULL);

Since this patch changes the early ENETDOWN check to jump to the out
label instead of returning immediately, the code will now unconditionally
dereference tx_ring->stats here even if the interface is going down
and the resources have already been freed.

> +	tx_ring->stats.xdp_tx_drops += num_frames - xdp_tx_frm_cnt;
> +
> +	return err ?: xdp_tx_frm_cnt;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015105536.1692348-1-vladimir.oltean@nxp.com?part=1