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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Infinite loop in memory-failure test on unexpected SIGBUS
--

--- Patch [5]: [PATCH v5 5/5] mm: selftests: Add shmem into memory failure test ---
commit b9a214a17a9d91396ae673c0a673788dd9d73afe
Author: Lisa Wang <[email protected]>

mm: selftests: Add shmem into memory failure test

Add a shmem memory failure selftest to test the shmem memory failure is
correct after modifying shmem return value.

> diff --git a/tools/testing/selftests/mm/memory-failure.c b/tools/testing/selftests/mm/memory-failure.c
> index 1a5a32e22cce2..5b26f0c446644 100644
> --- a/tools/testing/selftests/mm/memory-failure.c
> +++ b/tools/testing/selftests/mm/memory-failure.c
[ ... ]
> @@ -157,17 +166,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 reordering cause an infinite loop if an unexpected SIGBUS occurs?

Previously, the code checked ASSERT_EQ(setjmp, 0) before calling
check_memory(), allowing the test to fail gracefully.

With this change, check_memory() is called first. If an unexpected SIGBUS
was already received (setjmp == 1), check_memory() will access the address
again, triggering another SIGBUS. This causes the signal handler to siglongjmp
back to the sigsetjmp() site in the caller, which will then re-enter check()
and trigger the same sequence repeatedly.

> +		fallthrough;
> +	case MADV_HARD_CLEAN_SHMEM:
> +	case MADV_SOFT_CLEAN_SHMEM:
> +		/* It is not expected to receive a SIGBUS signal. */
> +		ASSERT_EQ(setjmp, 0);
>  
>  		/* The backing pfn of addr should have changed. */
>  		ASSERT_NE(pagemap_get_pfn(self->pagemap_fd, vaddr), self->pfn);
[ ... ]
> +TEST_F(memory_failure, dirty_shmem)
> +{
[ ... ]
> +	ret = sigsetjmp(signal_jmp_buf, 1);
> +	if (!ret && !self->injection_attempted) {
> +		self->injection_attempted = true;
> +		ASSERT_EQ(variant->inject(self, addr), 0);
> +	}
> +
> +	if (variant->type == MADV_HARD) {
> +		check(_metadata, self, addr, MADV_HARD_DIRTY_SHMEM, ret);
> +		ret = sigsetjmp(signal_jmp_buf, 1);
> +		if (ret == 0)
> +			FORCE_READ(*addr);
> +		check(_metadata, self, addr, READ_ERROR, ret);
> +	} else {
> +		check(_metadata, self, addr, MADV_SOFT_DIRTY_SHMEM, ret);

[Severity: Medium]
If the MADV_SOFT injection unexpectedly triggers a SIGBUS, will this else
block cause an infinite loop?

If a SIGBUS is caught during injection, sigsetjmp() returns 1, meaning
ret == 1. The else block is executed unconditionally, calling check() which now
accesses the memory before asserting setjmp == 0. This will trigger another
SIGBUS and siglongjmp back to the sigsetjmp() above, creating an infinite loop.

> +	}
> +
> +	ASSERT_EQ(munmap(addr, self->page_size), 0);
> +
> +	ASSERT_EQ(close(fd), 0);
> +}
> +
> +TEST_F(memory_failure, clean_shmem)
> +{
[ ... ]
> +	ret = sigsetjmp(signal_jmp_buf, 1);
> +	if (!ret && !self->injection_attempted) {
> +		self->injection_attempted = true;
> +		ASSERT_EQ(variant->inject(self, addr), 0);
> +	}
> +
> +	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]
Similar to the issue in dirty_shmem, if an unexpected SIGBUS occurs during
injection, ret will be 1, but this else block will unconditionally execute
FORCE_READ(*addr).

Since the address may be poisoned, FORCE_READ() will trigger another SIGBUS,
jumping back to the sigsetjmp() and creating an infinite loop. Should this
block check if ret == 0 before attempting to read the address?

> +		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/20260823-memory-failure-mf-delayed-fix-v5-0-d97a5703e45a@google.com?part=5