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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] s390/diag204: premature -EBUSY in diag204_update_cache() when cache is empty
- [Medium] s390/diag204: wrong break check on error in diag204_cache_replace()
- [Medium] s390/diag204: missing subcode 4 fallback in diag204_probe()
- [Medium] s390/diag204: unexpected -ERESTARTSYS in diag204_store() when allow_busy_wait is 0
- [Low] s390/diag204: redundant work rearm after cache free in diag204_work_fn()
--

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

s390/diag204: Introduce diag204 rework

This commit introduces a new diag204 implementation in
arch/s390/kernel/diag/diag204.c that caches diag204 data for up to one
second to rate-limit expensive hypervisor calls and frees the cache buffer
via a delayed work item after a period of inactivity.

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

[ ... ]

> +static void diag204_work_fn(struct work_struct *work)
> +{
> +	mutex_lock(&diag204_mutex);
> +	if (!READ_ONCE(g_cache.buf))
> +		goto unlock;
> +	if (diag204_is_cache_valid(DIAG204_CACHE_EXPIRY))
> +		goto rearm;
> +	if (READ_ONCE(g_cache.updating))
> +		goto rearm;
> +	diag204_set_cache(NULL);
> +rearm:
> +	diag204_schedule_work();
> +unlock:
> +	mutex_unlock(&diag204_mutex);
> +}

[Severity: Low]
In diag204_work_fn(), when the cache has expired and is not being updated,
diag204_set_cache(NULL) frees g_cache.buf and clears the pointer, but then
execution falls through to the rearm label and calls diag204_schedule_work().

Should this jump to unlock after diag204_set_cache(NULL) instead of
scheduling another delayed work run 10 seconds later when the cache is
already empty?

> +
> +static int diag204_cache_new(void *buf, unsigned long subcode)
> +{
> +	int rc;
> +
> +	lockdep_assert_held(&diag204_mutex);
> +	/*
> +	 * Don't allow busy wait or busy looping when g_cache.buf has
> +	 * no data yet. We first need a valid data cache before we
> +	 * can do updates with busy looping.
> +	 */
> +	rc = diag204(subcode, g_info.pages, buf);
> +	if (rc < 0)
> +		return rc;
> +	diag204_set_cache(buf);
> +	return rc;
> +}
> +
> +static int diag204_cache_replace(void *buf, unsigned long subcode, int allow_busy_wait)
> +{
> +	int rc;
> +
> +	lockdep_assert_held(&diag204_mutex);
> +	if (!allow_busy_wait) {
> +		rc = diag204(subcode, g_info.pages, buf);
> +	} else {
> +		/*
> +		 * Unlock before BUSY loop so updater can sleep, we rely on
> +		 * g_cache.updating flag here for synchronising with diag204
> +		 * work function and other diag204 callers.
> +		 */
> +		mutex_unlock(&diag204_mutex);
> +		do {
> +			rc = diag204(subcode, g_info.pages, buf);
> +			if (rc >= 0)
> +				break;
> +			if (signal_pending(current)) {
> +				mutex_lock(&diag204_mutex);
> +				return -ERESTARTSYS;
> +			}
> +			schedule_timeout_interruptible(DIAG204_BUSY_WAIT);
> +		} while (rc == -EBUSY);

[Severity: Medium]
In diag204_cache_replace(), when allow_busy_wait is set, the retry loop only
breaks early on rc >= 0.

If diag204() fails with a negative error other than -EBUSY (such as
-EOPNOTSUPP or -EINVAL), won't this loop either overwrite the error with
-ERESTARTSYS if a signal is pending, or sleep unnecessarily for
DIAG204_BUSY_WAIT before checking the loop condition? Should the break
condition check for rc != -EBUSY instead?

> +		mutex_lock(&diag204_mutex);
> +	}
> +	if (rc < 0)
> +		return rc;
> +	diag204_set_cache(buf);
> +	return rc;
> +}
> +
> +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);
> +	if (!READ_ONCE(g_cache.buf))
> +		rc = diag204_cache_new(buf, subcode);
> +	else
> +		rc = diag204_cache_replace(buf, subcode, allow_busy_wait);

[Severity: Medium]
In diag204_update_cache(), DIAG204_BIF_BIT is set in subcode when
g_info.has_bif && allow_busy is true, before checking whether g_cache.buf is
NULL.

When g_cache.buf is NULL (on the initial call to diag204_store() or after
diag204_work_fn() has freed the expired cache) and a caller passes
allow_busy = 1, diag204_cache_new() is called with DIAG204_BIF_BIT set in
subcode.

