Re: [RFC PATCH 12/13] mm/kwatch: add KUnit tests for the watch expression parser

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardcoded 64-bit absolute address in the KUnit test will cause compilation warnings and guaranteed runtime failures on 32-bit platforms.
- [Medium] KUnit test cannot be enabled when KWatch is a loadable module, and fixing the Kconfig exposes a multiple `module_init` build failure.
--

--- Patch [12]: [RFC PATCH 12/13] mm/kwatch: add KUnit tests for the watch expression parser ---
commit 5eda6d9ba13c286416e92d504d097afadcc1400d
Author: Jinchao Wang <wangjinchao600@gmail.com>

mm/kwatch: add KUnit tests for the watch expression parser

This commit adds a KUnit test suite for the KWatch expression parser. It
covers base anchors, offsets, dereference chains, and rejection of malformed
expressions.

> diff --git a/mm/kwatch/Kconfig b/mm/kwatch/Kconfig
> index b1c37a829dd53..74083040a1a33 100644
> --- a/mm/kwatch/Kconfig
> +++ b/mm/kwatch/Kconfig
> @@ -15,3 +15,13 @@ config KWATCH
>  	  exact instruction causing the illegal access.
>  
>  	  If unsure, say N.
> +
> +config KWATCH_KUNIT_TEST
> +	bool "KUnit tests for KWatch" if !KUNIT_ALL_TESTS
> +	depends on KWATCH && KUNIT

[Severity: Medium]
Will this test be silently disabled when KWATCH is built as a loadable module?

Because CONFIG_KWATCH_KUNIT_TEST is a bool, if KWATCH is configured as a
module ('m'), the dependency expression evaluates to 'm' and Kconfig will
automatically downgrade the boolean's visibility to 'n', making it impossible
to run the test suite for module builds.

> +	default KUNIT_ALL_TESTS
> +	help
> +	  Enable KUnit tests for the KWatch kernel module.

[ ... ]

> diff --git a/mm/kwatch/deref_test.c b/mm/kwatch/deref_test.c
> new file mode 100644
> index 0000000000000..094b7afeb235e
> --- /dev/null
> +++ b/mm/kwatch/deref_test.c
> @@ -0,0 +1,137 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <kunit/test.h>
> +#include "kwatch.h"
> +#include <linux/string.h>
> +
> +static void kwatch_test_parse_deref_chain(struct kunit *test)
> +{
> +	struct kwatch_config cfg;
> +	int ret;

[ ... ]

> +	// Test 13: Absolute address
> +	memset(&cfg, 0, sizeof(cfg));
> +	ret = kwatch_deref_parse(&cfg, "0xffffffff81000000+8");

[Severity: High]
Will this hardcoded 64-bit address cause failures on 32-bit platforms?

On 32-bit architectures, this string will exceed ULONG_MAX, causing
the internal kstrtoul() to safely fail and return -ERANGE. The parser
will then fall back to treating it as a global symbol name via
kallsyms_lookup_name(), which will also fail, ultimately returning -EINVAL
and breaking the test logic.

> +	KUNIT_EXPECT_EQ(test, ret, 0);
> +	KUNIT_EXPECT_EQ(test, cfg.base, KWATCH_BASE_ABS_ADDR);
> +	KUNIT_EXPECT_EQ(test, cfg.sym_addr, 0xffffffff81000000UL);

[Severity: High]
Does this 64-bit literal cause implicit truncation compiler warnings 
and test comparison failures on 32-bit architectures where unsigned long
is only 32 bits?

> +	KUNIT_EXPECT_EQ(test, cfg.offset_count, 1);
> +	KUNIT_EXPECT_EQ(test, cfg.offsets[0], 8);
> +}
> +
> +static struct kunit_case kwatch_deref_test_cases[] = {
> +	KUNIT_CASE(kwatch_test_parse_deref_chain),
> +	{}
> +};
> +
> +static struct kunit_suite kwatch_deref_test_suite = {
> +	.name = "kwatch_deref",
> +	.test_cases = kwatch_deref_test_cases,
> +};
> +
> +kunit_test_suite(kwatch_deref_test_suite);

[Severity: Medium]
If the Kconfig dependency above is fixed to allow this test to run with module
builds (e.g., depends on KWATCH != n), will this cause a multiple definition
build failure? 

Since deref_test.o is linked directly into the kwatch module in the Makefile,
and kunit_test_suite() emits a module_init() alias, this will conflict with
the primary module_init() definition that likely already exists for kwatch.

> +
> +MODULE_DESCRIPTION("KUnit tests for the KWatch watch expression parser");
> +MODULE_LICENSE("GPL");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714182243.10687-1-wangjinchao600@gmail.com?part=12