[PATCH v3 0/3] s390/debug: Fix several s390dbf issues reported by AI scan

[PATCH v3 0/3] s390/debug: Fix several s390dbf issues reported by AI scan

[Mikhail Zaslonko]

The s390dbf kernel parameter was added in commit a2cec6863709
("s390/debug: Add s390dbf kernel parameter"). This series fixes
three issues found in the surrounding code.

Patch 1 fixes a NULL pointer dereference in debug_set_level()
introduced by that commit: the NULL check for the id argument
was moved into the _debug_set_level() helper, but the newly added
debug_get_param() call was left in the wrapper ahead of it.

Patch 2 eliminates a duplicate override notice that appears when
drivers call debug_set_level() right after debug_register(), which
is the common pattern. With the s390dbf= parameter active and
many devices (e.g. many DASDs), this produced a lot of redundant
log lines during boot.

Patch 3 fixes a pre-existing race between debug area resize and
event logging. Setting pages to zero via the debugfs interface
races with concurrent event writers: the unlocked id->areas check
passes, but by the time the lock is acquired debug_areas_swap()
may have published a NULL pointer, which get_active_entry() then
dereferences. The race was confirmed to hang the system in testing
by running concurrent debug area resize (pages 0->4 in a tight
loop) against parallel DASD I/O.

Changes in v3:
- Patch 3: put back !id->areas check under lock in debug_flush()
  and remove existing !id->areas check before the lock (Peter Oberparleiter)

Changes in v2:
- Patch 1: Trimmed commit message; !id check removed from
  _debug_set_level() (Peter Oberparleiter)
- Patch 3: Trimmed commit message; !id->areas check removed
  from debug_flush() (Peter Oberparleiter)

Mikhail Zaslonko (3):
  s390/debug: Fix NULL pointer dereference in debug_set_level()
  s390/debug: Do not repeat parameter override notice on
    debug_set_level()
  s390/debug: Fix race between debug area resize and event logging

 arch/s390/kernel/debug.c | 54 ++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 18 deletions(-)

-- 
2.55.0

[PATCH v3 1/3] s390/debug: Fix NULL pointer dereference in debug_set_level()

[Mikhail Zaslonko]

Commit a2cec6863709 ("s390/debug: Add s390dbf kernel parameter")
incorrectly removed a null-id check from debug_set_level(), introducing
a possible NULL pointer dereference for debug-API users that put
debug_register() results unchecked into debug_set_level().

Fix this by moving the check from the internal _debug_set_level()
variant back to the external debug_set_level() wrapper.

