Re: [RFC PATCH net-next 10/13] net/mlx5e: add knod XDP offload support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] mlx5e_open() starts the offload worker unconditionally even if mlx5e_open_locked() fails.
- [High] mlx5e_knod_uninit() illegally frees priv->knodev which is owned and freed by the KNOD framework.
- [High] Unbounded iteration over channels causes out-of-bounds accesses on `knodev->wpriv` and `mlx5e_knod_release_batch`.
- [High] mlx5e_xmit_xdp_offload_buff() incorrectly casts standard netmem_ref pages to net_iov, reading garbage DMA addresses.
- [High] RX buffers mapped as DMA_FROM_DEVICE are transmitted on the TX queue without remapping, causing IOMMU faults.
- [High] Global static batch array is used concurrently by different netdev NAPI instances.
- [High] Missing channel restart during offload attach/detach leads to silent failure and Use-After-Free of `rq->knodev`.
- [High] `mlx5e_rx_offload_stop` races with active NAPI RX processing, leading to SPSC ring corruption and use-after-free of page pool pages.
- [High] A permanent failure during offload transmission (e.g., oversized MTU) causes the release loop to wedge forever, breaking the RX queue.
- [Low] The `nic_ops` structure is exported to the global linker namespace without the `static` keyword.
--

--- Patch [10]: [RFC PATCH net-next 10/13] net/mlx5e: add knod XDP offload support ---
commit de6f2fbf7fa4e2b457c356167b75d55f4c90a073
Author: Taehee Yoo <ap420073@gmail.com>

net/mlx5e: add knod XDP offload support

Let mlx5e act as a knod NIC: register with the knod core, feed received
packets to the accelerator over the per-queue SPSC ring, and transmit
accelerator verdicts (XDP_TX) back through the XDP SQ.  Restricted to
inline-none TX and single-page MTU.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
> index d8c7cb8837d74..82adfc9b7c1b6 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c

[ ... ]

