Re: [PATCH v2 1/3] s390/diag204: Introduce diag204 rework

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] s390/diag204: unlocked cache access and mutex unlock in diag204_store()
- [Medium] s390/diag204: memory leak of extended buffer in diag204_probe()
--

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

s390/diag204: Introduce diag204 rework

This commit introduces a caching mechanism in arch/s390/kernel/diag/diag204.c
to limit diag204 calls to at most once per second across all users. A delayed
work function frees the cached buffer when diag204 data is not fetched within
the expiration window.

> diff --git a/arch/s390/kernel/diag/diag204.c b/arch/s390/kernel/diag/diag204.c
> new file mode 100644
> index 0000000000000..7f366ea12a673
> --- /dev/null
> +++ b/arch/s390/kernel/diag/diag204.c
> @@ -0,0 +1,290 @@

[ ... ]

> +static void diag204_set_cache(void *buf)
> +{
> +	diag204_free_buffer(READ_ONCE(g_cache.buf));
> +	WRITE_ONCE(g_cache.buf, buf);
> +	if (!buf)
> +		WRITE_ONCE(g_cache.timestamp, 0);
> +	else
> +		WRITE_ONCE(g_cache.timestamp, ktime_get());
> +}

[ ... ]

> +static int diag204_update_cache(int allow_busy, int allow_busy_wait)
> +{
> +	unsigned long subcode;
> +	void *buf;
> +	int rc;
> +
> +	lockdep_assert_held(&diag204_mutex);
> +	buf = diag204_allocate_buffer();
> +	if (!buf)
> +		return -ENOMEM;
> +	subcode = g_info.type | g_info.subcode;
> +	if (g_info.has_bif && allow_busy)
> +		subcode |= DIAG204_BIF_BIT;
> +	WRITE_ONCE(g_cache.updating, true);
> +	mutex_unlock(&diag204_mutex);
> +	do {
> +		rc = diag204(subcode, g_info.pages, buf);
> +		if (rc != -EBUSY || !allow_busy_wait)
> +			break;
> +		if (signal_pending(current)) {
> +			rc = -ERESTARTSYS;
> +			break;
> +		}
> +		schedule_timeout_interruptible(DIAG204_BUSY_WAIT);
> +	} while (allow_busy_wait);
> +	mutex_lock(&diag204_mutex);
> +	if (rc >= 0) {
> +		diag204_set_cache(buf);
> +		diag204_schedule_work();
> +	} else {
> +		diag204_free_buffer(buf);
> +	}
> +	WRITE_ONCE(g_cache.updating, false);
> +	return rc;
> +}

[ ... ]

> +int diag204_store(void *buf, int allow_busy, int allow_busy_wait)
> +{
> +	int rc;
> +
> +	if (!diag204_is_available())
> +		return -EOPNOTSUPP;
> +	if (!allow_busy && allow_busy_wait)
> +		return -EOPNOTSUPP;
> +	if (!buf)
> +		return -ENOMEM;
> +	rc = 0;
> +	do {
> +		mutex_lock(&diag204_mutex);
> +		/* Exit loop with lock taken if there is no concurrent update */
> +		if (!READ_ONCE(g_cache.updating))
> +			break;
> +		mutex_unlock(&diag204_mutex);
> +		if (allow_busy && !allow_busy_wait)
> +			return -EBUSY;
> +		if (signal_pending(current))
> +			return -ERESTARTSYS;
> +		schedule_timeout_interruptible(DIAG204_BUSY_WAIT);
> +	} while (allow_busy_wait);
> +	if (!diag204_is_cache_valid(DIAG204_CACHE_EXPIRY))
> +		rc = diag204_update_cache(allow_busy, allow_busy_wait);
> +	if (rc >= 0)
> +		memcpy(buf, READ_ONCE(g_cache.buf), diag204_get_buf_size());
> +	mutex_unlock(&diag204_mutex);
> +	return rc;
> +}

[Severity: High]
In arch/s390/kernel/diag/diag204.c, what happens in diag204_store() when a
caller passes allow_busy == 0 and allow_busy_wait == 0 while another thread
is concurrently updating the cache in diag204_update_cache()?

While the first thread is inside diag204_update_cache(), it sets
g_cache.updating to true and drops diag204_mutex across the diag204() call:

	WRITE_ONCE(g_cache.updating, true);
	mutex_unlock(&diag204_mutex);

If a second thread calls diag204_store(buf, 0, 0) during that window, it
acquires diag204_mutex, sees READ_ONCE(g_cache.updating) is true, and calls
mutex_unlock(&diag204_mutex). Because allow_busy is 0, the -EBUSY return is
not taken. The thread then checks signal_pending(current) (returning
-ERESTARTSYS even though allow_busy_wait is 0) or sleeps in
schedule_timeout_interruptible(DIAG204_BUSY_WAIT).

At the end of the loop:

	} while (allow_busy_wait);

Because allow_busy_wait is 0, the loop terminates without holding
diag204_mutex and without rechecking g_cache.updating.

The second thread then calls diag204_is_cache_valid() and
diag204_update_cache() (both of which assert lockdep_assert_held(&diag204_mutex),
with diag204_update_cache() also calling mutex_unlock(&diag204_mutex)), copies
from g_cache.buf via memcpy() while diag204_set_cache() or diag204_work_fn()
may concurrently free or clear g_cache.buf via vfree(), and finally calls
mutex_unlock(&diag204_mutex) at the end of diag204_store() on an unheld or
other-thread-held mutex.

Should the loop in diag204_store() keep waiting until g_cache.updating is
false when allow_busy is 0, or how should non-busy callers synchronize with
an in-progress update?

> +static int diag204_probe(void)
> +{
> +	unsigned long subcode, type;
> +	int pages, rc;
> +	void *buf;
> +
> +	pages = diag204((unsigned long)DIAG204_INFO_EXT |
> +			(unsigned long)DIAG204_SUBC_RSI, 0, NULL);
> +	if (pages > 0) {
> +		buf = __diag204_allocate_buffer(pages);
> +		if (!buf) {
> +			rc = -ENOMEM;
> +			goto fail_alloc;
> +		}
> +		type = DIAG204_INFO_EXT;
> +		if (diag204(DIAG204_SUBC_STIB7 | type, pages, buf) >= 0) {
> +			subcode = DIAG204_SUBC_STIB7;
> +			goto out;
> +		}
> +		if (diag204(DIAG204_SUBC_STIB6 | type, pages, buf) >= 0) {
> +			subcode = DIAG204_SUBC_STIB6;
> +			goto out;
> +		}
> +	}
> +	/* extended data format is not available, fallback to simple */
> +	pages = 1;
> +	buf = __diag204_allocate_buffer(pages);

[Severity: Medium]
In diag204_probe() in arch/s390/kernel/diag/diag204.c, if the
DIAG204_SUBC_RSI call returns pages > 0 and __diag204_allocate_buffer(pages)
succeeds, but both the DIAG204_SUBC_STIB7 and DIAG204_SUBC_STIB6 calls fail,
does falling through to the simple format fallback overwrite buf without
calling vfree(buf) first and leak the extended vmalloc buffer?

[ ... ]

> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-diag204ioctl-v2-0-7e5a9d1cd21a@linux.ibm.com?part=1