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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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).

- [Medium] This patch removes the PCIe rejection from lan966x_xdp_setup()
  (drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c), so a bond can…
- [Low] In lan966x_xdp_pci_run(), xdp_init_buff() is called with frame_sz
  = fdma->db_size, and hard_start is the slot start (virt_addr -…

--

net: lan966x: add PCIe FDMA XDP support

This commit adds XDP support to the lan966x PCIe FDMA path. The IFH and
FCS are stripped before the program runs, and XDP_TX is done by copying
the frame into a TX slot behind a newly built IFH. lan966x_xdp_setup()
no longer rejects PCIe and skips the page_pool reload on that path.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> index 758554c951c52..949994874ed96 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c

[ ... ]

> @@ -139,7 +140,123 @@ static bool lan966x_fdma_pci_rx_size_fits(struct fdma *fdma, u32 blockl)
>  	       blockl <= fdma->db_size - XDP_PACKET_HEADROOM;
>  }
>  

[ ... ]

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

[ ... ]

> +	xdp_init_buff(&xdp, fdma->db_size, &port->xdp_rxq);
> +
> +	/* hard_start is set to slot start (virt_addr is XDP_PACKET_HEADROOM
> +	 * into the slot). Headroom includes the IFH; BPF may grow into it
> +	 * via adjust_head. IFH is rebuilt on XDP_TX and unread on XDP_PASS.
> +	 */
> +	xdp_prepare_buff(&xdp,
> +			 data - XDP_PACKET_HEADROOM,
> +			 XDP_PACKET_HEADROOM + IFH_LEN_BYTES,
> +			 data_len,
> +			 false);

[Severity: Low]
Can data_end end up past xdp_data_hard_end() here?

frame_sz is the full db_size and hard_start is the start of the slot, so
the XDP core computes the data limit as:

include/net/xdp.h:xdp_data_hard_end()
    ((xdp)->data_hard_start + (xdp)->frame_sz -
     SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))

The upper bound in lan966x_fdma_pci_rx_size_fits(), shown in the context
above, only reserves the headroom:

    blockl <= fdma->db_size - XDP_PACKET_HEADROOM;

lan966x_fdma_pci_rx_check_frame() sets data to slot + XDP_PACKET_HEADROOM
+ IFH_LEN_BYTES and data_len to blockl - IFH_LEN_BYTES - ETH_FCS_LEN. At
the largest accepted blockl, data_end is then slot + db_size -
ETH_FCS_LEN, roughly 316 bytes past xdp_data_hard_end().

For such a buffer, bpf_xdp_adjust_tail() returns -EINVAL for any offset,
shrinks included, because it checks this first:

    if (unlikely(data_end > data_hard_end))
        return -EINVAL;

Reaching this needs a blockl that eats into the shinfo reserve, for
example a malformed BLOCKL. It could also be an oversized frame truncated
to DATAL while lan966x_port_change_mtu() has raised MAXLEN but
fdma_resize() has not yet reallocated the rings.

Memory safety is not affected, since data_end stays inside the slot the
driver owns.

Should lan966x_fdma_pci_rx_size_fits() also reserve
SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) so it matches the frame_sz
given to xdp_init_buff()?

> +
> +	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;
> +	case XDP_ABORTED:
> +		trace_xdp_exception(port->dev, xdp_prog, act);
> +		fallthrough;
> +	case XDP_DROP:
> +		return FDMA_DROP;
> +	}
> +}

[Severity: Medium]
Now that lan966x_xdp_setup() no longer rejects PCIe, can a bond attach a
program to lan966x PCIe ports and then have its XDP_TX frames silently
dropped here?

The ports share lan966x_port_netdev_ops, which provides both .ndo_bpf and
.ndo_xdp_xmit. That is all bond_xdp_set() checks, so it accepts these
ports. It then pushes the program down:

bond_xdp_set()
    dev_xdp_propagate()
        lan966x_xdp_setup()    <- now succeeds on PCIe

bond_xdp_set() also calls
static_branch_inc(&bpf_master_redirect_enabled_key). After that,
bpf_prog_run_xdp() rewrites XDP_TX on a bond slave:

    if (act == XDP_TX && netif_is_bond_slave(xdp->rxq->dev))
        act = xdp_master_redirect(xdp);

In round-robin, XOR and 802.3ad modes, xdp_master_redirect() returns
XDP_REDIRECT whenever the bond picks a transmit slave other than the
receiving port. That action falls into the default case above:

bpf_warn_invalid_xdp_action() -> trace_xdp_exception() -> FDMA_DROP

A bond program that only returns XDP_TX attaches without error, but a
hash- or round-robin-dependent share of its packets is dropped. With two
slaves in round-robin, that is about half. Before this patch the attach
failed with -EOPNOTSUPP.

Should the PCIe path handle XDP_REDIRECT, or keep refusing the attach
when the port is a bond slave?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-lan966x-pci-fdma-v9-0-a17caf60f89a%40microchip.com