Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] virtio/snd: DMA cacheline sharing for control message payloads --
commit 332dc283a2bc483bd13699b7e6a008d8c41b3af6 Author: Joonho Lee <[email protected]> ALSA: virtio: Keep control payloads in refcounted messages Control requests can include scatterlists that point to caller-owned payloads. A synchronous send can time out while the device still owns those descriptors, so the caller can free a TLV or other payload before a late device access. > diff --git a/sound/virtio/virtio_ctl_msg.c b/sound/virtio/virtio_ctl_msg.c > index 3ad1e1e1b2054..62d4c529922ca 100644 > --- a/sound/virtio/virtio_ctl_msg.c > +++ b/sound/virtio/virtio_ctl_msg.c [ ... ] > @@ -85,20 +119,43 @@ void *virtsnd_ctl_msg_response(struct virtio_snd_msg > *msg) > * Return: Allocated message on success, NULL on failure. > */ > 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) > { > struct virtio_snd_msg *msg; > - > - if (!request_size || !response_size) > + size_t total_size = sizeof(*msg); > + u8 *data; > + > + if (!request_size || !response_size || request_size > UINT_MAX || > + response_size > UINT_MAX || request_data_size > UINT_MAX || > + response_data_size > UINT_MAX || > + check_add_overflow(total_size, request_size, &total_size) || > + check_add_overflow(total_size, response_size, &total_size) || > + check_add_overflow(total_size, request_data_size, &total_size) || > + check_add_overflow(total_size, response_data_size, &total_size)) > return NULL; > > - msg = kzalloc(sizeof(*msg) + request_size + response_size, gfp); > + msg = kzalloc(total_size, gfp); > if (!msg) > return NULL; > > - sg_init_one(&msg->sg_request, (u8 *)msg + sizeof(*msg), request_size); > - sg_init_one(&msg->sg_response, (u8 *)msg + sizeof(*msg) + request_size, > - response_size); > + data = (u8 *)msg + sizeof(*msg); > + sg_init_one(&msg->sg_request, data, (unsigned int)request_size); > + data += request_size; > + sg_init_one(&msg->sg_response, data, (unsigned int)response_size); > + data += response_size; > + msg->request_data_size = request_data_size; > + msg->response_data_size = response_data_size; > + if (request_data_size) { > + sg_init_one(&msg->sg_request_data, data, > + (unsigned int)request_data_size); > + data += request_data_size; > + } > + if (response_data_size) > + sg_init_one(&msg->sg_response_data, data, > + (unsigned int)response_data_size); [Severity: High] Does packing these DMA buffers tightly together after the virtio_snd_msg struct cause cacheline sharing issues? Because sizeof(*msg) and the header sizes are not aligned to ARCH_DMA_MINALIGN, these buffers might share CPU cachelines with each other and with CPU-accessed fields of virtio_snd_msg (like ref_count and cancelled). On non-cache-coherent architectures, mapping a buffer as DMA_FROM_DEVICE (such as sg_response and sg_response_data) causes the DMA API to invalidate the shared cacheline upon unmapping so the CPU sees the device's writes. If the CPU concurrently modifies msg->cancelled or msg->ref_count in that same cacheline (e.g., upon a timeout), could those writes be silently discarded by the cache invalidation? Previously, payloads were allocated separately via kzalloc(size), which guaranteed safe ARCH_DMA_MINALIGN boundaries. Embedding them inline appears to introduce a DMA cacheline sharing hazard. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
