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

[Daniel Machon]

> 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?

Re-raise of issue dismissed in v7. xdp_features doesn't advertise REDIRECT on
PCIe, and the warning path is the intended fallback

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