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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Prepended TSB in ndo_xdp_xmit completely overwrites struct xdp_frame.
- [Critical] Headroom validation in XDP_TX ignores metadata size, allowing out-of-bounds writes.
- [High] DMA mapping address in XDP_TX is incorrect when XDP metadata is present.
- [High] Redirected XDP frames will be leaked indefinitely if the target interface is idle.
- [Medium] bcmgenet_xdp_setup() clears NETDEV_XDP_ACT_NDO_XMIT when detaching an XDP program, hiding device capabilities.
- [Medium] Runt XDP frames (less than 60 bytes) will not be padded by the hardware and will be transmitted as undersized packets.
--

commit 893d59a522f087d2ccf486f06a06bb9e815cf330
Author: Nicolai Buchwitz <nb@tipi-net.de>

net: bcmgenet: add XDP_REDIRECT and ndo_xdp_xmit support

Add XDP_REDIRECT support and implement ndo_xdp_xmit for receiving
redirected frames from other devices.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 3c3b0c44ea8a..9dd258567824 100644
> --- 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;
> +		spin_lock(&tx_ring->lock);
> +		if (unlikely(!bcmgenet_xdp_xmit_frame(priv, tx_ring,
> +						      xdpf, false))) {

Does the DMA mapping calculation in bcmgenet_xdp_xmit_frame() for this local
XDP_TX path (dma_map == false) account for XDP metadata?

If an XDP program populates metadata, the calculation sizeof(*xdpf) +
xdpf->headroom in the else block of bcmgenet_xdp_xmit_frame() will point
to the start of the metadata rather than where the TSB was actually prepended
at xdpf->data. The hardware expects the TSB at the DMA address, so it might
misinterpret the metadata as the status block.

> +			spin_unlock(&tx_ring->lock);
>  			xdp_return_frame_rx_napi(xdpf);
>  			return XDP_DROP;
>  		}
> +		bcmgenet_xdp_ring_doorbell(priv, tx_ring);
> +		spin_unlock(&tx_ring->lock);
>  		return XDP_TX;

Does the headroom check earlier in this XDP_TX block adequately validate
xdp->data_meta?

Before backing up the pointers:
	if (xdp->data - xdp->data_hard_start <
	    sizeof(struct status_64) + sizeof(struct xdp_frame)) {

This check validates the space before xdp->data, but the code then subtracts
64 bytes from xdp->data_meta:
	xdp->data -= sizeof(struct status_64);
	xdp->data_meta -= sizeof(struct status_64);

If an XDP program populates metadata, xdp->data_meta will be lower than
xdp->data. The subtraction might push xdp->data_meta below
xdp->data_hard_start, causing out-of-bounds writes when
xdp_convert_buff_to_frame() constructs the struct xdp_frame.

[ ... ]

> @@ -3996,10 +4017,16 @@ static int bcmgenet_xdp_setup(struct net_device *dev,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	if (!prog)
> +		xdp_features_clear_redirect_target(dev);
> +
>  	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;
>  }

Can this unintentionally clear NETDEV_XDP_ACT_NDO_XMIT when detaching an XDP
program?

The driver statically implements ndo_xdp_xmit and advertises it in
dev->xdp_features during probe, indicating the interface can receive XDP
redirected frames from other interfaces regardless of its own XDP program state.
Calling xdp_features_clear_redirect_target(dev) when prog is NULL will
clear the NETDEV_XDP_ACT_NDO_XMIT capability, hiding it from discovery.

[ ... ]

> @@ -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;
> +	int sent = 0;
> +	int i;
> +
> +	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
> +		return -EINVAL;
> +
> +	if (unlikely(!netif_running(dev)))
> +		return -ENETDOWN;
> +
> +	spin_lock(&ring->lock);
> +
> +	for (i = 0; i < num_frames; i++) {
> +		if (!bcmgenet_xdp_xmit_frame(priv, ring, frames[i], true))
> +			break;

Does bcmgenet_xdp_xmit_frame() overwrite the struct xdp_frame metadata when
handling these redirected frames?

When dma_map == true for redirected frames, the driver prepends a 64-byte
hardware TSB by calling memset(xdpf->data - sizeof(struct status_64), 0, 64).
Since the struct xdp_frame is located right before xdpf->data (or any prepended
metadata), writing backwards into the headroom could completely zero out the
struct xdp_frame fields, leading to crashes when returning the frame.

Also, does this cause runt frames to be transmitted as undersized packets?

If the XDP frame is less than 60 bytes, enabling DMA_TX_APPEND_CRC inside
bcmgenet_xdp_xmit_frame() without first padding the frame might trigger the
GENET hardware quirk where it fails to pad packets to the minimum Ethernet size,
causing them to be dropped by receiving switches.

Is there a mechanism to ensure these redirected frames won't be leaked if the
target interface is idle?

Without setting DMA_TX_REQ_INTR in len_stat during transmission in
bcmgenet_xdp_xmit_frame(), TX completions for the XDP TX ring rely entirely
on being polled during the RX NAPI cycle. If the interface is receiving no
normal traffic, the NAPI poll might never run, causing the redirected frames to
sit in the TX ring indefinitely.

> +		sent++;
> +	}

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