Re: [PATCH net-next 2/4] i40e: add support for bpf_xdp_metadata_rx_hash()

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] i40e_config_netdev() sets `netdev->xdp_metadata_ops =
  &i40e_xdp_metadata_ops;` unconditionally…

--

i40e: add support for bpf_xdp_metadata_rx_hash()

This adds i40e_xdp_rx_hash() as an xdp_metadata_ops callback, extracting
the RSS hash and hash type from the RX descriptor the same way
i40e_rx_hash() does. A new I40E_RX_DESC_STATUS_FLTSTAT_MASK helper is
added to i40e_type.h and the ops struct is installed on the netdev.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 4b11ae9ed8d53..e487d697a634f 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c

[ ... ]

> @@ -13582,6 +13583,34 @@ static int i40e_xdp(struct net_device *dev,
>  	}
>  }
>  
> +static int i40e_xdp_rx_hash(const struct xdp_md *_ctx, u32 *hash,
> +			    enum xdp_rss_hash_type *rss_type)
> +{
> +	const struct i40e_xdp_buff *ctx = (const void *)_ctx;
> +	const union i40e_rx_desc *desc = ctx->desc;
> +	struct libeth_rx_pt rx_ptype;
> +	u8 raw_rx_ptype;
> +	u64 status;
> +
> +	status = le64_to_cpu(desc->wb.qword1.status_error_len);
> +	raw_rx_ptype = FIELD_GET(I40E_RXD_QW1_PTYPE_MASK, status);
> +	rx_ptype = libie_rx_pt_parse(raw_rx_ptype);
> +
> +	if (!libeth_rx_pt_has_hash(ctx->xdp.rxq->dev, rx_ptype) ||
> +	    FIELD_GET(I40E_RX_DESC_STATUS_FLTSTAT_MASK, status) !=
> +		    I40E_RX_DESC_FLTSTAT_RSS_HASH)
> +		return -ENODATA;
> +
> +	*hash = le32_to_cpu(desc->wb.qword0.hi_dword.rss);
> +	*rss_type = rx_ptype.hash_type;
> +
> +	return 0;
> +}
> +
> +static const struct xdp_metadata_ops i40e_xdp_metadata_ops = {
> +	.xmo_rx_hash		= i40e_xdp_rx_hash,
> +};
> +
>  static const struct net_device_ops i40e_netdev_ops = {
>  	.ndo_open		= i40e_open,
>  	.ndo_stop		= i40e_close,

[ ... ]

> @@ -13784,6 +13813,7 @@ static int i40e_config_netdev(struct i40e_vsi *vsi)
>  	i40e_vsi_config_netdev_tc(vsi, vsi->tc_config.enabled_tc);
>  
>  	netdev->netdev_ops = &i40e_netdev_ops;
> +	netdev->xdp_metadata_ops = &i40e_xdp_metadata_ops;

[Severity: Medium]
Should this assignment live inside the I40E_VSI_MAIN branch, next to where
the rest of the XDP capability state is set?

Earlier in i40e_config_netdev() the XDP capabilities are set only for the
main VSI:

	if (vsi->type == I40E_VSI_MAIN) {
		...
		netdev->xdp_features = NETDEV_XDP_ACT_BASIC |
				       NETDEV_XDP_ACT_REDIRECT |
				       NETDEV_XDP_ACT_XSK_ZEROCOPY |
				       NETDEV_XDP_ACT_RX_SG;
		netdev->xdp_zc_max_segs = I40E_MAX_BUFFER_TXD;
	} else {
		/* Relate the VSI_VMDQ name to the VSI_MAIN name ... */

but i40e_config_netdev() also runs for VMDQ2 netdevs, via i40e_vsi_setup():

	switch (vsi->type) {
	/* setup the netdev if needed */
	case I40E_VSI_MAIN:
	case I40E_VSI_VMDQ2:
		ret = i40e_config_netdev(vsi);

Those macvlan-offload "<pf>v%d" netdevs get xdp_features == 0, and
i40e_xdp() rejects every ndo_bpf command on them:

	if (vsi->type != I40E_VSI_MAIN)
		return -EINVAL;

So no native XDP program can be attached there, and xmo_rx_hash can never
be reached on such a netdev.

Does this make the netlink capability report inconsistent?  netdev_nl_dev_fill()
derives the xdp-rx-metadata-features bits purely from the presence of the
callback, with no cross-check against dev->xdp_features:

net/core/netdev-genl.c:netdev_nl_dev_fill() {
	...
#define XDP_METADATA_KFUNC(_, flag, __, xmo)			\
	if (netdev->xdp_metadata_ops && netdev->xdp_metadata_ops->xmo)	\
		xdp_rx_meta |= flag;
	...
}

which means a VMDQ2 netdev advertises NETDEV_XDP_RX_METADATA_HASH ("Device is
capable of exposing receive packet hash via bpf_xdp_metadata_rx_hash()") while
rejecting XDP entirely.

The follow-up patch adding bpf_xdp_metadata_rx_vlan_tag() extends the same
unconditional assignment, so NETDEV_XDP_RX_METADATA_VLAN_TAG ends up reported
on those netdevs too; the assignment is still unconditional at the end of the
series.

>  	netdev->watchdog_timeo = 5 * HZ;
>  	i40e_set_ethtool_ops(netdev);
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901211752.1922993-1-anthony.l.nguyen%40intel.com