Re: [PATCH v4] 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: unexpected signal interruption and -EBUSY in diag204_store()
- [Medium] s390/diag204: misplaced signal_pending() check in diag204_update_cache()
--

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

s390/diag204: Introduce diag204 rework

This commit reworks diag204 support to rate-limit hypervisor calls to at
most once per second using a shared cache, delayed work for buffer cleanup,
and a common kernel interface. Existing in-kernel users in hypfs and sthyi
are updated to use the new interface.

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

[ ... ]

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

[Severity: Medium]
When diag204_update_cache() is called with allow_busy_wait set (for example,
via hypfs_diag_create_files() or dbfs_d204_create()) and diag204() returns
-EBUSY, a signal arriving while the task is sleeping in
schedule_timeout_interruptible(HZ) wakes the task early.

Because the loop immediately starts the next iteration and calls diag204()
before checking signal_pending(current) again, doesn't a signal during the
sleep trigger another expensive diag204() call immediately without waiting
for the 1-second interval, and potentially miss returning -ERESTARTSYS if the
second diag204() call succeeds?

> +	mutex_lock(&diag204_mutex);
> +	if (rc >= 0) {
> +		diag204_replace_cache(buf);
> +		mod_delayed_work(system_dfl_wq, &diag204_work, DIAG204_WORK_DELAY);
> +	} 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;
> +	while (1) {
> +		rc = mutex_lock_interruptible(&diag204_mutex);
> +		if (rc)
> +			return rc;
> +		/* 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;
> +		/* Update in progress, wait until flag is cleared */
> +		rc = wait_event_interruptible(g_cache.wq, !READ_ONCE(g_cache.updating));
> +		if (rc)
> +			return rc;
> +	}

[Severity: High]
Can this loop in diag204_store() return unexpected -EINTR, -ERESTARTSYS, or
-EBUSY errors to callers that pass allow_busy_wait as 0?

Specifically, sthyi_fill() -> sthyi_update_cache() -> fill_dst() ->
diag204_get_data() calls diag204_store(diag204_buf, diag204_allow_busy, 0):

arch/s390/kernel/sthyi.c:diag204_get_data() {
	...
	rc = diag204_store(diag204_buf, diag204_allow_busy, 0);
	if (rc < 0) {
		diag204_free_buffer(diag204_buf);
		return ERR_PTR(rc);
	}
	...
}

Even though allow_busy_wait is 0, diag204_store() unconditionally uses
mutex_lock_interruptible() and wait_event_interruptible(), which return
-EINTR or -ERESTARTSYS if a signal or notify_signal is pending while
contending on diag204_mutex or waiting for g_cache.updating.

When sthyi_fill() is invoked from KVM's SIE intercept handler in
arch/s390/kvm/s390/intercept.c:handle_sthyi():

	cc = sthyi_fill(sctns, &rc);
	if (cc < 0)
		return cc;

and arch/s390/kvm/s390/s390.c:kvm_arch_vcpu_ioctl_run():

	rc = __vcpu_run(vcpu);

	if (signal_pending(current) && !rc) {
		kvm_run->exit_reason = KVM_EXIT_INTR;
		vcpu->stat.signal_exits++;
		rc = -EINTR;
	}

if a signal is pending on the vCPU thread during a concurrent diag204 cache
update, sthyi_fill() fails with -EINTR or -ERESTARTSYS after SIE has already
advanced the guest PSW past the STHYI instruction, leaving the guest buffer
and condition code unupdated and preventing kvm_arch_vcpu_ioctl_run() from
setting KVM_EXIT_INTR.

Also, when g_cache.updating is true and allow_busy && !allow_busy_wait holds,
diag204_store() returns -EBUSY without checking g_cache.has_bif, whereas
diag204_update_cache() checks g_cache.has_bif && allow_busy. Should the
concurrent update check also gate -EBUSY on g_cache.has_bif, and use
uninterruptible locking and waiting when allow_busy_wait is 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-diag204ioctl-v4-1-d5e90b56112b@linux.ibm.com?part=1