Re: [PATCH v5] s390/diag204: Add common diag204 interface with caching

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] s390/diag204: diag204_allocate_buffer() does not zero buffer as documented
--

commit 58fe47d5083de8e19767f3f73d70eeda63fea0cc
Author: Mete Durlu <meted@linux.ibm.com>

s390/diag204: Add common diag204 interface with caching

This commit introduces a shared diag204 implementation in
arch/s390/kernel/diag/diag204.c with a one-second data cache and converts
hypfs and sthyi to use it.

> diff --git a/arch/s390/kernel/diag/diag204.c b/arch/s390/kernel/diag/diag204.c
> new file mode 100644
> index 0000000000000..dc4b9df8ba925
> --- /dev/null
> +++ b/arch/s390/kernel/diag/diag204.c

[ ... ]

> +static void *__diag204_allocate_buffer(int pages)
> +{
> +	return __vmalloc_node(array_size(pages, PAGE_SIZE),
> +			      PAGE_SIZE, GFP_KERNEL, NUMA_NO_NODE,
> +			      __builtin_return_address(0));
> +}
> +
> +/**
> + * diag204_allocate_buffer() - Allocate a buffer for diag204
> + *
> + * Allocate a zeroed, page aligned vmalloc'ed buffer of diag204_get_buf_size()
> + * bytes, as required by diag204. The size is determined at initialization
> + * of diag204. Must only be called if diag204_is_available() returns true.
> + *
> + * Context: Can sleep (GFP_KERNEL).
> + * Return: Pointer to the buffer on success, NULL on failure.
> + */
> +void *diag204_allocate_buffer(void)
> +{
> +	return __diag204_allocate_buffer(diag204_cache_info.pages);
> +}

[ ... ]

> +/**
> + * diag204_store() - Store diag204 data
> + * @buf:             Diag204 data buffer allocated using
> + *                   diag204_allocate_buffer() or a page aligned, zeroed buffer
> + *                   with a size acquired from diag204_get_buf_size().
> + * @allow_busy:      Allow diag204 to return -EBUSY instead of blocking and
> + *                   return early when there is a concurrent updater.
> + *
> + * Diag204 queries CPU utilization data from the hypervisor. Per CPU
> + * utilization information is returned either only for the current guest
> + * or, with authorization for all guests configured.
> + *
> + * Context: diag204_store() can sleep and holds diag204_mutex to serialize
> + *          diag204 cache access.
> + *
> + * Return:
> + * * 0 on success
> + * * -EOPNOTSUPP diag204 is not supported
> + * * -EINVAL if @buf is NULL
> + * * -ENOMEM if the temporary buffer could not be allocated
> + * * -EBUSY if @allow_busy is set and diag204 or a concurrent cache update
> + *    is busy
> + * * -EINTR if interrupted by a signal
> + */
> +int diag204_store(void *buf, bool allow_busy)
> +{
> +	int rc;
> +
> +	if (!diag204_is_available())
> +		return -EOPNOTSUPP;
> +	if (!buf)
> +		return -EINVAL;
> +	rc = 0;
> +	if (allow_busy) {
> +		if (!mutex_trylock(&diag204_mutex))
> +			return -EBUSY;
> +	} else {
> +		if (mutex_lock_killable(&diag204_mutex))
> +			return -EINTR;
> +	}
> +	if (diag204_is_cache_valid())
> +		goto copy;
> +	rc = diag204_update_cache(allow_busy);
> +	if (rc)
> +		goto out;
> +copy:
> +	memcpy(buf, diag204_cache_info.buf, diag204_get_buf_size());
> +out:
> +	mutex_unlock(&diag204_mutex);
> +	return rc;
> +}

[Severity: Low]
This isn't a bug for current in-tree callers since diag204_update_cache()
zeroes diag204_cache_info.buf with memset() and diag204_store() overwrites
the caller buffer via memcpy(), but does diag204_allocate_buffer() actually
zero the allocated memory as documented?

In arch/s390/kernel/diag/diag204.c, diag204_allocate_buffer() calls
__diag204_allocate_buffer(), which invokes __vmalloc_node() with GFP_KERNEL
and without __GFP_ZERO, returning uninitialized memory despite the
kernel-doc stating that it allocates a zeroed buffer.

Similarly, the kernel-doc for diag204_store() states that buf must be a
page-aligned, zeroed buffer, even though diag204_store() only copies from
diag204_cache_info.buf into buf via memcpy().

Should __diag204_allocate_buffer() pass GFP_KERNEL | __GFP_ZERO, or should
the kernel-doc comments on diag204_allocate_buffer() and diag204_store() be
updated to match the implementation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-diag204ioctl-v5-1-87a96638d58a@linux.ibm.com?part=1