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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: bcmgenet: add XDP_REDIRECT and ndo_xdp_xmit support

This patch adds XDP_REDIRECT support in the RX path and implements
ndo_xdp_xmit for receiving redirected frames from other devices, batching
frames into ring 16 under a single spinlock. It also advertises
NETDEV_XDP_ACT_REDIRECT and NETDEV_XDP_ACT_NDO_XMIT in xdp_features.

> 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

[ ... ]

> @@ -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);
> +

Does this leave NETDEV_XDP_ACT_NDO_XMIT permanently cleared after a
single attach+detach cycle?

bcmgenet_probe() unconditionally sets:

    dev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
                        NETDEV_XDP_ACT_NDO_XMIT;

but on any call to bcmgenet_xdp_setup() with prog == NULL this code
runs xdp_features_clear_redirect_target(dev), which clears both
NETDEV_XDP_ACT_NDO_XMIT and NETDEV_XDP_ACT_NDO_XMIT_SG in
xdp_features_clear_redirect_target_locked() (net/core/xdp.c).

bcmgenet_xdp_xmit() does not depend on priv->xdp_prog — it only needs
netif_running() and priv->xdp_tx_ring, which is initialized
unconditionally in bcmgenet_init_tx_queues(). So after attach+detach,
is_valid_dst() in kernel/bpf/devmap.c:__xdp_enqueue() will see
NETDEV_XDP_ACT_NDO_XMIT cleared and reject every redirected frame with
-EOPNOTSUPP until another program is attached.

The paired xdp_features_set_redirect_target(dev, false) call only fires
for prog != NULL, so it does not restore the bit after a no-op detach,
and it is redundant with the probe-time advertisement when prog is
non-NULL.

>  	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;
> +	int sent = 0;
> +	int i;
> +
> +	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
> +		return -EINVAL;
> +
> +	if (unlikely(!netif_running(dev)))
> +		return -ENETDOWN;

Is there a use-after-free window between this check and a concurrent
bcmgenet_close()?

netif_running() is an unlocked test_bit on __LINK_STATE_START.
__dev_close_many() clears that bit and then calls ndo_stop, which
reaches bcmgenet_close() -> bcmgenet_netif_stop() -> bcmgenet_fini_dma(),
where kfree(priv->tx_cbs) frees the backing array that ring->cbs points
into.

The synchronize_net() inside dev_deactivate_many() only waits for
outstanding qdisc enqueuers, not for a remote device's NAPI loop that
is inside xdp_do_flush() -> __dev_flush() -> bq_xmit_all() ->
bcmgenet_xdp_xmit() on another CPU.

A caller that passed the netif_running() test just before
__LINK_STATE_START was cleared can then execute bcmgenet_get_txcb() and
dmadesc_set() against freed tx_cbs memory after bcmgenet_fini_dma() has
run.

Would an RCU-protected "xmit enabled" flag cleared on close followed by
synchronize_rcu() before freeing the ring be needed here?

Jakub: I think this is worried that sync_net will be skipped if the device
has no qdiscs on down(). I don't think we care.

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

How are completions reclaimed on priv->xdp_tx_ring (ring 16) when the
device has no local RX traffic?

The existing ring 16 has no TX interrupt and is reclaimed only from
bcmgenet_rx_poll(); the driver comment states:

    /* ring 16 has no interrupt */

bcmgenet_xdp_xmit() itself has no completion path. When this device is
used purely as a redirect target (receiving frames via ndo_xdp_xmit
from other NICs) with little or no local RX traffic, bcmgenet_rx_poll()
is not scheduled, completed descriptors are never reclaimed,
ring->free_bds stays at 0 after the first burst, and
bcmgenet_xdp_xmit_frame() returns false for every subsequent frame.
bcmgenet_xdp_xmit() then returns 0 (all frames dropped) until unrelated
RX traffic happens to arrive.

The XDP_TX path does not hit this because it runs from inside
bcmgenet_rx_poll(), which drains the ring on each call.

Would scheduling tx reclaim from ndo_xdp_xmit, or installing a
completion interrupt or timer for ring 16, be appropriate here?

> +
>  static const struct net_device_ops bcmgenet_netdev_ops = {
>  	.ndo_open		= bcmgenet_open,
>  	.ndo_stop		= bcmgenet_close,

[ ... ]

> @@ -4327,7 +4385,8 @@ static int bcmgenet_probe(struct platform_device *pdev)
>  			 NETIF_F_RXCSUM;
>  	dev->hw_features |= dev->features;
>  	dev->vlan_features |= dev->features;
> -	dev->xdp_features = NETDEV_XDP_ACT_BASIC;
> +	dev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
> +			    NETDEV_XDP_ACT_NDO_XMIT;
>  
>  	netdev_sw_irq_coalesce_default_on(dev);