Re: [PATCH v13 26/39] dyndbg-test: add do_bulk testpoint, rename do_prints to do_classes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dyndbg-test: early-boot infinite loop on negative repeat count in do_classes() and do_bulk()
--

commit bf6fb515ebe31cab587b06b6c8aeb75be6f0a072
Author: Jim Cromie <jim.cromie@gmail.com>

dyndbg-test: add do_bulk testpoint, rename do_prints to do_classes

This commit renames do_prints to do_classes, adds a new do_bulk module
parameter for generating high-volume debug output, and shares a common
parameter callback that supports repeat counts and negative duration timeouts.

> diff --git a/lib/test_dynamic_debug.c b/lib/test_dynamic_debug.c
> index 01ce07001d4c7..f78764025c441 100644
> --- a/lib/test_dynamic_debug.c
> +++ b/lib/test_dynamic_debug.c
> @@ -28,25 +28,57 @@
>  #endif
>  
>  #include <linux/module.h>
> +#include <linux/sched.h>
> +#include <linux/sched/signal.h>
> +#include <linux/jiffies.h>
>  
> -/* re-gen output by reading or writing sysfs node: do_prints */
> +#define TEST_MAX_REPEATS 10000
> +#define TEST_MAX_SECONDS 60
>  
> -static void do_prints(void); /* device under test */
> -static int param_set_do_prints(const char *instr, const struct kernel_param *kp)
> +/* re-trigger debug output by reading or writing sysfs nodes: do_classes or do_bulk */
> +static void do_classes(int); /* device under test */
> +static void do_bulk(int);    /* device under test */
> +
> +static int param_set_do_repeats(const char *instr, const struct kernel_param *kp)
>  {
> -	do_prints();
> +	int rc, ct;
> +	void (*repeat_fn)(int) = kp->arg;
> +
> +	rc = kstrtoint(instr, 0, &ct);
> +	if (rc) {
> +		pr_err("expecting numeric input, using 1 instead\n");
> +		ct = 1;
> +	}
> +	if (ct > TEST_MAX_REPEATS) {
> +		pr_warn("%s: clamping %d to %d\n", kp->name, ct, TEST_MAX_REPEATS);
> +		ct = TEST_MAX_REPEATS;
> +	} else if (ct < -TEST_MAX_SECONDS) {
> +		pr_warn("%s: clamping %d seconds to -%d\n",
> +			kp->name, ct, TEST_MAX_SECONDS);
> +		ct = -TEST_MAX_SECONDS;
> +	}
> +
> +	repeat_fn(ct);
> +
>  	return 0;
>  }
> -static int param_get_do_prints(char *buffer, const struct kernel_param *kp)
> +
> +static int param_get_do_repeats(char *buffer, const struct kernel_param *kp)
>  {
> -	do_prints();
> -	return scnprintf(buffer, PAGE_SIZE, "did do_prints\n");
> +	void (*repeat_fn)(int) = kp->arg;
> +
> +	repeat_fn(1);
> +
> +	return scnprintf(buffer, PAGE_SIZE, "did 1 %s\n", kp->name);
>  }
> -static const struct kernel_param_ops param_ops_do_prints = {
> -	.set = param_set_do_prints,
> -	.get = param_get_do_prints,
> +
> +static const struct kernel_param_ops param_ops_do_repeats = {
> +	.set = param_set_do_repeats,
> +	.get = param_get_do_repeats,
>  };
> -module_param_cb(do_prints, &param_ops_do_prints, NULL, 0600);
> +
> +module_param_cb(do_classes, &param_ops_do_repeats, do_classes, 0600);
> +module_param_cb(do_bulk, &param_ops_do_repeats, do_bulk, 0600);

[ ... ]

> @@ -210,20 +247,85 @@ static void do_levels(void)
>  	prdbg(Vu5);
>  	prdbg(Vu6);
>  	prdbg(Vu7);
> +	prdbg(Vu8);
>  #endif
>  }
>  
> -static void do_prints(void)
> +static void do_classes(int ct)
> +{
> +	if (ct < 0) {
> +		unsigned long timeout = jiffies + (-ct) * HZ;
> +
> +		pr_debug("%s for %d seconds:\n", __func__, -ct);
> +		while (time_before(jiffies, timeout)) {
> +			if (signal_pending(current))
> +				break;
> +			do_cats();
> +			do_levels();
> +			cond_resched();
> +		}
> +		return;
> +	}
> +
> +	pr_debug("do_classes %u times:\n", ct);
> +	for (; ct; ct--) {
> +		if (signal_pending(current))
> +			break;
> +		do_cats();
> +		do_levels();
> +		cond_resched();
> +	}
> +}
> +
> +static void do_bulk(int ct)
>  {
> -	pr_debug("do_prints:\n");
> -	do_cats();
> -	do_levels();
> +	unsigned int i;
> +
> +	if (ct < 0) {
> +		unsigned long timeout = jiffies + (-ct) * HZ;
> +
> +		pr_debug("%s for %d seconds:\n", __func__, -ct);
> +		for (i = 0; time_before(jiffies, timeout); i++) {
> +			if (signal_pending(current))
> +				break;
> +			pr_debug("bulk msg %u.0\n", i + 1);
> +			pr_debug("bulk msg %u.1\n", i + 1);
> +			pr_debug("bulk msg %u.2\n", i + 1);
> +			pr_debug("bulk msg %u.3\n", i + 1);
> +			pr_debug("bulk msg %u.4\n", i + 1);
> +			pr_debug("bulk msg %u.5\n", i + 1);
> +			pr_debug("bulk msg %u.6\n", i + 1);
> +			pr_debug("bulk msg %u.7\n", i + 1);
> +			pr_debug("bulk msg %u.8\n", i + 1);
> +			pr_debug("bulk msg %u.9\n", i + 1);
> +			cond_resched();
> +		}
> +		return;
> +	}

[Severity: Medium]
Can passing a negative value to do_classes or do_bulk on the kernel command
line hang the boot when CONFIG_TEST_DYNAMIC_DEBUG=y (or
CONFIG_TEST_DYNAMIC_DEBUG_SUBMOD=y)?

Both do_classes and do_bulk are registered with module_param_cb(), which sets
the parameter level to -1:

include/linux/moduleparam.h:
#define module_param_cb(name, ops, arg, perm) \
	__module_param_call(MODULE_PARAM_PREFIX, name, ops, arg, perm, -1, 0)

When the test driver is built-in, command-line parameters such as
test_dynamic_debug.do_classes=-1 or test_dynamic_debug.do_bulk=-1 are parsed
in start_kernel() -> parse_args() while local interrupts are disabled and
before init_IRQ(), tick_init(), timers_init(), and time_init() have run.

At that stage of boot, param_set_do_repeats() calls do_classes(ct) or
do_bulk(ct) with ct < 0, which loops waiting for time_before(jiffies,
timeout) to become false. Since jiffies does not advance and
signal_pending(current) on init_task never becomes true during early boot,
will this loop spin forever?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-dd-cmap-part2-clean-v13-0-9267923be612@gmail.com?part=26