Re: [PATCH] can: virtio: fix virtqueue use before DRIVER_OK

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] can: virtio: RX/TX stall if interrupt fires before
  virtio_can_napi_enable()
--

commit 8c2e9ab878f651e327c76d0ed7de23824865f16e
Author: Seonghyeon Lim <forfortuna@naver.com>

can: virtio: fix virtqueue use before DRIVER_OK

This commit calls virtio_device_ready() before populating the RX virtqueue
in virtio_can_probe() and virtio_can_restore() so that virtqueues are not
notified before DRIVER_OK is set.

> diff --git a/drivers/net/can/virtio_can.c b/drivers/net/can/virtio_can.c
> index f67d0bf09681f..ae35a00b784bd 100644
> --- a/drivers/net/can/virtio_can.c
> +++ b/drivers/net/can/virtio_can.c
> @@ -912,6 +912,9 @@ static int virtio_can_probe(struct virtio_device *vdev)
>  		err = -ENOMEM;
>  		goto on_failure;
>  	}
> +
> +	virtio_device_ready(vdev);
> +
>  	virtio_can_populate_rx_vq(vdev);
>  
>  	err = register_virtio_can_dev(dev);
> @@ -966,6 +969,9 @@ static int __maybe_unused virtio_can_restore(struct virtio_device *vdev)
>  		virtio_can_del_vq(vdev);
>  		return -ENOMEM;
>  	}
> +
> +	virtio_device_ready(vdev);
> +
>  	virtio_can_populate_rx_vq(vdev);
>  
>  	if (netif_running(ndev)) {

[Severity: High]
Can an RX or TX virtqueue interrupt arriving before
virtio_can_napi_enable() permanently wedge the queue?

virtio_device_ready() unbreaks the virtqueues and sets DRIVER_OK while
priv->napi and priv->napi_tx are still disabled (NAPI_STATE_SCHED is set).
In virtio_can_open(), virtio_can_start() then sends
VIRTIO_CAN_SET_CTRL_MODE_START and sleeps in wait_for_completion() before
calling virtio_can_napi_enable():

drivers/net/can/virtio_can.c:virtio_can_open() {
	...
	err = virtio_can_start(ndev);
	if (err) {
		close_candev(ndev);
		return err;
	}

	virtio_can_napi_enable(priv);
}

drivers/net/can/virtio_can.c:virtio_can_rx_intr() {
	struct virtio_can_priv *can_priv = vq->vdev->priv;

	virtqueue_disable_cb(vq);
	napi_schedule(&can_priv->napi);
}

CPU0                              CPU1 (virtio interrupt)
virtio_can_restore() / probe()
  virtio_device_ready(vdev)
  virtio_can_populate_rx_vq(vdev)
  virtio_can_open()
    virtio_can_start()
      ...
      wait_for_completion()
                                  virtio_can_rx_intr()
                                    virtqueue_disable_cb(vq)
                                    napi_schedule(&can_priv->napi)
                                      <no-op: NAPI_STATE_SCHED set>
    virtio_can_napi_enable()
      napi_enable(&priv->napi)    <-- clears NAPI_STATE_SCHED;
                                      vq cb stays disabled

Once napi_enable() clears NAPI_STATE_SCHED without scheduling a poll or
re-enabling the virtqueue callback, virtio_can_rx_poll() never runs and RX
is permanently stalled.

Enabling NAPI before virtio_can_start(), or kicking the virtqueues via
virtqueue_napi_schedule() after napi_enable() as virtio-net does in
virtnet_napi_do_enable(), would avoid losing the notification.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-virtio-can-ready-v1-1-0b4dd020af0b@naver.com?part=1