The comment in diag204_cache_new() notes that busy waiting or busy looping
should not be used when g_cache.buf has no data yet, and diag204_cache_new()
does not loop on -EBUSY. If the hypervisor is busy, won't diag204() return
-EBUSY immediately instead of blocking to populate the initial cache (or
performing the requested busy wait)?

> +	WRITE_ONCE(g_cache.updating, false);
> +	if (rc < 0)
> +		diag204_free_buffer(buf);
> +	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;
> +	if (allow_busy && !allow_busy_wait) {
> +		mutex_lock(&diag204_mutex);
> +		/*
> +		 * Drop the lock if an update is ongoing, user allows to return
> +		 * BUSY without waiting
> +		 */
> +		if (READ_ONCE(g_cache.updating)) {
> +			mutex_unlock(&diag204_mutex);
> +			diag204_schedule_work();
> +			return -EBUSY;
> +		}
> +	} else {
> +		/* Wait for the ongoing update to end */
> +		while (1) {
> +			mutex_lock(&diag204_mutex);
> +			if (!READ_ONCE(g_cache.updating))
> +				break;
> +			mutex_unlock(&diag204_mutex);
> +			if (signal_pending(current))
> +				return -ERESTARTSYS;
> +			schedule_timeout_interruptible(DIAG204_BUSY_WAIT);
> +		}
> +	}

[Severity: Medium]
Can diag204_store() unexpectedly return -ERESTARTSYS when called with
allow_busy = 0 and allow_busy_wait = 0?

If one thread calls diag204_store() with allow_busy = 1 and
allow_busy_wait = 1 while g_cache.buf is non-NULL and expired,
diag204_update_cache() -> diag204_cache_replace() sets g_cache.updating = true
and drops diag204_mutex while sleeping in its -EBUSY loop.

If a second thread concurrently calls diag204_store(buf, 0, 0), the
allow_busy && !allow_busy_wait check is false, so it enters the else branch
above, sees g_cache.updating == true, drops diag204_mutex, and returns
-ERESTARTSYS if a signal is pending on current, even though the caller did not
request an interruptible wait.

In the follow-up commit "s390/diag204: Replace diag204 users with new
implementation", callers such as hypfs_diag_create_files() (via
hypfs_write_iter() after hypfs_delete_tree() has already removed the hypfs
directory tree, and via hypfs_fill_super()) and diag204_get_data() in
arch/s390/kernel/sthyi.c (via sthyi_fill(), including KVM's handle_sthyi()
intercept handler) call diag204_store() with allow_busy = 0 and
allow_busy_wait = 0 and do not expect -ERESTARTSYS, which can leave the hypfs
tree deleted or abort STHYI emulation after SIE has already advanced the guest
PSW.

[ ... ]

> +/*
> + * diag204_probe() has to find out, which type of diagnose 204 implementation
> + * we have on our machine. Currently there are three possible scanarios:
> + *   - subcode 4   + simple data format (only one page)
> + *   - subcode 4-6 + extended data format
> + *   - subcode 4-7 + extended data format
> + *
> + * Subcode 5 is used to retrieve the size of the data, provided by subcodes
> + * 6 and 7. Subcode 7 basically has the same function as subcode 6. In addition
> + * to subcode 6 it provides also information about secondary cpus.
> + * In order to get as much information as possible, we first try
> + * subcode 7, then 6 and if both fail, we use subcode 4.
> + */
> +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;
> +		}
> +	} else {
> +		/* extended data format is not available, fallback to simple */
> +		pages = 1;
> +		buf = __diag204_allocate_buffer(pages);
> +		if (!buf) {
> +			rc = -ENOMEM;
> +			goto fail_alloc;
> +		}
> +		type = DIAG204_INFO_SIMPLE;
> +		if (diag204(DIAG204_SUBC_STIB4 | type, pages, buf) >= 0) {
> +			subcode = DIAG204_SUBC_STIB4;
> +			goto out;
> +		}
> +	}
> +	rc = -EOPNOTSUPP;
> +	goto fail_store;

[Severity: Medium]
In diag204_init() -> diag204_probe(), the fallback to DIAG204_INFO_SIMPLE with
DIAG204_SUBC_STIB4 is inside the else branch of if (pages > 0).

If the DIAG204_SUBC_RSI call returns pages > 0, but both DIAG204_SUBC_STIB7
and DIAG204_SUBC_STIB6 fail, won't diag204_probe() skip the DIAG204_SUBC_STIB4
fallback and fail with -EOPNOTSUPP, contrary to the function comment and the
previous probe logic in arch/s390/hypfs/hypfs_diag.c?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-diag204ioctl-v1-0-25645b97bf4e@linux.ibm.com?part=1