Fixes: a2cec6863709 ("s390/debug: Add s390dbf kernel parameter")

Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
---
 arch/s390/kernel/debug.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index 14d2b58ad093..e06abf1dbc21 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -1074,9 +1074,6 @@ static void _debug_set_level(debug_info_t *id, int new_level)
 {
 	unsigned long flags;
 
-	if (!id)
-		return;
-
 	if (new_level == DEBUG_OFF_LEVEL) {
 		pr_info("%s: switched off\n", id->name);
 	} else if ((new_level > DEBUG_MAX_LEVEL) || (new_level < 0)) {
@@ -1101,6 +1098,9 @@ static void _debug_set_level(debug_info_t *id, int new_level)
  */
 void debug_set_level(debug_info_t *id, int new_level)
 {
+	if (!id)
+		return;
+
 	/* Level specified via kernel parameter takes precedence */
 	debug_get_param(id->name, &new_level, NULL);
 
-- 
2.55.0

[PATCH v3 2/3] s390/debug: Do not repeat parameter override notice on debug_set_level()

[Mikhail Zaslonko]

Commit a2cec6863709 ("s390/debug: Add s390dbf kernel parameter") calls
debug_get_param() from both debug_info_create() and debug_set_level().
Since debug_get_param() emits the override notice unconditionally, and
drivers typically call debug_set_level() right after debug_register(),
the same line is printed twice per debug area:

  s390dbf: 0.0.1234: override level to 6
  s390dbf: 0.0.1234: override level to 6

For areas registered per device this is multiplied by the device count.
With 's390dbf=0.0.*:6' a system with many DASDs emits a large number of
redundant lines during boot.

Add a quiet parameter to debug_get_param() and pass quiet=true from
debug_set_level(), where the override has already been announced during
registration. The remaining callers keep printing the notice.

Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
---
 arch/s390/kernel/debug.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index e06abf1dbc21..cf411f203571 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -182,7 +182,7 @@ static struct debug_param_t {
 static int debug_param_num;
 
 /* functions */
-static void debug_get_param(const char *name, int *level, int *pages)
+static void debug_get_param(const char *name, int *level, int *pages, bool quiet)
 {
 	struct debug_param_t *p;
 	int i;
@@ -192,11 +192,13 @@ static void debug_get_param(const char *name, int *level, int *pages)
 		if (!glob_match(p->name, name))
 			continue;
 		if (level && p->level != PARAM_UNSET) {
-			pr_info("%s: override level to %d\n", name, p->level);
+			if (!quiet)
+				pr_info("%s: override level to %d\n", name, p->level);
 			*level = p->level;
 		}
 		if (pages && p->pages != PARAM_UNSET) {
-			pr_info("%s: override pages to %d\n", name, p->pages);
+			if (!quiet)
+				pr_info("%s: override pages to %d\n", name, p->pages);
 			*pages = p->pages;
 		}
 	}
@@ -251,7 +253,7 @@ static int __init s390dbf_parse(char *arg)
 	 * regular memory allocations are possible.
 	 */
 	for (i = 0, id = __s390dbf_info; &id[i] < __s390dbf_info_end; i++)
-		debug_get_param(id[i]->name, &id[i]->level, NULL);
+		debug_get_param(id[i]->name, &id[i]->level, NULL, false);
 
 	return rc;
 }
@@ -395,7 +397,7 @@ static debug_info_t *debug_info_create(const char *name, int pages_per_area,
 	int level = DEBUG_DEFAULT_LEVEL;
 	debug_info_t *rc;
 
-	debug_get_param(name, &level, &pages_per_area);
+	debug_get_param(name, &level, &pages_per_area, false);
 	rc = debug_info_alloc(name, pages_per_area, nr_areas, buf_size, level, ALL_AREAS);
 	if (!rc)
 		goto out;
@@ -960,7 +962,7 @@ void debug_register_static(debug_info_t *id, int pages_per_area, int nr_areas)
 		return;
 	}
 
-	debug_get_param(id->name, &id->level, &pages_per_area);
+	debug_get_param(id->name, &id->level, &pages_per_area, false);
 	copy = debug_info_alloc("", pages_per_area, nr_areas, id->buf_size,
 				id->level, ALL_AREAS);
 	if (!copy) {
@@ -1101,8 +1103,11 @@ void debug_set_level(debug_info_t *id, int new_level)
 	if (!id)
 		return;
 
-	/* Level specified via kernel parameter takes precedence */
-	debug_get_param(id->name, &new_level, NULL);
+	/*
+	 * Level specified via kernel parameter takes precedence. The override
+	 * was already announced during registration, so stay quiet here.
+	 */
+	debug_get_param(id->name, &new_level, NULL, true);
 
 	_debug_set_level(id, new_level);
 }
-- 
2.55.0

[PATCH v3 3/3] s390/debug: Fix race between debug area resize and event logging

[Mikhail Zaslonko]

Trace functions check for non-NULL id->areas without lock to minimize
overhead. This opens a race window where a NULL pointer dereference
occurs if id->areas is set to NULL (e.g. via echo 0 > ../pages) after
the check and before id->lock is taken.

Fix this by rechecking id->areas under lock.

Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
---
 arch/s390/kernel/debug.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index cf411f203571..b5bf8284dbfc 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -1283,7 +1283,7 @@ void debug_set_critical(void)
 debug_entry_t *debug_event_common(debug_info_t *id, int level, const void *buf,
 				  int len)
 {
-	debug_entry_t *active;
+	debug_entry_t *active = NULL;
 	unsigned long flags;
 
 	if (!debug_active || !id->areas)
@@ -1294,6 +1294,8 @@ debug_entry_t *debug_event_common(debug_info_t *id, int level, const void *buf,
 	} else {
 		raw_spin_lock_irqsave(&id->lock, flags);
 	}
+	if (!id->areas)
+		goto out;
 	do {
 		active = get_active_entry(id);
 		memcpy(DEBUG_DATA(active), buf, min(len, id->buf_size));
@@ -1303,7 +1305,7 @@ debug_entry_t *debug_event_common(debug_info_t *id, int level, const void *buf,
 		len -= id->buf_size;
 		buf += id->buf_size;
 	} while (len > 0);
-
+out:
 	raw_spin_unlock_irqrestore(&id->lock, flags);
 	return active;
 }
@@ -1316,7 +1318,7 @@ EXPORT_SYMBOL(debug_event_common);
 debug_entry_t *debug_exception_common(debug_info_t *id, int level,
 				      const void *buf, int len)
 {
-	debug_entry_t *active;
+	debug_entry_t *active = NULL;
 	unsigned long flags;
 
 	if (!debug_active || !id->areas)
@@ -1327,6 +1329,8 @@ debug_entry_t *debug_exception_common(debug_info_t *id, int level,
 	} else {
 		raw_spin_lock_irqsave(&id->lock, flags);
 	}
+	if (!id->areas)
+		goto out;
 	do {
 		active = get_active_entry(id);
 		memcpy(DEBUG_DATA(active), buf, min(len, id->buf_size));
@@ -1336,7 +1340,7 @@ debug_entry_t *debug_exception_common(debug_info_t *id, int level,
 		len -= id->buf_size;
 		buf += id->buf_size;
 	} while (len > 0);
-
+out:
 	raw_spin_unlock_irqrestore(&id->lock, flags);
 	return active;
 }
@@ -1362,7 +1366,7 @@ static inline int debug_count_numargs(char *string)
 debug_entry_t *__debug_sprintf_event(debug_info_t *id, int level, char *string, ...)
 {
 	debug_sprintf_entry_t *curr_event;
-	debug_entry_t *active;
+	debug_entry_t *active = NULL;
 	unsigned long flags;
 	int numargs, idx;
 	va_list ap;
@@ -1377,6 +1381,8 @@ debug_entry_t *__debug_sprintf_event(debug_info_t *id, int level, char *string,
 	} else {
 		raw_spin_lock_irqsave(&id->lock, flags);
 	}
+	if (!id->areas)
+		goto out;
 	active = get_active_entry(id);
 	curr_event = (debug_sprintf_entry_t *) DEBUG_DATA(active);
 	va_start(ap, string);
@@ -1385,6 +1391,7 @@ debug_entry_t *__debug_sprintf_event(debug_info_t *id, int level, char *string,
 		curr_event->args[idx] = va_arg(ap, long);
 	va_end(ap);
 	debug_finish_entry(id, active, level, 0);
+out:
 	raw_spin_unlock_irqrestore(&id->lock, flags);
 
 	return active;
@@ -1397,7 +1404,7 @@ EXPORT_SYMBOL(__debug_sprintf_event);
 debug_entry_t *__debug_sprintf_exception(debug_info_t *id, int level, char *string, ...)
 {
 	debug_sprintf_entry_t *curr_event;
-	debug_entry_t *active;
+	debug_entry_t *active = NULL;
 	unsigned long flags;
 	int numargs, idx;
 	va_list ap;
@@ -1413,6 +1420,8 @@ debug_entry_t *__debug_sprintf_exception(debug_info_t *id, int level, char *stri
 	} else {
 		raw_spin_lock_irqsave(&id->lock, flags);
 	}
+	if (!id->areas)
+		goto out;
 	active = get_active_entry(id);
 	curr_event = (debug_sprintf_entry_t *)DEBUG_DATA(active);
 	va_start(ap, string);
@@ -1421,6 +1430,7 @@ debug_entry_t *__debug_sprintf_exception(debug_info_t *id, int level, char *stri
 		curr_event->args[idx] = va_arg(ap, long);
 	va_end(ap);
 	debug_finish_entry(id, active, level, 1);
+out:
 	raw_spin_unlock_irqrestore(&id->lock, flags);
 
 	return active;
@@ -1663,9 +1673,11 @@ static void debug_flush(debug_info_t *id, int area)
 	unsigned long flags;
 	int i, j;
 
-	if (!id || !id->areas)
+	if (!id)
 		return;
 	raw_spin_lock_irqsave(&id->lock, flags);
+	if (!id->areas)
+		goto out;
 	if (area == DEBUG_FLUSH_ALL) {
 		id->active_area = 0;
 		memset(id->active_entries, 0, id->nr_areas * sizeof(int));
@@ -1680,6 +1692,7 @@ static void debug_flush(debug_info_t *id, int area)
 		for (i = 0; i < id->pages_per_area; i++)
 			memset(id->areas[area][i], 0, PAGE_SIZE);
 	}
+out:
 	raw_spin_unlock_irqrestore(&id->lock, flags);
 }
 
-- 
2.55.0

Re: [PATCH v3 1/3] s390/debug: Fix NULL pointer dereference in debug_set_level()

[sashiko-bot]

> Commit a2cec6863709 ("s390/debug: Add s390dbf kernel parameter")
> incorrectly removed a null-id check from debug_set_level(), introducing
> a possible NULL pointer dereference for debug-API users that put
> debug_register() results unchecked into debug_set_level().
> 
> Fix this by moving the check from the internal _debug_set_level()
> variant back to the external debug_set_level() wrapper.
> 
> Fixes: a2cec6863709 ("s390/debug: Add s390dbf kernel parameter")
> 
> Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903130733.709001-1-zaslonko@linux.ibm.com?part=1

Re: [PATCH v3 2/3] s390/debug: Do not repeat parameter override notice on debug_set_level()

[sashiko-bot]

> Commit a2cec6863709 ("s390/debug: Add s390dbf kernel parameter") calls
> debug_get_param() from both debug_info_create() and debug_set_level().
> Since debug_get_param() emits the override notice unconditionally, and
> drivers typically call debug_set_level() right after debug_register(),
> the same line is printed twice per debug area:
> 
>   s390dbf: 0.0.1234: override level to 6
>   s390dbf: 0.0.1234: override level to 6
> 
> For areas registered per device this is multiplied by the device count.
> With 's390dbf=0.0.*:6' a system with many DASDs emits a large number of
> redundant lines during boot.
> 
> Add a quiet parameter to debug_get_param() and pass quiet=true from
> debug_set_level(), where the override has already been announced during
> registration. The remaining callers keep printing the notice.
> 
> Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903130733.709001-1-zaslonko@linux.ibm.com?part=2

Re: [PATCH v3 3/3] s390/debug: Fix race between debug area resize and event logging

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Preexisting NULL pointer dereference in debug_info_copy() if debug_register_static() fails to allocate memory.
--

commit 1749ca122b22e4769c7efc7f4a52046777b678ef
Author: Mikhail Zaslonko <zaslonko@linux.ibm.com>

s390/debug: Fix race between debug area resize and event logging

Trace functions check for non-NULL id->areas without lock to minimize
overhead. This opens a race window where a NULL pointer dereference
occurs if id->areas is set to NULL after the check and before id->lock
is taken. This commit fixes the race by rechecking id->areas under lock.

> diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
> index cf411f203571b..b5bf8284dbfc8 100644
> --- a/arch/s390/kernel/debug.c
> +++ b/arch/s390/kernel/debug.c
[ ... ]
> @@ -1663,9 +1673,11 @@ static void debug_flush(debug_info_t *id, int area)
>  	unsigned long flags;
>  	int i, j;
>  
> -	if (!id || !id->areas)
> +	if (!id)
>  		return;
>  	raw_spin_lock_irqsave(&id->lock, flags);
> +	if (!id->areas)
> +		goto out;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential NULL
pointer dereference in debug_info_copy() if debug_register_static() fails
to allocate memory.

In arch/s390/kernel/debug.c:debug_register_static(), if the allocation fails:

	if (!copy) {
		pr_err("Registering debug feature %s failed\n", id->name);

		/* Clear pointers to prevent tracing into released initdata. */
		raw_spin_lock_irqsave(&id->lock, flags);
		id->areas = NULL;
		id->active_pages = NULL;
		id->active_entries = NULL;
		raw_spin_unlock_irqrestore(&id->lock, flags);

		return;
	}

This sets id->areas to NULL but leaves id->pages_per_area and id->nr_areas
with their original EARLY_PAGES and EARLY_AREAS values.

Later, if debug_info_copy() is called when a user accesses the exposed
debugfs file:

	for (i = 0; i < in->nr_areas; i++) {
		for (j = 0; j < in->pages_per_area; j++)
			memcpy(rc->areas[i][j], in->areas[i][j], PAGE_SIZE);

Will this cause a NULL pointer dereference when accessing in->areas[i][j]
since in->areas is NULL but the loop still executes based on in->nr_areas?
Should we reset id->pages_per_area and id->nr_areas to 0 on the error path
in debug_register_static(), or add a NULL check in debug_info_copy()?

>  	if (area == DEBUG_FLUSH_ALL) {
>  		id->active_area = 0;
>  		memset(id->active_entries, 0, id->nr_areas * sizeof(int));
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903130733.709001-1-zaslonko@linux.ibm.com?part=3