Re: [PATCH] tools/mm: add hwpoison-panic tool

[SJ Park <sj@kernel.org>]

On Fri, 31 Jul 2026 06:16:12 -0700 Breno Leitao <leitao@debian.org> wrote:

> Add a tool that enables the vm.panic_on_unrecoverable_memory_failure
> sysctl, picks a kernel-owned PFN and writes its physical address to
> hard_offline_page.  Three page kinds are selectable with -k: rodata
> (default), slab or pgtable. In all cases the host should panic.
> 
> Example:
> 
>         # ./hwpoison-panic  -k slab -f
>         injecting hwpoison at phys 0x100032000 (pfn 0x100032, kind=slab)
>         expecting kernel panic: 'Memory failure: <pfn>: unrecoverable page'
> 
> In dmesg, you will see:
> 
>         Memory failure: 0x100032: unhandlable page.
>         Memory failure: 0x100032: recovery action for reserved kernel page: Ignored
>         Kernel panic - not syncing: Memory failure: 0x100032: unrecoverable page
> 
> This lives in tools/mm rather than selftests/mm because every successful
> run crashes the machine, which is not something to run from CI.

If the purpose of the new program is testing, would it make more sense to put
it under tools/testing/ ?

Also, does this test really need to be end-to-end test that cause panic?  How
about doing unit test of core functions, using tools like kunit?  Or,
live-patching panic() to only print messages and return before running the
test?

> 
> Argument -f is required so an accidental invocation does not take the
> box down.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  MAINTAINERS                              |   1 +
>  tools/mm/.gitignore                      |   1 +
>  tools/mm/Makefile                        |   2 +
>  tools/mm/memory-failure/hwpoison-panic.c | 353 +++++++++++++++++++++++++++++++
>  4 files changed, 357 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 56b337a064478..799c4883d5b0b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12094,6 +12094,7 @@ F:	include/linux/memory-failure.h
>  F:	include/trace/events/memory-failure.h
>  F:	mm/hwpoison-inject.c
>  F:	mm/memory-failure.c
> +F:	tools/mm/memory-failure/
>  F:	tools/testing/selftests/mm/memory-failure.c
>  
>  HYCON HY46XX TOUCHSCREEN SUPPORT
> diff --git a/tools/mm/.gitignore b/tools/mm/.gitignore
> index 1446a659e5408..b9b80d909db4a 100644
> --- a/tools/mm/.gitignore
> +++ b/tools/mm/.gitignore
> @@ -3,3 +3,4 @@ slabinfo
>  page-types
>  page_owner_sort
>  thp_swap_allocator_test
> +memory-failure/hwpoison-panic
> diff --git a/tools/mm/Makefile b/tools/mm/Makefile
> index 858186a6eefdb..9f6de957a95a0 100644
> --- a/tools/mm/Makefile
> +++ b/tools/mm/Makefile
> @@ -4,6 +4,7 @@
>  include ../scripts/Makefile.include
>  
>  BUILD_TARGETS=page-types slabinfo page_owner_sort page_owner_filter thp_swap_allocator_test
> +BUILD_TARGETS+=memory-failure/hwpoison-panic

I was initially thought why not underscore but dash on the name?  But, I
realize it is obviously too persoanl taste.

[...]
> diff --git a/tools/mm/memory-failure/hwpoison-panic.c b/tools/mm/memory-failure/hwpoison-panic.c
> new file mode 100644
> index 0000000000000..b8076d86da15b
> --- /dev/null
> +++ b/tools/mm/memory-failure/hwpoison-panic.c
> @@ -0,0 +1,353 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * hwpoison-panic: verify vm.panic_on_unrecoverable_memory_failure by
> + * injecting a hwpoison error on a kernel-owned page and confirming the
> + * kernel panics.

While reading the above explanation, I was expecting this program will directly
say me if the kernel panic-ed or not.  But this only says if it was not
panic-ed iiuc?  How about saying, like, "Trigger intentional kernel panic by
..." ?

No strong opinion.  I'm never a good English reader.

[...]
> +static const struct page_kind *parse_args(int argc, char **argv, int *force)
> +{
> +	const struct page_kind *kind;
> +	const char *name = "rodata";
> +	int c;
> +
> +	while ((c = getopt_long(argc, argv, "k:fh", opts, NULL)) != -1) {
> +		switch (c) {
> +		case 'k':
> +			name = optarg;
> +			break;
> +		case 'f':
> +			*force = 1;
> +			break;
> +		case 'h':
> +			usage();
> +			exit(EXIT_SUCCESS);
> +		default:
> +			usage();
> +			exit(EXIT_FAILURE);
> +		}
> +	}
> +
> +	kind = lookup_kind(name);
> +	if (!kind)
> +		fatal("unknown kind '%s' (expected: rodata|slab|pgtable)\n",
> +		      name);
> +
> +	return kind;
> +}
> +
> +static void check_prereqs(int force)
> +{
> +	if (geteuid())
> +		fatal("must run as root\n");
> +	if (access(SYSCTL_PATH, W_OK))
> +		fatal("%s not present (kernel without the sysctl?)\n",
> +		      SYSCTL_PATH);
> +	if (access(INJECT_PATH, W_OK))
> +		fatal("%s not present (no MEMORY_HOTPLUG?)\n", INJECT_PATH);
> +	if (!force)
> +		fatal("this panics the kernel; pass -f to run it in a disposable VM\n");

I personally feel like this option is not very intuitive to me.  I think the
name could be better to be intuitive.  Just removing the option also seems fine
to me.  If we really want to warn the brave root user, adding an interactive
prompt (E.g., "This will panic the kernel.  Proceed? [y/N]") also be an
intuitive and safer way in my opinion.

No strong opinion.  My user experience design skill is undoubtedly bad.  Feel
free to keep this as is.

> +}
[...]
> +int main(int argc, char **argv)
> +{
> +	unsigned long phys_addr, pfn, prior;
> +	const struct page_kind *kind;
> +	const char *verdict;
> +	int force = 0;
> +
> +	kind = parse_args(argc, argv, &force);
> +	page_size = getpagesize();
> +	check_prereqs(force);

Maybe check_prereqs() can be called before getpagesize()?

[...]


Thanks,
SJ