Re: [PATCH v1 3/5] ALSA: virtio: Retain control requests through device reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] virtio_sound: DMA mapping leak from uncalled virtqueue_detach_unused_buf()
- [Low] cleanup: mixing goto and scoped_guard in virtsnd_ctl_msg_send()
--

commit 58ca5d3aaf697e12525c1d956e26244a616c7d76
Author: Joonho Lee <mynameisjune111@gmail.com>
ALSA: virtio: Retain control requests through device reset

This commit updates the virtio sound driver to handle control message teardown
by keeping references to messages through device reset. It marks messages
canceled under the queue lock and defers freeing them until the queue has been
deleted.

> diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c
> index 6f35276416fed..ed884327bb0ef 100644
> --- a/sound/virtio/virtio_card.c
> +++ b/sound/virtio/virtio_card.c
> @@ -349,13 +349,15 @@ static void virtsnd_remove(struct virtio_device *vdev)
>  	unsigned int i;
>  
>  	virtsnd_disable_event_vq(snd);
> -	virtsnd_ctl_msg_cancel_all(snd);
>  
>  	if (snd->card)
>  		snd_card_free(snd->card);
>  
> +	virtsnd_ctl_msg_cancel_all(snd);
> +
>  	virtio_reset_device(vdev);
>  	vdev->config->del_vqs(vdev);
> +	virtsnd_ctl_msg_cleanup(snd);

[Severity: Medium]
Does deleting the virtqueues here after reset without first detaching the
unused buffers leak pending DMA mappings? Because virtqueue_add_sgs() creates
DMA mappings for these buffers, if they aren't drained with
virtqueue_detach_unused_buf() before del_vqs(), those mappings might not be
unmapped.

[ ... ]

> @@ -385,6 +387,7 @@ static int virtsnd_freeze(struct virtio_device *vdev)
>  
>  	virtio_reset_device(vdev);
>  	vdev->config->del_vqs(vdev);
> +	virtsnd_ctl_msg_cleanup(snd);

[Severity: Medium]
Similarly, does suspending the device here destroy the virtqueues without
detaching and unmapping pending DMA buffers?


> diff --git a/sound/virtio/virtio_ctl_msg.c b/sound/virtio/virtio_ctl_msg.c
> index 6433c870f271a..3ad1e1e1b2054 100644
> --- a/sound/virtio/virtio_ctl_msg.c
> +++ b/sound/virtio/virtio_ctl_msg.c
> @@ -147,12 +150,17 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
>  		psgs[nouts + nins++] = in_sgs;
>  
>  	scoped_guard(spinlock_irqsave, &queue->lock) {
> -		rc = virtqueue_add_sgs(queue->vqueue, psgs, nouts, nins, msg,
> -				       GFP_ATOMIC);
> +		if (snd->ctl_msgs_blocked)
> +			rc = -ENODEV;
> +		else
> +			rc = virtqueue_add_sgs(queue->vqueue, psgs, nouts, nins,
> +					       msg, GFP_ATOMIC);
>  		if (!rc) {
>  			notify = virtqueue_kick_prepare(queue->vqueue);
>  
>  			list_add_tail(&msg->list, &snd->ctl_msgs);
> +			if (notify)
> +				virtqueue_notify(queue->vqueue);
>  		}
>  	}

[ ... ]

> @@ -187,6 +192,11 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
>  		goto on_exit;
>  	}
>  
> +	if (READ_ONCE(msg->cancelled)) {
> +		rc = -ENODEV;
> +		goto on_exit;
> +	}
> +
>  	switch (le32_to_cpu(response->code)) {
>  	case VIRTIO_SND_S_OK:
>  		rc = 0;

[Severity: Low]
This isn't a bug, but does mixing scope-based cleanup like scoped_guard with
traditional goto-based error handling (goto on_exit) violate the cleanup
guidelines?

The guidelines state that for a given routine, either all resources should be
converted to scope-based cleanup or none of them should be, to prevent brittle
ownership semantics and future bugs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791867377.git.mynameisjune111@gmail.com?part=3