* [PATCH v2 1/3] s390/diag204: Introduce diag204 rework
2026-10-22 11:49 [PATCH v2 0/3] s390: Diag204 Rework Mete Durlu
@ 2026-10-22 11:49 ` Mete Durlu
2026-10-22 12:05 ` sashiko-bot
2026-10-22 11:49 ` [PATCH v2 2/3] s390/diag204: Replace diag204 users with new implementation Mete Durlu
2026-10-22 11:49 ` [PATCH v2 3/3] s390/diag: Add diag204 IOCTL to diag misc device Mete Durlu
2 siblings, 1 reply; 7+ messages in thread
From: Mete Durlu @ 2026-10-22 11:49 UTC (permalink / raw)
To: Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, Sumanth Korikkar
Cc: Ilya Leoshkevich, linux-s390, Mete Durlu
diag204 is an expensive call, limit its invocation to once per
second at most for all users. Use a caching mechanism to provide diag204
data at demand. Cache buffer gets freed by a delayed work function if
diag204 data is not fetched after a certain amount of time.
Signed-off-by: Mete Durlu <meted@linux.ibm.com>
---
arch/s390/include/asm/diag.h | 7 +
arch/s390/kernel/diag/diag204.c | 290 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 297 insertions(+)
diff --git a/arch/s390/include/asm/diag.h b/arch/s390/include/asm/diag.h
index 1416ee1ad323..8669cfa039d3 100644
--- a/arch/s390/include/asm/diag.h
+++ b/arch/s390/include/asm/diag.h
@@ -336,6 +336,13 @@ static inline bool diag204_has_bif(void)
return sclp.has_diag204_bif;
}
+int diag204_store(void *buf, int allow_busy, int allow_busy_wait);
+enum diag204_format diag204_get_info_type(void);
+enum diag204_sc diag204_get_subcode(void);
+void *diag204_allocate_buffer(void);
+void diag204_free_buffer(void *buf);
+bool diag204_is_available(void);
+int diag204_get_buf_size(void);
int diag204(unsigned long subcode, unsigned long size, void *addr);
int diag224(void *ptr);
int diag26c(void *req, void *resp, enum diag26c_sc subcode);
diff --git a/arch/s390/kernel/diag/diag204.c b/arch/s390/kernel/diag/diag204.c
new file mode 100644
index 000000000000..7f366ea12a67
--- /dev/null
+++ b/arch/s390/kernel/diag/diag204.c
@@ -0,0 +1,290 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Request CPU utilization information from underlying diag 0x204
+ * facility. Diag 0x204 provides a variety of utilization values.
+ *
+ * Copyright IBM Corp. 2026
+ */
+
+#define pr_fmt(fmt) "diag204: " fmt
+
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/timer.h>
+#include <linux/vmalloc.h>
+
+#include <asm/diag.h>
+
+struct diag204_info {
+ int pages; /* number of pages for diag204 data */
+ enum diag204_format type; /* used diag204 data format */
+ enum diag204_sc subcode; /* used diag204 subcode for store */
+ bool has_bif;
+};
+
+struct diag204_cache {
+ ktime_t timestamp;
+ void *buf; /* page aligned diag204 buffer */
+ bool updating; /* ongoing cache update indicator */
+};
+
+static struct diag204_cache g_cache;
+static struct diag204_info g_info;
+static DEFINE_MUTEX(diag204_mutex);
+
+#define DIAG204_CACHE_EXPIRY (NSEC_PER_SEC)
+#define DIAG204_WORK_DELAY (HZ * 10)
+#define DIAG204_BUSY_WAIT (HZ / 10)
+
+static void diag204_work_fn(struct work_struct *work);
+static DECLARE_DELAYED_WORK(diag204_work, diag204_work_fn);
+
+bool diag204_is_available(void)
+{
+ return g_info.pages > 0;
+}
+
+enum diag204_format diag204_get_info_type(void)
+{
+ return g_info.type;
+}
+
+enum diag204_sc diag204_get_subcode(void)
+{
+ return g_info.subcode;
+}
+
+int diag204_get_buf_size(void)
+{
+ return g_info.pages * PAGE_SIZE;
+}
+
+void diag204_free_buffer(void *buf)
+{
+ if (!buf)
+ return;
+ vfree(buf);
+}
+
+/*
+ * If we use subcode 6 or 7 with extended data format, we can (and should) use
+ * vmalloc, since we need a lot of memory in that case. Currently
+ * up to 423 pages!
+ */
+static void *__diag204_allocate_buffer(int pages)
+{
+ void *buf;
+
+ buf = __vmalloc_node(array_size(pages, PAGE_SIZE), PAGE_SIZE,
+ GFP_KERNEL | __GFP_ZERO, NUMA_NO_NODE,
+ __builtin_return_address(0));
+ return buf;
+}
+
+void *diag204_allocate_buffer(void)
+{
+ return __diag204_allocate_buffer(g_info.pages);
+}
+
+static void diag204_schedule_work(void)
+{
+ mod_delayed_work(system_dfl_wq, &diag204_work, DIAG204_WORK_DELAY);
+}
+
+static int diag204_is_cache_valid(unsigned long duration_ns)
+{
+ ktime_t expire;
+
+ lockdep_assert_held(&diag204_mutex);
+ if (!READ_ONCE(g_cache.buf))
+ return 0;
+ expire = ktime_add_ns(READ_ONCE(g_cache.timestamp), duration_ns);
+ if (ktime_after(ktime_get(), expire))
+ return 0;
+ return 1;
+}
+
+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 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);
+ goto unlock;
+rearm:
+ diag204_schedule_work();
+unlock:
+ mutex_unlock(&diag204_mutex);
+}
+
+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;
+}
+
+/*
+ * diag204_store() provides users data retrieved from the diag204 facility.
+ * Since diag204 is an expensive call to make, it is limited to a call per
+ * second for now. To circumvent this limitation a caching mechanism is used.
+ *
+ * diag204_store() first checks if diag204 cache is present and valid. If the
+ * cache is valid, it copies the contents to the users buffer.
+ * If the cache is expired, it first updates the cache and then proceeds with
+ * the same copy operation.
+ *
+ * @buf: Users have to manage their own buffers by using
+ * diag204_allocate_buffer() and diag204_free_buffer().
+ *
+ * @allow_busy: If diag204 supports busy indication facility(bif), users can
+ * specify diag204 to utilize it and return BUSY instead of blocking until
+ * data becomes ready.
+ *
+ * @allow_busy_wait: If users allow diag204 to use bif, they would also get
+ * the option to specify an interruptible wait behavior by allowing busy wait.
+ * This is useful since it gives users to interrupt diag204 waiting period
+ * via signals.
+ */
+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;
+}
+
+/*
+ * 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;
+ }
+ }
+ /* 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;
+out:
+ rc = 0;
+ g_info.pages = pages;
+ g_info.subcode = subcode;
+ g_info.type = type;
+ g_info.has_bif = diag204_has_bif();
+fail_store:
+ vfree(buf);
+fail_alloc:
+ return rc;
+}
+
+static int __init diag204_init(void)
+{
+ return diag204_probe();
+}
+device_initcall(diag204_init);
--
2.56.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 1/3] s390/diag204: Introduce diag204 rework
2026-10-22 11:49 ` [PATCH v2 1/3] s390/diag204: Introduce diag204 rework Mete Durlu
@ 2026-10-22 12:05 ` sashiko-bot
0 siblings, 0 replies; 7+ messages in thread
From: sashiko-bot @ 2026-10-22 12:05 UTC (permalink / raw)
To: Mete Durlu
Cc: linux-s390, Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/3] s390/diag204: Replace diag204 users with new implementation
2026-10-22 11:49 [PATCH v2 0/3] s390: Diag204 Rework Mete Durlu
2026-10-22 11:49 ` [PATCH v2 1/3] s390/diag204: Introduce diag204 rework Mete Durlu
@ 2026-10-22 11:49 ` Mete Durlu
2026-10-22 12:02 ` sashiko-bot
2026-10-22 11:49 ` [PATCH v2 3/3] s390/diag: Add diag204 IOCTL to diag misc device Mete Durlu
2 siblings, 1 reply; 7+ messages in thread
From: Mete Durlu @ 2026-10-22 11:49 UTC (permalink / raw)
To: Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, Sumanth Korikkar
Cc: Ilya Leoshkevich, linux-s390, Mete Durlu
Replace the existing diag204 users in kernelspace with the new diag204
interface.
Signed-off-by: Mete Durlu <meted@linux.ibm.com>
---
arch/s390/hypfs/hypfs_diag.c | 140 ++++------------------------------------
arch/s390/hypfs/hypfs_diag.h | 4 +-
arch/s390/hypfs/hypfs_diag_fs.c | 9 ++-
arch/s390/include/asm/diag.h | 1 -
arch/s390/kernel/diag/Makefile | 2 +-
arch/s390/kernel/sthyi.c | 30 +++------
6 files changed, 29 insertions(+), 157 deletions(-)
diff --git a/arch/s390/hypfs/hypfs_diag.c b/arch/s390/hypfs/hypfs_diag.c
index 08777f57bd24..c3eebe4667fa 100644
--- a/arch/s390/hypfs/hypfs_diag.c
+++ b/arch/s390/hypfs/hypfs_diag.c
@@ -22,139 +22,24 @@
#define DBFS_D204_HDR_VERSION 0
-static enum diag204_sc diag204_store_sc; /* used subcode for store */
-static enum diag204_format diag204_info_type; /* used diag 204 data format */
-
static void *diag204_buf; /* 4K aligned buffer for diag204 data */
-static int diag204_buf_pages; /* number of pages for diag204 data */
-
-enum diag204_format diag204_get_info_type(void)
-{
- return diag204_info_type;
-}
-
-static void diag204_set_info_type(enum diag204_format type)
-{
- diag204_info_type = type;
-}
-
-/* Diagnose 204 functions */
-/*
- * For the old diag subcode 4 with simple data format we have to use real
- * memory. If we use subcode 6 or 7 with extended data format, we can (and
- * should) use vmalloc, since we need a lot of memory in that case. Currently
- * up to 93 pages!
- */
-static void diag204_free_buffer(void)
+static void hypfs_diag204_free_buffer(void)
{
- vfree(diag204_buf);
+ diag204_free_buffer(diag204_buf);
diag204_buf = NULL;
}
-void *diag204_get_buffer(enum diag204_format fmt, int *pages)
+void *hypfs_diag204_get_buffer(void)
{
- if (diag204_buf) {
- *pages = diag204_buf_pages;
+ if (diag204_buf)
return diag204_buf;
- }
- if (fmt == DIAG204_INFO_SIMPLE) {
- *pages = 1;
- } else {/* DIAG204_INFO_EXT */
- *pages = diag204((unsigned long)DIAG204_SUBC_RSI |
- (unsigned long)DIAG204_INFO_EXT, 0, NULL);
- if (*pages <= 0)
- return ERR_PTR(-EOPNOTSUPP);
- }
- diag204_buf = __vmalloc_node(array_size(*pages, PAGE_SIZE),
- PAGE_SIZE, GFP_KERNEL, NUMA_NO_NODE,
- __builtin_return_address(0));
+ diag204_buf = diag204_allocate_buffer();
if (!diag204_buf)
return ERR_PTR(-ENOMEM);
- diag204_buf_pages = *pages;
return diag204_buf;
}
-/*
- * 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)
-{
- void *buf;
- int pages, rc;
-
- buf = diag204_get_buffer(DIAG204_INFO_EXT, &pages);
- if (!IS_ERR(buf)) {
- if (diag204((unsigned long)DIAG204_SUBC_STIB7 |
- (unsigned long)DIAG204_INFO_EXT, pages, buf) >= 0) {
- diag204_store_sc = DIAG204_SUBC_STIB7;
- diag204_set_info_type(DIAG204_INFO_EXT);
- goto out;
- }
- if (diag204((unsigned long)DIAG204_SUBC_STIB6 |
- (unsigned long)DIAG204_INFO_EXT, pages, buf) >= 0) {
- diag204_store_sc = DIAG204_SUBC_STIB6;
- diag204_set_info_type(DIAG204_INFO_EXT);
- goto out;
- }
- diag204_free_buffer();
- }
-
- /* subcodes 6 and 7 failed, now try subcode 4 */
-
- buf = diag204_get_buffer(DIAG204_INFO_SIMPLE, &pages);
- if (IS_ERR(buf)) {
- rc = PTR_ERR(buf);
- goto fail_alloc;
- }
- if (diag204((unsigned long)DIAG204_SUBC_STIB4 |
- (unsigned long)DIAG204_INFO_SIMPLE, pages, buf) >= 0) {
- diag204_store_sc = DIAG204_SUBC_STIB4;
- diag204_set_info_type(DIAG204_INFO_SIMPLE);
- goto out;
- } else {
- rc = -EOPNOTSUPP;
- goto fail_store;
- }
-out:
- rc = 0;
-fail_store:
- diag204_free_buffer();
-fail_alloc:
- return rc;
-}
-
-int diag204_store(void *buf, int pages)
-{
- unsigned long subcode;
- int rc;
-
- subcode = diag204_get_info_type();
- subcode |= diag204_store_sc;
- if (diag204_has_bif())
- subcode |= DIAG204_BIF_BIT;
- while (1) {
- rc = diag204(subcode, pages, buf);
- if (rc != -EBUSY)
- break;
- if (signal_pending(current))
- return -ERESTARTSYS;
- schedule_timeout_interruptible(DIAG204_BUSY_WAIT);
- }
- return rc < 0 ? rc : 0;
-}
-
struct dbfs_d204_hdr {
u64 len; /* Length of d204 buffer without header */
u16 version; /* Version of header */
@@ -173,19 +58,20 @@ static int dbfs_d204_create(void **data, void **data_free_ptr, size_t *size)
int rc, buf_size;
void *base;
- buf_size = PAGE_SIZE * (diag204_buf_pages + 1) + sizeof(d204->hdr);
+ buf_size = diag204_get_buf_size();
+ buf_size += PAGE_SIZE + sizeof(d204->hdr);
base = vzalloc(buf_size);
if (!base)
return -ENOMEM;
d204 = PTR_ALIGN(base + sizeof(d204->hdr), PAGE_SIZE) - sizeof(d204->hdr);
- rc = diag204_store(d204->buf, diag204_buf_pages);
- if (rc) {
+ rc = diag204_store(d204->buf, 1, 1);
+ if (rc < 0) {
vfree(base);
return rc;
}
d204->hdr.version = DBFS_D204_HDR_VERSION;
- d204->hdr.len = PAGE_SIZE * diag204_buf_pages;
- d204->hdr.sc = diag204_store_sc;
+ d204->hdr.len = diag204_get_buf_size();
+ d204->hdr.sc = diag204_get_subcode();
*data = d204;
*data_free_ptr = base;
*size = d204->hdr.len + sizeof(struct dbfs_d204_hdr);
@@ -202,7 +88,7 @@ __init int hypfs_diag_init(void)
{
int rc;
- if (diag204_probe()) {
+ if (!diag204_is_available()) {
pr_info("The hardware system does not support hypfs\n");
return -ENODATA;
}
@@ -219,6 +105,6 @@ __init int hypfs_diag_init(void)
void hypfs_diag_exit(void)
{
hypfs_diag_fs_exit();
- diag204_free_buffer();
+ hypfs_diag204_free_buffer();
hypfs_dbfs_remove_file(&dbfs_file_d204);
}
diff --git a/arch/s390/hypfs/hypfs_diag.h b/arch/s390/hypfs/hypfs_diag.h
index b5218135b8fe..6b5bea39b6f9 100644
--- a/arch/s390/hypfs/hypfs_diag.h
+++ b/arch/s390/hypfs/hypfs_diag.h
@@ -12,9 +12,7 @@
#include <asm/diag.h>
-enum diag204_format diag204_get_info_type(void);
-void *diag204_get_buffer(enum diag204_format fmt, int *pages);
-int diag204_store(void *buf, int pages);
+void *hypfs_diag204_get_buffer(void);
int __hypfs_diag_fs_init(void);
void __hypfs_diag_fs_exit(void);
diff --git a/arch/s390/hypfs/hypfs_diag_fs.c b/arch/s390/hypfs/hypfs_diag_fs.c
index 39621c326010..13b5a2a4729d 100644
--- a/arch/s390/hypfs/hypfs_diag_fs.c
+++ b/arch/s390/hypfs/hypfs_diag_fs.c
@@ -303,15 +303,14 @@ int hypfs_diag_create_files(struct dentry *root)
struct dentry *systems_dir, *hyp_dir;
void *time_hdr, *part_hdr;
void *buffer, *ptr;
- int i, rc, pages;
+ int i, rc;
- buffer = diag204_get_buffer(diag204_get_info_type(), &pages);
+ buffer = hypfs_diag204_get_buffer();
if (IS_ERR(buffer))
return PTR_ERR(buffer);
- rc = diag204_store(buffer, pages);
- if (rc)
+ rc = diag204_store(buffer, 0, 0);
+ if (rc < 0)
return rc;
-
systems_dir = hypfs_mkdir(root, "systems");
if (IS_ERR(systems_dir))
return PTR_ERR(systems_dir);
diff --git a/arch/s390/include/asm/diag.h b/arch/s390/include/asm/diag.h
index 8669cfa039d3..ca78ca45a8a2 100644
--- a/arch/s390/include/asm/diag.h
+++ b/arch/s390/include/asm/diag.h
@@ -121,7 +121,6 @@ enum diag204_sc {
#define DIAG204_SUBCODE_MASK 0xffff
#define DIAG204_BIF_BIT 0x80000000
-#define DIAG204_BUSY_WAIT (HZ / 10)
/* The two available diag 204 data formats */
enum diag204_format {
diff --git a/arch/s390/kernel/diag/Makefile b/arch/s390/kernel/diag/Makefile
index 956aee6c4090..6e255b66be44 100644
--- a/arch/s390/kernel/diag/Makefile
+++ b/arch/s390/kernel/diag/Makefile
@@ -1 +1 @@
-obj-y := diag_misc.o diag324.o diag.o diag310.o
+obj-y := diag_misc.o diag324.o diag.o diag310.o diag204.o
diff --git a/arch/s390/kernel/sthyi.c b/arch/s390/kernel/sthyi.c
index 5eae2e25997a..9adb26588d5b 100644
--- a/arch/s390/kernel/sthyi.c
+++ b/arch/s390/kernel/sthyi.c
@@ -305,29 +305,19 @@ static struct diag204_x_part_block *lpar_cpu_inf(struct lpar_cpu_inf *part_inf,
static void *diag204_get_data(bool diag204_allow_busy)
{
- unsigned long subcode;
void *diag204_buf;
- int pages, rc;
-
- subcode = DIAG204_SUBC_RSI;
- subcode |= DIAG204_INFO_EXT;
- pages = diag204(subcode, 0, NULL);
- if (pages < 0)
- return ERR_PTR(pages);
- if (pages == 0)
- return ERR_PTR(-ENODATA);
- diag204_buf = __vmalloc_node(array_size(pages, PAGE_SIZE),
- PAGE_SIZE, GFP_KERNEL, NUMA_NO_NODE,
- __builtin_return_address(0));
+ int rc;
+
+ if (!diag204_is_available())
+ return ERR_PTR(-EOPNOTSUPP);
+ if (diag204_get_subcode() != DIAG204_SUBC_STIB7)
+ return ERR_PTR(-EOPNOTSUPP);
+ diag204_buf = diag204_allocate_buffer();
if (!diag204_buf)
return ERR_PTR(-ENOMEM);
- subcode = DIAG204_SUBC_STIB7;
- subcode |= DIAG204_INFO_EXT;
- if (diag204_has_bif() && diag204_allow_busy)
- subcode |= DIAG204_BIF_BIT;
- rc = diag204(subcode, pages, diag204_buf);
+ rc = diag204_store(diag204_buf, diag204_allow_busy, 0);
if (rc < 0) {
- vfree(diag204_buf);
+ diag204_free_buffer(diag204_buf);
return ERR_PTR(rc);
}
return diag204_buf;
@@ -462,7 +452,7 @@ static int fill_dst(void *dst, u64 *rc)
fill_hdr(sctns);
fill_stsi(sctns);
fill_diag(sctns, diag204_buf);
- vfree(diag204_buf);
+ diag204_free_buffer(diag204_buf);
return 0;
}
--
2.56.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 2/3] s390/diag204: Replace diag204 users with new implementation
2026-10-22 11:49 ` [PATCH v2 2/3] s390/diag204: Replace diag204 users with new implementation Mete Durlu
@ 2026-10-22 12:02 ` sashiko-bot
0 siblings, 0 replies; 7+ messages in thread
From: sashiko-bot @ 2026-10-22 12:02 UTC (permalink / raw)
To: Mete Durlu
Cc: linux-s390, Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] s390/diag204: unlocked cache access and unlock imbalance in diag204_store()
- [Medium] s390/diag204: memory leak in diag204_probe() on simple format fallback
--
commit 5ab79951b16e6e4f69656f47d09396782867bf4f
Author: Mete Durlu <meted@linux.ibm.com>
s390/diag204: Replace diag204 users with new implementation
This commit converts the existing kernelspace diag204 users in hypfs and
sthyi to use the new diag204 interface and enables building
arch/s390/kernel/diag/diag204.c.
> diff --git a/arch/s390/hypfs/hypfs_diag.c b/arch/s390/hypfs/hypfs_diag.c
> index 08777f57bd244..c3eebe4667faf 100644
> --- a/arch/s390/hypfs/hypfs_diag.c
> +++ b/arch/s390/hypfs/hypfs_diag.c
> @@ -22,139 +22,24 @@
[ ... ]
> -static int diag204_probe(void)
> -{
> - void *buf;
> - int pages, rc;
> -
> - buf = diag204_get_buffer(DIAG204_INFO_EXT, &pages);
> - if (!IS_ERR(buf)) {
> - if (diag204((unsigned long)DIAG204_SUBC_STIB7 |
> - (unsigned long)DIAG204_INFO_EXT, pages, buf) >= 0) {
> - diag204_store_sc = DIAG204_SUBC_STIB7;
> - diag204_set_info_type(DIAG204_INFO_EXT);
> - goto out;
> - }
> - if (diag204((unsigned long)DIAG204_SUBC_STIB6 |
> - (unsigned long)DIAG204_INFO_EXT, pages, buf) >= 0) {
> - diag204_store_sc = DIAG204_SUBC_STIB6;
> - diag204_set_info_type(DIAG204_INFO_EXT);
> - goto out;
> - }
> - diag204_free_buffer();
> - }
[Severity: Medium]
Does the replacement diag204_probe() in arch/s390/kernel/diag/diag204.c (now
built via arch/s390/kernel/diag/Makefile) leak the extended format vmalloc
buffer buf when falling back to DIAG204_INFO_SIMPLE?
In the removed hypfs implementation above, diag204_free_buffer() freed the
DIAG204_INFO_EXT buffer if both DIAG204_SUBC_STIB7 and DIAG204_SUBC_STIB6
failed.
In arch/s390/kernel/diag/diag204.c:diag204_probe(), if DIAG204_SUBC_RSI
returns pages > 0 and __diag204_allocate_buffer(pages) succeeds, but both
DIAG204_SUBC_STIB7 and DIAG204_SUBC_STIB6 return a negative error code,
execution falls through to the simple format fallback and overwrites buf
without freeing the first allocation:
arch/s390/kernel/diag/diag204.c:diag204_probe() {
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);
}
Should vfree(buf) (or diag204_free_buffer(buf)) be called before overwriting
buf in the fallback path?
[ ... ]
> diff --git a/arch/s390/hypfs/hypfs_diag_fs.c b/arch/s390/hypfs/hypfs_diag_fs.c
> index 39621c3260103..13b5a2a4729d1 100644
> --- a/arch/s390/hypfs/hypfs_diag_fs.c
> +++ b/arch/s390/hypfs/hypfs_diag_fs.c
> @@ -303,15 +303,14 @@ int hypfs_diag_create_files(struct dentry *root)
> struct dentry *systems_dir, *hyp_dir;
> void *time_hdr, *part_hdr;
> void *buffer, *ptr;
> - int i, rc, pages;
> + int i, rc;
>
> - buffer = diag204_get_buffer(diag204_get_info_type(), &pages);
> + buffer = hypfs_diag204_get_buffer();
> if (IS_ERR(buffer))
> return PTR_ERR(buffer);
> - rc = diag204_store(buffer, pages);
> - if (rc)
> + rc = diag204_store(buffer, 0, 0);
> + if (rc < 0)
> return rc;
> -
> systems_dir = hypfs_mkdir(root, "systems");
> if (IS_ERR(systems_dir))
> return PTR_ERR(systems_dir);
[ ... ]
> diff --git a/arch/s390/kernel/sthyi.c b/arch/s390/kernel/sthyi.c
> index 5eae2e25997ae..9adb26588d5b5 100644
> --- a/arch/s390/kernel/sthyi.c
> +++ b/arch/s390/kernel/sthyi.c
> @@ -305,29 +305,19 @@ static struct diag204_x_part_block *lpar_cpu_inf(struct lpar_cpu_inf *part_inf,
>
> static void *diag204_get_data(bool diag204_allow_busy)
> {
> - unsigned long subcode;
> void *diag204_buf;
> - int pages, rc;
> -
> - subcode = DIAG204_SUBC_RSI;
> - subcode |= DIAG204_INFO_EXT;
> - pages = diag204(subcode, 0, NULL);
> - if (pages < 0)
> - return ERR_PTR(pages);
> - if (pages == 0)
> - return ERR_PTR(-ENODATA);
> - diag204_buf = __vmalloc_node(array_size(pages, PAGE_SIZE),
> - PAGE_SIZE, GFP_KERNEL, NUMA_NO_NODE,
> - __builtin_return_address(0));
> + int rc;
> +
> + if (!diag204_is_available())
> + return ERR_PTR(-EOPNOTSUPP);
> + if (diag204_get_subcode() != DIAG204_SUBC_STIB7)
> + return ERR_PTR(-EOPNOTSUPP);
> + diag204_buf = diag204_allocate_buffer();
> if (!diag204_buf)
> return ERR_PTR(-ENOMEM);
> - subcode = DIAG204_SUBC_STIB7;
> - subcode |= DIAG204_INFO_EXT;
> - if (diag204_has_bif() && diag204_allow_busy)
> - subcode |= DIAG204_BIF_BIT;
> - rc = diag204(subcode, pages, diag204_buf);
> + rc = diag204_store(diag204_buf, diag204_allow_busy, 0);
> if (rc < 0) {
> - vfree(diag204_buf);
> + diag204_free_buffer(diag204_buf);
> return ERR_PTR(rc);
> }
> return diag204_buf;
[Severity: Critical]
Can calling diag204_store() with allow_busy=0 and allow_busy_wait=0 from
hypfs_diag_create_files() or fill_dst()->diag204_get_data() (when
is_diag204_cached(sctns) is false) cause unlocked cache access and an
unbalanced mutex_unlock(&diag204_mutex)?
In arch/s390/kernel/diag/diag204.c:diag204_update_cache(), the thread
updating the cache sets g_cache.updating = true and unlocks diag204_mutex
while invoking diag204().
If a second thread concurrently calls diag204_store(buf, 0, 0) while
g_cache.updating is true:
arch/s390/kernel/diag/diag204.c:diag204_store() {
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);
}
Because allow_busy is 0 and allow_busy_wait is 0, the loop unlocks
diag204_mutex, does not take the -EBUSY return, sleeps once (or returns
-ERESTARTSYS on a pending signal), and then exits at while (allow_busy_wait)
without holding diag204_mutex.
It then calls diag204_is_cache_valid(), diag204_update_cache(), and
memcpy(buf, READ_ONCE(g_cache.buf), ...) without holding diag204_mutex
(risking a NULL dereference or use-after-free on g_cache.buf against
diag204_set_cache()), followed by an unbalanced mutex_unlock(&diag204_mutex).
Also, in hypfs_diag_create_files(), the previous diag204_store() set
DIAG204_BIF_BIT when supported and waited interruptibly on -EBUSY. Should
hypfs_diag_create_files() pass allow_busy=1, allow_busy_wait=1 (matching
dbfs_d204_create()), and how should diag204_store() handle concurrent
updates when allow_busy=0 and allow_busy_wait=0?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-diag204ioctl-v2-0-7e5a9d1cd21a@linux.ibm.com?part=2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 3/3] s390/diag: Add diag204 IOCTL to diag misc device
2026-10-22 11:49 [PATCH v2 0/3] s390: Diag204 Rework Mete Durlu
2026-10-22 11:49 ` [PATCH v2 1/3] s390/diag204: Introduce diag204 rework Mete Durlu
2026-10-22 11:49 ` [PATCH v2 2/3] s390/diag204: Replace diag204 users with new implementation Mete Durlu
@ 2026-10-22 11:49 ` Mete Durlu
2026-10-22 12:04 ` sashiko-bot
2 siblings, 1 reply; 7+ messages in thread
From: Mete Durlu @ 2026-10-22 11:49 UTC (permalink / raw)
To: Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, Sumanth Korikkar
Cc: Ilya Leoshkevich, linux-s390, Mete Durlu
Add IOCTL implementation necessary to fetch diag204 data from the new
diag misc device under /dev.
Signed-off-by: Mete Durlu <meted@linux.ibm.com>
---
arch/s390/include/uapi/asm/diag.h | 8 ++++++++
arch/s390/kernel/diag/diag204.c | 41 ++++++++++++++++++++++++++++++++++++++
arch/s390/kernel/diag/diag_ioctl.h | 3 +++
arch/s390/kernel/diag/diag_misc.c | 6 ++++++
4 files changed, 58 insertions(+)
diff --git a/arch/s390/include/uapi/asm/diag.h b/arch/s390/include/uapi/asm/diag.h
index b7e6ccb4ff6e..985667b2917a 100644
--- a/arch/s390/include/uapi/asm/diag.h
+++ b/arch/s390/include/uapi/asm/diag.h
@@ -22,11 +22,19 @@ struct diag310_memtop {
__u64 nesting_lvl;
};
+struct diag204_request {
+ __u64 address;
+ int allow_busy;
+ int allow_busy_wait;
+};
+
/* Diag ioctl definitions */
#define DIAG324_GET_PIBBUF _IOWR(DIAG_MAGIC_STR, 0x77, struct diag324_pib)
#define DIAG324_GET_PIBLEN _IOR(DIAG_MAGIC_STR, 0x78, size_t)
#define DIAG310_GET_STRIDE _IOR(DIAG_MAGIC_STR, 0x79, size_t)
#define DIAG310_GET_MEMTOPLEN _IOWR(DIAG_MAGIC_STR, 0x7a, size_t)
#define DIAG310_GET_MEMTOPBUF _IOWR(DIAG_MAGIC_STR, 0x7b, struct diag310_memtop)
+#define DIAG204_GET_LEN _IOR(DIAG_MAGIC_STR, 0x7c, size_t)
+#define DIAG204_GET_BUF _IOWR(DIAG_MAGIC_STR, 0x7d, struct diag204_request)
#endif /* __S390_UAPI_ASM_DIAG_H */
diff --git a/arch/s390/kernel/diag/diag204.c b/arch/s390/kernel/diag/diag204.c
index 7f366ea12a67..3450a013258f 100644
--- a/arch/s390/kernel/diag/diag204.c
+++ b/arch/s390/kernel/diag/diag204.c
@@ -14,6 +14,8 @@
#include <linux/vmalloc.h>
#include <asm/diag.h>
+#include <uapi/asm/diag.h>
+#include "diag_ioctl.h"
struct diag204_info {
int pages; /* number of pages for diag204 data */
@@ -220,6 +222,45 @@ int diag204_store(void *buf, int allow_busy, int allow_busy_wait)
return rc;
}
+long diag204_len(unsigned long arg)
+{
+ if (!diag204_is_available())
+ return -EOPNOTSUPP;
+ if (put_user(diag204_get_buf_size(), (size_t __user *)arg))
+ return -EFAULT;
+ return 0;
+}
+
+long diag204_buf(unsigned long arg)
+{
+ struct diag204_request __user *udata;
+ int rc, allow_busy, allow_busy_wait;
+ u64 address;
+ void *buf;
+
+ if (!diag204_is_available())
+ return -EOPNOTSUPP;
+ udata = (struct diag204_request __user *)arg;
+ if (get_user(address, &udata->address))
+ return -EFAULT;
+ if (get_user(allow_busy, &udata->allow_busy))
+ return -EFAULT;
+ if (get_user(allow_busy_wait, &udata->allow_busy_wait))
+ return -EFAULT;
+ buf = diag204_allocate_buffer();
+ if (!buf)
+ return -ENOMEM;
+ rc = diag204_store(buf, allow_busy, allow_busy_wait);
+ if (rc < 0) {
+ diag204_free_buffer(buf);
+ return rc;
+ }
+ rc = copy_to_user((void __user *)address, buf,
+ diag204_get_buf_size());
+ diag204_free_buffer(buf);
+ return rc ? -EFAULT : 0;
+}
+
/*
* diag204_probe() has to find out, which type of diagnose 204 implementation
* we have on our machine. Currently there are three possible scanarios:
diff --git a/arch/s390/kernel/diag/diag_ioctl.h b/arch/s390/kernel/diag/diag_ioctl.h
index 7080be946785..7718b57ffe3e 100644
--- a/arch/s390/kernel/diag/diag_ioctl.h
+++ b/arch/s390/kernel/diag/diag_ioctl.h
@@ -4,6 +4,9 @@
#include <linux/types.h>
+long diag204_len(unsigned long arg);
+long diag204_buf(unsigned long arg);
+
long diag324_pibbuf(unsigned long arg);
long diag324_piblen(unsigned long arg);
diff --git a/arch/s390/kernel/diag/diag_misc.c b/arch/s390/kernel/diag/diag_misc.c
index efffe02ea02e..68742f49a3e6 100644
--- a/arch/s390/kernel/diag/diag_misc.c
+++ b/arch/s390/kernel/diag/diag_misc.c
@@ -35,6 +35,12 @@ static long diag_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
case DIAG310_GET_MEMTOPBUF:
rc = diag310_memtop_buf(arg);
break;
+ case DIAG204_GET_LEN:
+ rc = diag204_len(arg);
+ break;
+ case DIAG204_GET_BUF:
+ rc = diag204_buf(arg);
+ break;
default:
rc = -ENOIOCTLCMD;
break;
--
2.56.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 3/3] s390/diag: Add diag204 IOCTL to diag misc device
2026-10-22 11:49 ` [PATCH v2 3/3] s390/diag: Add diag204 IOCTL to diag misc device Mete Durlu
@ 2026-10-22 12:04 ` sashiko-bot
0 siblings, 0 replies; 7+ messages in thread
From: sashiko-bot @ 2026-10-22 12:04 UTC (permalink / raw)
To: Mete Durlu
Cc: linux-s390, Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] s390/diag: unlocked cache access and mutex unlock in diag204_store()
- [Medium] s390/diag: missing subcode metadata and non-UAPI types in diag204 ioctl
--
commit 8eea5b568c2725dd27a2e3675754996596fdf2e4
Author: Mete Durlu <meted@linux.ibm.com>
s390/diag: Add diag204 IOCTL to diag misc device
This commit adds ioctl handlers to the s390 diag misc device to query the
diag204 buffer length and retrieve diag204 data from userspace.
> diff --git a/arch/s390/include/uapi/asm/diag.h b/arch/s390/include/uapi/asm/diag.h
> index b7e6ccb4ff6e4..985667b2917a5 100644
> --- a/arch/s390/include/uapi/asm/diag.h
> +++ b/arch/s390/include/uapi/asm/diag.h
> @@ -22,11 +22,19 @@ struct diag310_memtop {
> __u64 nesting_lvl;
> };
>
> +struct diag204_request {
> + __u64 address;
> + int allow_busy;
> + int allow_busy_wait;
> +};
> +
> /* Diag ioctl definitions */
> #define DIAG324_GET_PIBBUF _IOWR(DIAG_MAGIC_STR, 0x77, struct diag324_pib)
> #define DIAG324_GET_PIBLEN _IOR(DIAG_MAGIC_STR, 0x78, size_t)
> #define DIAG310_GET_STRIDE _IOR(DIAG_MAGIC_STR, 0x79, size_t)
> #define DIAG310_GET_MEMTOPLEN _IOWR(DIAG_MAGIC_STR, 0x7a, size_t)
> #define DIAG310_GET_MEMTOPBUF _IOWR(DIAG_MAGIC_STR, 0x7b, struct diag310_memtop)
> +#define DIAG204_GET_LEN _IOR(DIAG_MAGIC_STR, 0x7c, size_t)
> +#define DIAG204_GET_BUF _IOWR(DIAG_MAGIC_STR, 0x7d, struct diag204_request)
>
> #endif /* __S390_UAPI_ASM_DIAG_H */
[Severity: Medium]
During boot, diag204_probe() in arch/s390/kernel/diag/diag204.c selects
between subcodes 4, 6, and 7 and sets g_info.subcode and g_info.type
(simple vs. extended format). When userspace calls diag_ioctl() ->
diag204_buf(), the raw hardware buffer is copied to req.address via
copy_to_user() without exposing g_info.subcode (diag204_get_subcode()) or
g_info.type (diag204_get_info_type()).
Unlike dbfs_d204_create() in arch/s390/hypfs/hypfs_diag.c, which prepends
struct dbfs_d204_hdr containing d204->hdr.sc = diag204_get_subcode(), the
raw hardware headers (struct diag204_info_blk_hdr and
struct diag204_x_info_blk_hdr) do not contain a subcode or format field.
Because simple and extended formats have different header and entry sizes
(and subcodes 6 and 7 share the extended header layout while differing in
secondary processor reporting), how can userspace determine which subcode
or format was used to populate the buffer returned by diag204_buf()?
Also, in struct diag204_request, should allow_busy and allow_busy_wait use
fixed-width types (__u32 or __u64) instead of raw int, and should the
ioctl definition use _IOW instead of _IOWR since diag204_buf() only reads
struct diag204_request?
> diff --git a/arch/s390/kernel/diag/diag204.c b/arch/s390/kernel/diag/diag204.c
> index 7f366ea12a673..3450a013258fd 100644
> --- a/arch/s390/kernel/diag/diag204.c
> +++ b/arch/s390/kernel/diag/diag204.c
[ ... ]
> @@ -220,6 +222,45 @@ int diag204_store(void *buf, int allow_busy, int allow_busy_wait)
> return rc;
> }
[ ... ]
> +long diag204_buf(unsigned long arg)
> +{
> + struct diag204_request __user *udata;
> + int rc, allow_busy, allow_busy_wait;
> + u64 address;
> + void *buf;
> +
> + if (!diag204_is_available())
> + return -EOPNOTSUPP;
> + udata = (struct diag204_request __user *)arg;
> + if (get_user(address, &udata->address))
> + return -EFAULT;
> + if (get_user(allow_busy, &udata->allow_busy))
> + return -EFAULT;
> + if (get_user(allow_busy_wait, &udata->allow_busy_wait))
> + return -EFAULT;
> + buf = diag204_allocate_buffer();
> + if (!buf)
> + return -ENOMEM;
> + rc = diag204_store(buf, allow_busy, allow_busy_wait);
[Severity: Critical]
In diag_ioctl() -> diag204_buf() -> diag204_store(), user-controlled
allow_busy and allow_busy_wait values are passed directly into
diag204_store(). What happens if a caller passes allow_busy == 0 and
allow_busy_wait == 0 while another thread is concurrently updating the
cache (g_cache.updating is true)?
Looking at arch/s390/kernel/diag/diag204.c:diag204_store():
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);
When g_cache.updating is true and both allow_busy and allow_busy_wait are
0, the loop unlocks diag204_mutex, sleeps in
schedule_timeout_interruptible(), and then terminates because
allow_busy_wait is 0.
Doesn't this leave diag204_mutex unlocked for the remainder of
diag204_store(), triggering lockdep assertions in diag204_is_cache_valid()
and diag204_update_cache(), racing with vfree() of g_cache.buf in
diag204_set_cache() and diag204_work_fn() during memcpy() (or reading a
null g_cache.buf if the first cache fill has not completed), and calling
mutex_unlock(&diag204_mutex) without holding the lock?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-diag204ioctl-v2-0-7e5a9d1cd21a@linux.ibm.com?part=3
^ permalink raw reply [flat|nested] 7+ messages in thread