Re: [PATCH v3 5/6] platform/mellanox: mlxbf-tmfifo: implement synchronize_cbs()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Device reset and `synchronize_cbs()` fail to stop the background work handler, leading to a race condition and Use-After-Free during virtqueue removal.
--

commit 7ff4c967b176976015826d77ef33286ed4bdfbba
Author: Karl Mehltretter <kmehltretter@gmail.com>

platform/mellanox: mlxbf-tmfifo: implement synchronize_cbs()

This commit implements synchronize_cbs() for the mlxbf-tmfifo driver. It uses
the existing per-direction spinlocks to synchronize with virtqueue callbacks
running in the FIFO work item instead of relying on synchronize_rcu().

> diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c b/drivers/platform/mellanox/mlxbf-tmfifo.c
> index 3c6408581373b..664bec20a7aeb 100644
> --- a/drivers/platform/mellanox/mlxbf-tmfifo.c
> +++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
> @@ -1135,6 +1135,20 @@ static void mlxbf_tmfifo_virtio_reset(struct virtio_device *vdev)
>  	tm_vdev->status = 0;
>  }
>  
> +/* Synchronize with callbacks running in the FIFO work item. */
> +static void mlxbf_tmfifo_virtio_synchronize_cbs(struct virtio_device *vdev)
> +{
> +	struct mlxbf_tmfifo_vdev *tm_vdev = mlxbf_vdev_to_tmfifo(vdev);
> +	struct mlxbf_tmfifo *fifo = tm_vdev->vrings[0].fifo;
> +	unsigned long flags;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(fifo->spin_lock); i++) {
> +		spin_lock_irqsave(&fifo->spin_lock[i], flags);
> +		spin_unlock_irqrestore(&fifo->spin_lock[i], flags);
> +	}
> +}

[Severity: Critical]
This is a pre-existing issue, but does toggling the spinlocks here actually
guarantee that the work handler has finished and won't execute again?

If a privileged user unbinds the virtio device via sysfs while traffic is
actively processing, the driver core calls virtio_reset_device(), which
invokes mlxbf_tmfifo_virtio_reset() and mlxbf_tmfifo_virtio_synchronize_cbs().

Because mlxbf_tmfifo_virtio_reset() fails to halt or cancel the background
work item, the core proceeds to free the virtqueues in
mlxbf_tmfifo_virtio_del_vqs().

The concurrently running work handler (mlxbf_tmfifo_work_rxtx()) can then
check the virtqueue pointer outside the spinlocks and proceed to use it:

drivers/platform/mellanox/mlxbf-tmfifo.c:mlxbf_tmfifo_work_rxtx() {
...
	vring = &tm_vdev->vrings[queue_id];
	if (vring->vq)
		mlxbf_tmfifo_rxtx(vring, is_rx);
...
}

And finally dereference the freed virtqueue in mlxbf_tmfifo_rxtx(), causing a
use-after-free:

drivers/platform/mellanox/mlxbf-tmfifo.c:mlxbf_tmfifo_rxtx() {
...
	/* Notify upper layer that packet is done. */
	spin_lock_irqsave(&fifo->spin_lock[is_rx], flags);
	vring_interrupt(0, vring->vq);
	spin_unlock_irqrestore(&fifo->spin_lock[is_rx], flags);
...
}

This issue wasn't introduced by this patch, as the former synchronize_rcu()
fallback also failed to protect the non-RCU work handler, but this patch fails
to correct the vulnerability.

Should the background work item be canceled during reset to prevent this?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908053817.26065-1-kmehltretter@gmail.com?part=5