Re: [RFC PATCH 11/12] virtio: expose device memory buffer occupancy over debugfs

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] `virtio_dmb_hiwater_set` contains a TOCTOU race with `virtio_dmb_inc_used` that can cause the high-water mark to be permanently set lower than the current total usage.
--

--- Patch [11]: [RFC PATCH 11/12] virtio: expose device memory buffer occupancy over debugfs ---
commit 821ee4cbeac917d62e62a95c88f58236d2f6db9c
Author: Alexander Graf <[email protected]>

virtio: expose device memory buffer occupancy over debugfs

This commit adds debugfs files to expose the memory occupancy of virtio
device memory buffers when CONFIG_VIRTIO_DEBUG is enabled. It introduces
counters for total allocated pages, allocation failures, and a high-water
mark to track peak usage.

> diff --git a/drivers/virtio/virtio_dmb.c b/drivers/virtio/virtio_dmb.c
> index 42126f928bc06..fe556d585f244 100644
> --- a/drivers/virtio/virtio_dmb.c
> +++ b/drivers/virtio/virtio_dmb.c

[ ... ]

> +static int virtio_dmb_hiwater_set(void *data, u64 val)
> +{
> +	struct virtio_dmb *dmb = data;
> +
> +	/* Restarting the measurement is the only meaningful write. */
> +	if (val)
> +		return -EINVAL;
> +
> +	/*
> +	 * Restart from what is allocated now rather than from zero, so that
> +	 * the file never reports a peak below the occupancy it is read
> +	 * alongside.
> +	 */
> +	atomic_long_set(&dmb->used_hiwater,
> +			atomic_long_read(&dmb->total_used));

[Severity: Medium]
Can this sequence result in the high-water mark being set lower than the
current total usage due to a race condition?

If a write to the debugfs used_pages_hiwater file occurs concurrently with
new allocations, could we see the following sequence:

Thread A reads total_used (e.g., 10) in virtio_dmb_hiwater_set().

Thread B allocates memory in virtio_dmb_inc_used(), increasing both
total_used and used_hiwater (e.g., to 110).

Thread A then blindly overwrites used_hiwater with the stale value (10)
using atomic_long_set().

This seems to contradict the comment above it, leaving used_hiwater
substantially lower than total_used. Is there a way to safely read and
update this to avoid a time-of-check to time-of-use race?

> +
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(virtio_dmb_hiwater_fops, virtio_dmb_hiwater_get,
> +			 virtio_dmb_hiwater_set, "%llu\n");

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11