Re: [PATCH v6 5/5] mm: selftests: Add shmem into memory failure test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/mm: infinite loop on unexpected SIGBUS during memory-failure test
- [Low] selftests/mm: misspelled FALLTHROUGH comment breaks compiler warning suppression
--

commit 7c0289ba3b46e8635863454f24b73b9a63de8df1
Author: Lisa Wang <wyihan@google.com>

mm: selftests: Add shmem into memory failure test

This commit adds a shmem memory failure selftest to verify expected behaviors
when combining page dirtiness and failure types. It introduces new tests
for clean and dirty shmem pages to check if SIGBUS is correctly triggered
or suppressed.

> diff --git a/tools/testing/selftests/mm/memory-failure.c b/tools/testing/selftests/mm/memory-failure.c
> index 1a5a32e22cce2..d5d17a262c000 100644
> --- a/tools/testing/selftests/mm/memory-failure.c
> +++ b/tools/testing/selftests/mm/memory-failure.c

[ ... ]

> @@ -157,17 +162,22 @@ static void check(struct __test_metadata *_metadata, FIXTURE_DATA(memory_failure
>  	case MADV_HARD_CLEAN_PAGECACHE:
>  	case MADV_SOFT_CLEAN_PAGECACHE:
>  	case MADV_SOFT_DIRTY_PAGECACHE:
> -		/* It is not expected to receive a SIGBUS signal. */
> -		ASSERT_EQ(setjmp, 0);
> -
> +	case MADV_SOFT_DIRTY_SHMEM:
>  		/* The page content should remain unchanged. */
>  		ASSERT_TRUE(check_memory(vaddr, self->page_size));

[Severity: Medium]
Will this cause an infinite loop if an unexpected SIGBUS is caught? In
check(), check_memory() is now called before ASSERT_EQ(setjmp, 0). If
soft-offlining unexpectedly results in a SIGBUS, the signal handler jumps
back, but check_memory() will then re-trigger the SIGBUS before the setjmp
assertion can catch the error and fail the test.

> +		/* FALLTHORUGH */

[Severity: Low]
Is this a typo? The spelling FALLTHORUGH will fail to match the compiler's
regex for intentional fallthroughs and might generate a
-Wimplicit-fallthrough build warning.

> +	case MADV_HARD_CLEAN_SHMEM:
> +	case MADV_SOFT_CLEAN_SHMEM:
> +		/* It is not expected to receive a SIGBUS signal. */
> +		ASSERT_EQ(setjmp, 0);

[ ... ]

> @@ -355,4 +379,88 @@ TEST_F(memory_failure, dirty_pagecache)

[ ... ]

> +TEST_F(memory_failure, clean_shmem)
> +{

[ ... ]

> +	if (variant->type == MADV_HARD) {
> +		check(_metadata, self, addr, MADV_HARD_CLEAN_SHMEM, ret);
> +		ret = sigsetjmp(signal_jmp_buf, 1);
> +		if (ret == 0)
> +			FORCE_READ(*addr);
> +		check(_metadata, self, addr, READ_ERROR, ret);
> +	} else {
> +		/* Test the address accessability without check_memory(). */
> +		FORCE_READ(*addr);

[Severity: Medium]
Does executing FORCE_READ(*addr) unconditionally before validating the ret
value cause an infinite loop? 

If the test unexpectedly caught a SIGBUS during injection or a subsequent
access, the sigbus_handler jumps back to sigsetjmp() which sets ret = 1.
Executing FORCE_READ(*addr) without checking if ret == 0 triggers another
SIGBUS endlessly, rather than letting the test report a failure.

> +		check(_metadata, self, addr, MADV_SOFT_CLEAN_SHMEM, ret);
> +	}
> +
> +	ASSERT_EQ(munmap(addr, self->page_size), 0);
> +
> +	ASSERT_EQ(close(fd), 0);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917-memory-failure-mf-delayed-fix-v6-0-4b00856b5364@google.com?part=5