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 <[email protected]> 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/[email protected]?part=3
