Re: [PATCH v1 5/5] ALSA: virtio: Keep control payloads in refcounted messages

[sashiko-bot@kernel.org]

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 <mynameisjune111@gmail.com>

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/cover.1791867377.git.mynameisjune111@gmail.com?part=5