Re: [PATCH net-next v8 13/15] net: lan966x: add PCIe FDMA XDP support

[netdev-bot+sashiko]

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

Critical: 1 · High: 0 · Medium: 2 · 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).

- [Critical] Use-after-free and double free on the PCIe RX path when an
  XDP program shrinks a frame.
- [Medium] On the PCIe path the driver accepts XDP but cannot handle
  XDP_REDIRECT, and the bond driver cannot tell.
- [Medium] In lan966x_fdma_pci_napi_poll(), a poll that consumes all
  FDMA_PCI_DCB_MAX (256) RX descriptors never re-arms any of them.

--

net: lan966x: add PCIe FDMA XDP support

This commit adds basic XDP support (XDP_PASS, XDP_DROP, XDP_TX) to the
lan966x PCIe FDMA path. The IFH and FCS are stripped before the program
runs, and the pointer and length from after XDP are passed on to skb
construction or to a new memcpy-based XDP_TX routine.
lan966x_xdp_setup() now accepts programs on PCIe and skips the page_pool
reload there.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> index 7185e65dda435..216e9cbcd1580 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c

[ ... ]

> +static int lan966x_xdp_pci_run(struct lan966x_port *port, void *data,
> +			       u32 data_len, void **xdp_data, u32 *xdp_len)
> +{

[ ... ]

> +	act = bpf_prog_run_xdp(xdp_prog, &xdp);
> +
> +	*xdp_data = xdp.data;
> +	*xdp_len = xdp.data_end - xdp.data;
> +
> +	switch (act) {
> +	case XDP_PASS:
> +		return FDMA_PASS;
> +	case XDP_TX:
> +		return lan966x_fdma_pci_xmit_xdpf(port, *xdp_data, *xdp_len) ?
> +		       FDMA_DROP : FDMA_TX;
> +	default:
> +		bpf_warn_invalid_xdp_action(port->dev, xdp_prog, act);
> +		fallthrough;

[Severity: Medium]
What happens here when the PCIe port is a bond slave?

The PCIe ports use the same lan966x_port_netdev_ops as the platform
ports, so both .ndo_bpf and .ndo_xdp_xmit are set. bond_xdp_set() looks
only at those pointers and never reads xdp_features:

drivers/net/bonding/bond_main.c:bond_xdp_set() {
    ...
		if (!slave_dev->netdev_ops->ndo_bpf ||
		    !slave_dev->netdev_ops->ndo_xdp_xmit) {
    ...
}

This patch removes the -EOPNOTSUPP return for PCIe in
lan966x_xdp_setup(), so a bond with a lan966x PCIe slave can now install
its program on that slave. For bond slaves, bpf_prog_run_xdp() can then
turn XDP_TX into XDP_REDIRECT:

net/core/filter.c:xdp_master_redirect() {
    ...
	if (slave && slave != xdp->rxq->dev) {
		ri->tgt_index = slave->ifindex;
		ri->map_id = INT_MAX;
		ri->map_type = BPF_MAP_TYPE_UNSPEC;
		return XDP_REDIRECT;
    ...
}

It can also return XDP_ABORTED when the master is down.

This switch has no XDP_REDIRECT case. Would those frames go to the
default branch, log a "Driver unsupported" warning and be dropped?
Nothing calls xdp_do_redirect(), so the redirect info set by
xdp_master_redirect() would never be used.

[ ... ]

>  static struct sk_buff *lan966x_fdma_pci_rx_get_frame(struct lan966x_rx *rx,
> -						     u64 src_port)
> +						     u64 src_port, void *data,
> +						     u32 data_len)
>  {

[ ... ]

> -	memcpy(skb->data,
> -	       fdma_dataptr_virt_addr_contiguous(fdma,
> -						 fdma->dcb_index,
> -						 fdma->db_index),
> -						 data_len);
> +	memcpy(skb->data, data, data_len);
>  
>  	skb_put(skb, data_len);
>  
>  	skb->dev = lan966x->ports[src_port]->dev;
> -	skb_pull(skb, IFH_LEN_BYTES);
> -
> -	skb_trim(skb, skb->len - ETH_FCS_LEN);
>  
>  	skb->protocol = eth_type_trans(skb, skb->dev);

[Severity: Critical]
Can this lead to a use-after-free when an XDP program shrinks the frame?

data_len is now the length from after XDP, returned by
lan966x_xdp_pci_run(). bpf_xdp_adjust_tail() and bpf_xdp_adjust_head()
let it shrink to ETH_HLEN. Take a 14-byte frame with h_proto 0x8100
that returns XDP_PASS. eth_type_trans() then leaves skb->len == 0 and
skb->protocol == ETH_P_8021Q.

Now suppose src_port is in bridge_mask and IGMP/MLD redirect is enabled
in ANA_CPU_FWD_CFG. lan966x_port_mc_set() enables it when multicast
snooping is on, and snooping is on by default. In that case the code
after this point reaches:

drivers/net/ethernet/microchip/lan966x/lan966x_main.c:lan966x_hw_offload() {
    ...
	if (eth_type_vlan(skb->protocol)) {
		skb = skb_vlan_untag(skb);
		if (unlikely(!skb))
			return false;
	}
    ...
}

pskb_may_pull(skb, VLAN_HLEN + 2) fails in skb_vlan_untag(), so it
frees the skb with kfree_skb() and returns NULL. lan966x_hw_offload()
only returns a bool, so the caller still holds the freed pointer:

		if (!lan966x_hw_offload(lan966x, src_port, skb))
			skb->offload_fwd_mark = 0;
	}

	skb->dev->stats.rx_bytes += skb->len;

The freed skb is then returned to lan966x_fdma_pci_napi_poll(), which
passes it to napi_gro_receive(). Would that be a use-after-free followed
by a double free?

The problem in lan966x_hw_offload() is older. Before this patch it could
not be reached, because skbs were built from the hardware length and the
MAC drops runts. The frame contents can also come from the network, for
example when a decap program uses adjust_head on a minimum-size outer
frame.

[ ... ]

> @@ -312,8 +430,17 @@ static int lan966x_fdma_pci_napi_poll(struct napi_struct *napi, int weight)
>  			 */
>  			fdma_dcb_advance(fdma);
>  			continue;
> +		case FDMA_TX:
> +			fdma_dcb_advance(fdma);
> +			continue;
> +		case FDMA_DROP:
> +			fdma_dcb_advance(fdma);
> +			continue;
>  		}

[Severity: Medium]
What happens if one poll consumes all 256 (FDMA_PCI_DCB_MAX) RX
descriptors?

Every result path calls fdma_dcb_advance(), which wraps at n_dcbs. After
256 frames, dcb_index is back at the value saved in dcb_reload. The
re-arm loop at the end of lan966x_fdma_pci_napi_poll() then runs zero
times:

	while (dcb_reload != fdma->dcb_index) {
		old_dcb = dcb_reload;
		dcb_reload++;
		dcb_reload &= fdma->n_dcbs - 1;

So none of the consumed DCBs are re-armed, and they all keep
FDMA_DCB_STATUS_DONE. With a budget above 256, fdma_has_frames() stays
true and the same frames are processed again in the same call. With a
budget of exactly 256, the next poll processes them again.

Would this deliver stale frames twice, run XDP on them again, and resend
the XDP_TX frames?

Only the budget bounds the loop. __napi_busy_loop() passes the socket's
SO_BUSY_POLL_BUDGET straight to napi_poll(), and that value can be up to
U16_MAX with CAP_NET_ADMIN. The ring can fill completely, because the
hardware fills the last DCB before it stops on the invalid nextptr.

This loop was added by the earlier "net: lan966x: add PCIe FDMA support"
commit in this series. This patch extends it with the XDP_TX and
XDP_DROP results, and it does not seem to be fixed later in the series.
The platform lan966x_fdma_napi_poll() has the same pattern with 512
DCBs.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-lan966x-pci-fdma-v8-0-201c7b707d8b%40microchip.com