[PATCH v2 0/3] s390: Diag204 Rework

[PATCH v2 0/3] s390: Diag204 Rework

[Mete Durlu]

v2: 
- prevent redundant work rearm after cache free in diag204_work_fn().
  (Sashiko)
- always fallback to subcode 4 in diag204_probe(). (Sashiko)
- fix errno check during busy wait loop and prevent uncessary
  sleeps. (Sashiko)
- simplify diag204 cache update function, allow for diag to return busy
  even when diag cache is empty. (Sashiko)
- add a check for diag204 subcode in sthyi as it depends on subcode 7
  being available. (Sashiko)
- remove CAP_SYS_ADMIN check in IOCTL handler (Hendrik Brueckner)
- add missing READ_ONCE() to g_cache.timestamp
- schedule delayed work only after a successful cache update
- rebase patches

Background
========================================================================
The goal is to create a single source where all relevant diag204
functionality is exposed and managed.

The main problems being addressed are;
* Diag204 is expensive and calling it too frequently can cause
  performance issues and also data integrity problems. Rate limiting is
  required.
* Diag204 requires a prep phase to utilize properly which means code
  duplication for each user.
* Diag204's current interface in debugfs may not be easily reachable
  as debugfs is seen as a security vulnarability by some distributions.
  The new IOCTL diag device should offer alternartive for diag204.

This is also a prerequisite for utilizing diag204 data in hiperdispatch.
There are plans to incorporate utilization data for making smarter
decisions.

Patches
========================================================================
Patch 1-2:
This patch is split into two, first half is introducing the new diag204
implementation and the second half is replacing the exisiting users.
This is purely for easier review. Before accepted I expect first two
patches to be squashed.

Patch 1:
The first patch introduces a caching mechanism inspired by Sumanth
Korikkar's diag324 implementation. Data buffer gets cached and cache
access is serialized via mutexes. If unused for a defined period diag204
data cache gets freed by a worker after a delay, that way it is
guaranteed that if an LPAR has no diag204 users, no memory is consumed.

Patch 2:
The second patch adapts the existing diag204 users to the new
interface.

Patch 3:
The third patch brings diag204 functionality to the new diag misc device.
Misc device gets two new magic values for diag204, one to fetch the data
size and the other is to actually get the diag data.
The uapi now has a new struct diag204_request for requesting the data.
Struct holds a pointer address to the userspace buffer and two behavior
altering flags; allow_busy and allow_busy_loop. Impact of said flags
are explained in Considerations section below.

New Diag204 Usage
========================================================================
Users from kernel are now exposed to a couple of common functions 
which should allow for a simpler way of utilizing diag204.
The new streamlined flow now looks like;

...
{
	void *buf = diag204_allocate_buffer();
	int rc = diag204_store(buf, allow_busy, allow_busy_wait);

	/* Do whatever with the diag data*/

	diag204_free_buffer(buf);
...

Most of the details of diag204 is now hidden away. Initial probing and
availability check is done via a device_initcall().

Implementation
========================================================================
Initial checks for diag204 support, determining which subcode to use and
querying page count required to hold diag data are all done in a
device_initcall() once. Page count returned is the maximum amount
required for that CEC, subcodes should also stay the same during
runtime.

Diag204 should be rate limited to a diag call per second at most.
In order to achive the rate limiting while also allowing users to
acquire data a caching mechanism similar to Sumanth's diag324
implementation is proposed.
The caching mechanism is slightly more complicated to allow support for
interruptible busy waiting. When diag204's busy indication facility is
installed users can specify a flag to put diag204_store() into a
sleepable and interruptible loop that lasts until diag204 is no longer
busy. This way the amount of diag calls and system calls from userspace
are reduced.
If busy indication facility is not present default fallback is the
blocking diag204 call as before. Users don't get an indication about
this.(IMO, it wouldn't be useful)

Diag204 calls are serialized with a mutex and cache updates are
protected with help of a flag. Since busy wait loops can sleep they have
to release the mutex before sleep. The updating cache flag comes in
here, it is only modified when the mutex is being held and if a cache
update is taking place.

Utilizing diag204 arms a work function which periodically checks if the
current cache is expired. If it finds that the cache is expired, it
frees the cache buffer and stops.

IOCTL Interface
========================================================================
A similar IOCTL interface to existing diag calls is implemented.
Two new ioctl definitions along with a new data struct is added to the
uapi. DIAG204_GET_LEN allows userspace to acquire required buffer
length and DIAG204_GET_BUF fetches the diag204 data.
When issuing an IOCTL call with DIAG204_GET_BUF users have to utilize
the new diag204_request struct to specify the allocated buffer address
along with the allow_busy an allow_busy_wait flags for diag204 store
operation.

Considerations
========================================================================
* At the moment diag204 size request returns a page count requirement of
  423. That is the maximum amount of space needed currently for a CEC to
  service diag204.
  With this series at times there will be ~6Mb (1.6Mb for each; cache,
  temp, user buffer...) memory allocated for diag204 data per lpar.

* With the new IOCTL interface, userspace is defaulted to the
  largest diag204 data request subcode available(from subcodes 4, 6 and
  7 always the largest available is used as it returns more data).
  While this aligns with the existing behavior, question is, would
  there be any value in allowing users to specify which subcode to use?
  Although this also means more complicated implementation.

* A new addition with the IOCTL interface are the allow_busy and
  allow_busy_wait toggles. For sure there is benefit in allowing diag204
  to return BUSY instead of waiting for longer durations when it is not
  ready at the moment. allow_busy_wait adds an interruptable polling
  loop around that, with that mechanism diag204 users from userspace can
  wait for diag204 to return from its BUSY state within the same system
  call. Obviously using only a single system call is better but diag204
  users are not performance critical atm and the same polling can be
  implemented from userspace side. This can help reduce some complexity
  on the kernel code.

Changes from the earlier series;
* Address mutex lockdep issues (Sumanth Korikkar)
* Add permission check for IOCTL (Sumanth Korikkar)
* Pass buffers set to 0 when calling diag (Sumanth Korikkar)

---
---
base-commit: a804c5742bff224d4cb4d25dd383554d598be32b

Mete Durlu (3):
      s390/diag204: Introduce diag204 rework
      s390/diag204: Replace diag204 users with new implementation
      s390/diag: Add diag204 IOCTL to diag misc device

 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       |   8 +-
 arch/s390/include/uapi/asm/diag.h  |   8 +
 arch/s390/kernel/diag/Makefile     |   2 +-
 arch/s390/kernel/diag/diag204.c    | 331 +++++++++++++++++++++++++++++++++++++
 arch/s390/kernel/diag/diag_ioctl.h |   3 +
 arch/s390/kernel/diag/diag_misc.c  |   6 +
 arch/s390/kernel/sthyi.c           |  30 ++--
 10 files changed, 384 insertions(+), 157 deletions(-)

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

[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

[PATCH v2 2/3] s390/diag204: Replace diag204 users with new implementation

[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

[PATCH v2 3/3] s390/diag: Add diag204 IOCTL to diag misc device

[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

Re: [PATCH v2 2/3] s390/diag204: Replace diag204 users with new implementation

[sashiko-bot]

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

Re: [PATCH v2 3/3] s390/diag: Add diag204 IOCTL to diag misc device

[sashiko-bot]

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

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

[sashiko-bot]

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