Re: [PATCH] selftests/mm: unpoison pages in memory-failure teardown

[David Hildenbrand (Arm)]

On 7/27/26 11:54, Muhammad Usama Anjum wrote:
> The memory-failure tests call cleanup() only after all result checks.
> A failed ASSERT_* invokes fixture teardown and aborts the test, so it
> skips cleanup() and leaves the injected page hardware-poisoned.
> 
> Invoke cleanup() from FIXTURE_TEARDOWN() instead. Guard it with
> self->triggered so tests that exit before injection do not try to
> unpoison a page when no injection was attempted. This runs the existing
> HWPoison and HardwareCorrupted checks on both normal and assertion-failure
> paths.
> 
> Fixes: ff4ef2fbd101 ("selftests/mm: add memory failure anonymous page test")
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@xxxxxxx>
> ---
>  tools/testing/selftests/mm/memory-failure.c | 26 +++++++++------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/memory-failure.c b/tools/testing/selftests/mm/memory-failure.c
> index 5d00aab31f9b5..eaa8b9bd401a1 100644
> --- a/tools/testing/selftests/mm/memory-failure.c
> +++ b/tools/testing/selftests/mm/memory-failure.c
> @@ -122,13 +122,6 @@ static void teardown_sighandler(void)
>  	sigaction(SIGBUS, &sa, NULL);
>  }
>  
> -FIXTURE_TEARDOWN(memory_failure)
> -{
> -	close(self->kpageflags_fd);
> -	close(self->pagemap_fd);
> -	teardown_sighandler();
> -}
> -
>  static void prepare(struct __test_metadata *_metadata, FIXTURE_DATA(memory_failure) * self,
>  		    void *vaddr)
>  {
> @@ -200,8 +193,7 @@ static void check(struct __test_metadata *_metadata, FIXTURE_DATA(memory_failure
>  	ASSERT_EQ(pfn_flags & KPF_HWPOISON, KPF_HWPOISON);
>  }
>  
> -static void cleanup(struct __test_metadata *_metadata, FIXTURE_DATA(memory_failure) * self,
> -		    void *vaddr)
> +static void cleanup(struct __test_metadata *_metadata, FIXTURE_DATA(memory_failure) * self)
>  {
>  	unsigned long size;
>  	uint64_t pfn_flags;
> @@ -217,6 +209,16 @@ static void cleanup(struct __test_metadata *_metadata, FIXTURE_DATA(memory_failu
>  	ASSERT_EQ(size, self->corrupted_size);
>  }
>  
> +FIXTURE_TEARDOWN(memory_failure)
> +{
> +	if (self->triggered)
> +		cleanup(_metadata, self);

If the variant->inject(self, addr) fails, self->triggered would already have
been set.

Wouldn't it be cleaner to have a new self->poisoned that we set only after
->inject succeeded?

-- 
Cheers,

David