Re: [PATCH net-next v7 5/7] net: bcmgenet: add XDP_REDIRECT and ndo_xdp_xmit support

[sashiko-bot@kernel.org]

Sashiko AI review found 5 potential issue(s):
- [High] Headroom check in `bcmgenet_run_xdp` uses `xdp->data` instead of `xdp->data_meta`, causing valid packets with metadata to be dropped.
- [High] Redirected frames are never reclaimed if the interface lacks RX traffic, leading to DoS via page pool starvation.
- [High] Tearing down the XDP TX ring causes a severe out-of-bounds access on the `dev->_tx` array during interface close.
- [High] `bcmgenet_xdp_xmit` accesses unclocked hardware registers during system suspend, causing an asynchronous external abort/system lockup.
- [Medium] Inconsistent XDP features reporting for XDP redirect support.
--

commit 9f494eaf789828bf8ab38fcd7ac6a3e4b72f5798
Author: Nicolai Buchwitz <nb@tipi-net.de>

net: bcmgenet: add XDP_REDIRECT and ndo_xdp_xmit support

This commit adds XDP_REDIRECT support to the bcmgenet driver. It implements
ndo_xdp_xmit to allow receiving redirected frames and updates the RX path
to handle the XDP_REDIRECT action.

> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
[ ... ]
> @@ -2451,11 +2452,25 @@ static unsigned int bcmgenet_run_xdp(struct bcmgenet_rx_ring *ring,
>  						true);
>  			return XDP_DROP;
>  		}
> -		if (unlikely(!bcmgenet_xdp_xmit_frame(priv, xdpf, false))) {
> +
> +		tx_ring = &priv->xdp_tx_ring;

Looking at the code just before this in the XDP_TX path, the headroom is
checked using xdp->data - xdp->data_hard_start.

Since this includes any prepended metadata space, if xdp->data and
xdp->data_meta are shifted backwards by 64 bytes for the Transmit Status
Block, could xdp->data_meta underflow data_hard_start or leave insufficient
room for the xdp_frame?

Would it be more correct to check the headroom using xdp->data_meta instead
to avoid unexpectedly dropping valid packets with metadata?

> +		spin_lock(&tx_ring->lock);
> +		if (unlikely(!bcmgenet_xdp_xmit_frame(priv, tx_ring,
> +						      xdpf, false))) {
> +			spin_unlock(&tx_ring->lock);
>  			xdp_return_frame_rx_napi(xdpf);
>  			return XDP_DROP;
>  		}
[ ... ]
> @@ -3996,10 +4017,16 @@ static int bcmgenet_xdp_setup(struct net_device *dev,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	if (!prog)
> +		xdp_features_clear_redirect_target(dev);
> +

Since the XDP TX ring is always initialized when the interface is brought
up, the device can act as a redirect target even without an RX XDP program
loaded.

If a user attaches and then detaches an XDP program, will this clear call
spuriously stop advertising the device's ability to act as an XDP redirect
target?

>  	old_prog = xchg(&priv->xdp_prog, prog);
>  	if (old_prog)
>  		bpf_prog_put(old_prog);
>  
> +	if (prog)
> +		xdp_features_set_redirect_target(dev, false);
> +
>  	return 0;
>  }
[ ... ]
> @@ -4013,6 +4040,36 @@ static int bcmgenet_xdp(struct net_device *dev, struct netdev_bpf *xdp)
>  	}
>  }
>  
> +static int bcmgenet_xdp_xmit(struct net_device *dev, int num_frames,
> +			     struct xdp_frame **frames, u32 flags)
> +{
> +	struct bcmgenet_priv *priv = netdev_priv(dev);
> +	struct bcmgenet_tx_ring *ring = &priv->xdp_tx_ring;

When bcmgenet_close() tears down the interface, it invokes
bcmgenet_tx_reclaim_all(dev) which unconditionally executes
netdev_tx_reset_queue(netdev_get_tx_queue(dev, ring->index)).

Since the netdev only allocates GENET_MAX_MQ_CNT + 1 (5) TX queues, and the
XDP TX ring uses ring->index = DESC_INDEX (16), will this attempt to
access dev->_tx[16] out of bounds?

> +	int sent = 0;
> +	int i;
> +
> +	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
> +		return -EINVAL;
> +
> +	if (unlikely(!netif_running(dev)))
> +		return -ENETDOWN;

During system suspend, bcmgenet_suspend() disables device clocks but leaves
__LINK_STATE_START active, so netif_running(dev) remains true.

If a redirect occurs while suspended, could this attempt to write to
unclocked MMIO registers and trigger a hard bus lockup on ARM architectures?

Is it possible to use netif_device_present() to ensure frames are dropped
safely when the hardware is detached?

> +
> +	spin_lock(&ring->lock);
> +
> +	for (i = 0; i < num_frames; i++) {
> +		if (!bcmgenet_xdp_xmit_frame(priv, ring, frames[i], true))
> +			break;
> +		sent++;
> +	}

Ring 16 operates without hardware TX interrupts and relies exclusively on
the RX NAPI polling loop to reclaim its transmitted buffers.

If the interface acts as a redirect target but receives no physical
incoming traffic, its RX NAPI is never scheduled.

Could this cause the XDP TX ring to fill up, dropping all subsequent
redirected frames and indefinitely leaking or starving the source
interface's page pool buffers?

> +
> +	if (sent)
> +		bcmgenet_xdp_ring_doorbell(priv, ring);
> +
> +	spin_unlock(&ring->lock);
> +
> +	return sent;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260416054743.1289191-1-nb@tipi-net.de?part=5