Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] ALSA: virtio: misaligned control value access in virtsnd_kctl_get() --
commit 15e13c28bcb1079484dfa947a5ae6109ebb5b3c9 Author: Joonho Lee <[email protected]> ALSA: virtio: Keep control payloads alive through DMA completion This commit moves control request and response payloads into a reference-counted DMA buffer owned by struct virtio_snd_msg so caller buffers are not exposed to DMA after a timeout or interruption. It also aligns each scatterlist region to the greater of the DMA cacheline size and the alignment of struct virtio_snd_ctl_value. > diff --git a/sound/virtio/virtio_ctl_msg.c b/sound/virtio/virtio_ctl_msg.c > index 7114bea4033a5..2e0ff15e729da 100644 > --- a/sound/virtio/virtio_ctl_msg.c > +++ b/sound/virtio/virtio_ctl_msg.c [ ... ] > @@ -85,20 +161,85 @@ void *virtsnd_ctl_msg_response(struct virtio_snd_msg > *msg) > struct virtio_snd_msg *virtsnd_ctl_msg_alloc(size_t request_size, > - size_t response_size, gfp_t gfp) > + size_t response_size, > + size_t request_data_size, > + size_t response_data_size, > + gfp_t gfp) > { [ ... ] > + cache_alignment = dma_get_cache_alignment(); > + if (cache_alignment <= 0) > + return NULL; > + /* DMA coherence does not remove the CPU's type alignment requirement. > */ > + alignment = max_t(size_t, cache_alignment, > + __alignof__(struct virtio_snd_ctl_value)); [ ... ] > diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c > index 45f7b6a5b3080..4cf75dedbe9a3 100644 > --- a/sound/virtio/virtio_kctl.c > +++ b/sound/virtio/virtio_kctl.c [ ... ] > @@ -161,10 +163,12 @@ static int virtsnd_kctl_get(struct snd_kcontrol > *kcontrol, > unsigned int i; > int rc; > > - msg = virtsnd_ctl_msg_alloc(request_size, response_size, GFP_KERNEL); > + msg = virtsnd_ctl_msg_alloc(request_size, response_size, 0, 0, > + GFP_KERNEL); > if (!msg) > return -ENOMEM; [Severity: Medium] Does packing both the response header and struct virtio_snd_ctl_value into sg_response leave kvalue misaligned in virtsnd_kctl_get()? In virtsnd_ctl_msg_alloc(), each scatterlist region is aligned to at least __alignof__(struct virtio_snd_ctl_value), which is 8 bytes because of the __le64 integer64[64] array. In virtsnd_kctl_put(), the request header is struct virtio_snd_ctl_hdr (8 bytes), so placing kvalue immediately after it preserves 8-byte alignment. However, in virtsnd_kctl_get(), response_data_size is passed as 0 and response_size is sizeof(struct virtio_snd_hdr) + sizeof(*kvalue), with kvalue computed right after the 4-byte struct virtio_snd_hdr: sound/virtio/virtio_kctl.c:virtsnd_kctl_get() { ... kvalue = (void *)((u8 *)virtsnd_ctl_msg_response(msg) + sizeof(struct virtio_snd_hdr)); switch (type) { ... case VIRTIO_SND_CTL_TYPE_INTEGER64: for (i = 0; i < count; ++i) uvalue->value.integer64.value[i] = le64_to_cpu(kvalue->value.integer64[i]); break; ... } Since virtsnd_ctl_msg_response(msg) is aligned to a multiple of 8 bytes and sizeof(struct virtio_snd_hdr) is 4 bytes, kvalue is guaranteed to land at an 8n + 4 address, causing unaligned 64-bit loads when reading a VIRTIO_SND_CTL_TYPE_INTEGER64 control. Should the control value payload be placed in response_data (or the request/response payloads placed in sg_request_data and sg_response_data) so that kvalue starts on an aligned boundary? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