> +static inline bool mlx5e_xmit_xdp_offload_buff(struct mlx5e_xdpsq *sq,
> +					       struct mlx5e_rq *rq,
> +					       struct spsc_bd *bd)
> +{
> +	struct mlx5e_xmit_data_frags xdptxdf = {};
> +	struct mlx5e_xmit_data *xdptxd;
> +
> +	/* attach is restricted to inline-none NICs, so the WQE inlines no
> +	 * header and xdptxd->data is never read (left NULL here).
> +	 */
> +	xdptxd = &xdptxdf.xd;
> +	xdptxd->len = bd->len;
> +	xdptxd->has_frags = 0;
> +	xdptxd->dma_addr = netmem_to_net_iov(bd->netmem)->desc.dma_addr +
> +			   bd->off;

[Severity: High]
Is this cast safe if the memory isn't a devmem/net_iov?

If the interface uses standard host memory, `netmem_to_net_iov()` will read
garbage data for the DMA address instead of properly mapping it.

> +
> +	if (!mlx5e_xmit_xdp_frame(sq, xdptxd, 0, NULL))
> +		return false;

[Severity: High]
Will this trigger an IOMMU access violation?

If rq->buff.map_dir defaults to DMA_FROM_DEVICE (when there's no software
XDP program), this transmits the buffer without remapping it for device
read access.

[ ... ]

> +struct mlx5e_knod_release_batch {
> +	struct spsc_bd *bds[NAPI_POLL_WEIGHT];
> +	struct spsc_pass_bd pass[NAPI_POLL_WEIGHT];
> +};
> +
> +static struct mlx5e_knod_release_batch
> +mlx5e_knod_release_batch[KNOD_SPSC_MAX];

[Severity: High]
Is this static array prone to cross-device data races?

If multiple mlx5e network interfaces are active concurrently, their NAPI
threads might run in parallel on different CPUs. Because this array is
global and indexed only by rq->ix in mlx5e_rx_offload_release_pending(),
they could overwrite each other's batches.

Additionally, since mlx5e supports creating one RX channel per CPU core,
can rq->ix exceed KNOD_SPSC_MAX and cause an out-of-bounds access?

[ ... ]

> +			case KNOD_TX:
> +				if (!mlx5e_xmit_xdp_offload_buff(rq->xdpsq, rq,
> +								 bds[i]))
> +					goto stop_release;

[Severity: High]
Does this cause an infinite retry loop for permanent failures?

If mlx5e_xmit_xdp_offload_buff() fails because a packet exceeds the TX MTU
limit, taking this goto stop_release leaves the buffer in the ring. 

The next NAPI poll would then retry the exact same oversized packet
indefinitely, stalling the queue.

[ ... ]

> +static int mlx5e_rx_offload_xdp_attach(struct knod_dev *knodev)
> +{
[ ... ]
> +	pr_debug("Attaching XDP offload to netdev %s\n", knodev->netdev->name);
> +	WRITE_ONCE(priv->knodev, knodev);
> +
> +	return 0;
> +}
> +
> +static int mlx5e_rx_offload_xdp_detach(struct knod_dev *knodev)
> +{
> +	struct mlx5e_priv *priv = netdev_priv(knodev->netdev);
> +
> +	pr_debug("Detaching XDP offload from netdev %s\n",
> +		 knodev->netdev->name);
> +	WRITE_ONCE(priv->knodev, NULL);
> +
> +	return 0;
> +}

[Severity: High]
Could this lead to use-after-free and silent data path failures if the
interface is administratively UP?

By updating priv->knodev without restarting the active network channels
(e.g., via mlx5e_safe_switch_params()), a hot-detach leaves the active RX
queues holding stale pointers to a knodev structure that the framework
is about to free.

> +
> +struct knod_nic_ops nic_ops = {
> +	.attach = mlx5e_rx_offload_xdp_attach,
> +	.detach = mlx5e_rx_offload_xdp_detach,
> +};

[Severity: Low]
Should this struct be marked static?

Exporting nic_ops to the global namespace could create a collision hazard
if any other driver defines a variable with the same name.

> +
> +int mlx5e_knod_init(struct mlx5e_priv *priv)
[ ... ]
> +void mlx5e_knod_uninit(struct mlx5e_priv *priv)
> +{
> +	knod_netdev_unregister(priv->knetdev);
> +	kfree(priv->knetdev);
> +	priv->knetdev = NULL;
> +	kfree(priv->knodev);
> +	WRITE_ONCE(priv->knodev, NULL);
> +}

[Severity: High]
Does this code free memory it doesn't own?

The knodev pointer is a framework-owned object passed via the attach()
callback. Calling kfree(priv->knodev) here could lead to a double-free
when the framework subsequently cleans up the device.

[ ... ]

> +void mlx5e_rx_offload_stop(struct mlx5e_priv *priv)
> +{
[ ... ]
> +	synchronize_net();
> +	for (i = 0; i < KNOD_SPSC_MAX; i++) {
[ ... ]
> +		spsc_rewind(&knodev->wpriv[i].spsc_bds);
> +		while (!spsc_pop(&knodev->wpriv[i].spsc_bds, (void **)&bd))
> +			page_pool_put_full_netmem(netmem_get_pp(bd->netmem),
> +						  bd->netmem, true);
> +	}
> +}

[Severity: High]
Could this race with NAPI and corrupt the SPSC ring or page pool?

In mlx5e_close(), mlx5e_rx_offload_stop() is called before
mlx5e_close_locked(), meaning NAPI is still actively processing.

Calling page_pool_put_full_netmem() with allow_direct = true concurrently
with NAPI violates the page pool API locking constraints.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index aa8610cedaa85..cfc6ba6dc492b 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -918,6 +918,7 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
>  	pool_size = 1 << params->log_rq_mtu_frames;
>  
>  	rq->mkey_be = cpu_to_be32(mdev->mlx5e_res.hw_objs.mkey);
> +	rq->knodev = rq->priv->knodev;

[Severity: High]
Does rq->buff.map_dir need to be DMA_BIDIRECTIONAL when hardware offload
is active?

In mlx5e_alloc_rq() just above this addition, rq->buff.map_dir is set to
DMA_FROM_DEVICE if params->xdp_prog is false. This causes IOMMU faults
later when the buffer is transmitted directly on the TX queue without being
remapped.

[ ... ]

> @@ -3592,6 +3597,7 @@ int mlx5e_open(struct net_device *netdev)
>  		mlx5e_modify_admin_state(priv->mdev, MLX5_PORT_UP);
>  	mutex_unlock(&priv->state_lock);
>  
> +	mlx5e_rx_offload_start(priv);
>  	return err;
>  }

[Severity: High]
Should mlx5e_rx_offload_start() be skipped if mlx5e_open_locked() fails?

If err indicates failure, the netdev remains closed, so mlx5e_close()
won't be called to clean up. This leaves the worker running indefinitely
and can cause a use-after-free when it accesses the failed queues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260719175857.4071636-1-ap420073@gmail.com?part=10