Re: [PATCH v3 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: diag204_store() rejects allow_busy when BIF is unsupported
- [Medium] s390/diag204: diag204_store() returns -ERESTARTSYS when allow_busy_wait is 0
--

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

s390/diag204: Introduce diag204 rework

This commit introduces a caching layer for diagnose 204 calls in
arch/s390/kernel/diag/diag204.c to limit hardware invocations to at most
once per second, freeing the cache buffer via delayed work when idle.

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

[ ... ]

> +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;

[ ... ]

> +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 (allow_busy && !g_info.has_bif)
> +		return -EINVAL;

[Severity: High]
Should diag204_store() in arch/s390/kernel/diag/diag204.c return -EINVAL when
allow_busy is set and g_info.has_bif is false, or should it fall back to a
regular blocking diagnose 204 call?

In diag204_update_cache(), the code checks both g_info.has_bif and allow_busy
before setting DIAG204_BIF_BIT:

	subcode = g_info.type | g_info.subcode;
	if (g_info.has_bif && allow_busy)
		subcode |= DIAG204_BIF_BIT;

Because diag204_update_cache() is only called from diag204_store(), the early
return of -EINVAL in diag204_store() when allow_busy && !g_info.has_bif makes
the g_info.has_bif check in diag204_update_cache() unreachable when
!g_info.has_bif.

When the follow-up change ("s390/diag204: Replace diag204 users with new
implementation") converts existing callers:

- dbfs_d204_create() in arch/s390/hypfs/hypfs_diag.c and
  hypfs_diag_create_files() in arch/s390/hypfs/hypfs_diag_fs.c both call
  diag204_store(..., 1, 1) unconditionally.
- diag204_get_data() in arch/s390/kernel/sthyi.c calls
  diag204_store(diag204_buf, diag204_allow_busy, 0), where
  diag204_allow_busy is true on every cache refresh after the initial fill.

On systems where !sclp.has_diag204_bif, will this cause hypfs initialization,
debugfs diag_204 reads, and sthyi cache updates to fail with -EINVAL instead
of falling back to a non-busy-indication diag204 call?

> +	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 (1);

[Severity: Medium]
Can this wait loop in diag204_store() in arch/s390/kernel/diag/diag204.c
unexpectedly sleep interruptibly and return -ERESTARTSYS when the caller
passes allow_busy = 0 and allow_busy_wait = 0?

If one thread is inside diag204_update_cache() with g_cache.updating set to
true while dropping diag204_mutex, a second thread calling
diag204_store(buf, 0, 0) will see READ_ONCE(g_cache.updating) as true:

	if (allow_busy && !allow_busy_wait)
		return -EBUSY;
	if (signal_pending(current))
		return -ERESTARTSYS;
	schedule_timeout_interruptible(DIAG204_BUSY_WAIT);

Because allow_busy is 0, the first condition is false and the loop checks
signal_pending(current) and returns -ERESTARTSYS even though allow_busy_wait
was not requested.

When the follow-up commits ("s390/diag204: Replace diag204 users with new
implementation" and "s390/diag: Add diag204 IOCTL to diag misc device") are
applied, callers such as diag204_buf() or the guest sthyi path:

handle_sthyi()
  -> sthyi_fill()
    -> sthyi_update_cache()
      -> fill_dst()
        -> diag204_get_data(false)
          -> diag204_store(diag204_buf, 0, 0)

can hit this path when a signal or TIF_NOTIFY_SIGNAL is pending during a
concurrent cache update. In handle_sthyi(), an unexpected -ERESTARTSYS return
from sthyi_fill() when the sthyi cache is not yet populated causes
handle_sthyi() to return before filling the guest buffer or setting the guest
condition code after the guest instruction pointer has already been advanced.

Should the concurrent update wait in diag204_store() avoid interruptible
sleep and -ERESTARTSYS when !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;
> +}

[ ... ]

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