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

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

[Muhammad Usama Anjum]

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@arm.com>
---
 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);
+
+	close(self->kpageflags_fd);
+	close(self->pagemap_fd);
+	teardown_sighandler();
+}
+
 TEST_F(memory_failure, anon)
 {
 	char *addr;
@@ -242,8 +244,6 @@ TEST_F(memory_failure, anon)
 	else
 		check(_metadata, self, addr, MADV_SOFT_ANON, ret);
 
-	cleanup(_metadata, self, addr);
-
 	ASSERT_EQ(munmap(addr, self->page_size), 0);
 }
 
@@ -309,8 +309,6 @@ TEST_F(memory_failure, clean_pagecache)
 	else
 		check(_metadata, self, addr, MADV_SOFT_CLEAN_PAGECACHE, ret);
 
-	cleanup(_metadata, self, addr);
-
 	ASSERT_EQ(munmap(addr, self->page_size), 0);
 
 	ASSERT_EQ(close(fd), 0);
@@ -358,8 +356,6 @@ TEST_F(memory_failure, dirty_pagecache)
 	else
 		check(_metadata, self, addr, MADV_SOFT_DIRTY_PAGECACHE, ret);
 
-	cleanup(_metadata, self, addr);
-
 	ASSERT_EQ(munmap(addr, self->page_size), 0);
 
 	ASSERT_EQ(close(fd), 0);
-- 
2.47.3

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@arm.com>
> ---
>  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

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

[Muhammad Usama Anjum]

On 27/07/2026 1:31 pm, David Hildenbrand (Arm) wrote:
> 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@arm.com>
>> ---
>>  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?
Setting self->poisoned after variant->inject() returns would not cover the
MADV_HARD signal path.

The call path of madvise(MADV_HWPOISON) is:

madvise() -> madvise_do_behavior() -> madvise_inject_error(MF_ACTION_REQUIRED) ->
memory_failure() -> hwpoison_user_mappings() -> kill_procs(BUS_MCEERR_AR) ->
kill_proc() -> force_sig_mceerr()

SIGBUS is therefore queued while the madvise system call is executing and
delivered on return to userspace, before execution can continue after
variant->inject().

The test's sigbus_action() then calls siglongjmp(), so an assignment placed
after variant->inject() would be bypassed. The flag would remain false and
the test would attempt the injection again after returning to sigsetjmp().

Also, memory_failure() sets PG_hwpoison before several recovery paths that
can return an error, so a failed injection return does not prove that the
page was not poisoned.

Maybe renaming triggered to injection_attempted is more precise here.

-- 
Thanks,
Usama

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

[David Hildenbrand (Arm)]

On 7/28/26 16:22, Muhammad Usama Anjum wrote:
> On 27/07/2026 1:31 pm, David Hildenbrand (Arm) wrote:
>> 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@arm.com>
>>> ---
>>>  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?
> Setting self->poisoned after variant->inject() returns would not cover the
> MADV_HARD signal path.
> 
> The call path of madvise(MADV_HWPOISON) is:
> 
> madvise() -> madvise_do_behavior() -> madvise_inject_error(MF_ACTION_REQUIRED) ->
> memory_failure() -> hwpoison_user_mappings() -> kill_procs(BUS_MCEERR_AR) ->
> kill_proc() -> force_sig_mceerr()
> 
> SIGBUS is therefore queued while the madvise system call is executing and
> delivered on return to userspace, before execution can continue after
> variant->inject().
> 
> The test's sigbus_action() then calls siglongjmp(), so an assignment placed
> after variant->inject() would be bypassed. The flag would remain false and
> the test would attempt the injection again after returning to sigsetjmp().
> 
> Also, memory_failure() sets PG_hwpoison before several recovery paths that
> can return an error, so a failed injection return does not prove that the
> page was not poisoned.
> 
> Maybe renaming triggered to injection_attempted is more precise here.

Yes, that would make it clearer. + adding a comment that we want to cleanup even
when injection was attempted, but failed.

Thanks!

-- 
Cheers,

David

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

[Miaohe Lin]

On 2026/7/29 3:06, David Hildenbrand (Arm) wrote:
> On 7/28/26 16:22, Muhammad Usama Anjum wrote:
>> On 27/07/2026 1:31 pm, David Hildenbrand (Arm) wrote:
>>> 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@arm.com>
>>>> ---
>>>>  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?
>> Setting self->poisoned after variant->inject() returns would not cover the
>> MADV_HARD signal path.
>>
>> The call path of madvise(MADV_HWPOISON) is:
>>
>> madvise() -> madvise_do_behavior() -> madvise_inject_error(MF_ACTION_REQUIRED) ->
>> memory_failure() -> hwpoison_user_mappings() -> kill_procs(BUS_MCEERR_AR) ->
>> kill_proc() -> force_sig_mceerr()
>>
>> SIGBUS is therefore queued while the madvise system call is executing and
>> delivered on return to userspace, before execution can continue after
>> variant->inject().
>>
>> The test's sigbus_action() then calls siglongjmp(), so an assignment placed
>> after variant->inject() would be bypassed. The flag would remain false and
>> the test would attempt the injection again after returning to sigsetjmp().
>>
>> Also, memory_failure() sets PG_hwpoison before several recovery paths that
>> can return an error, so a failed injection return does not prove that the
>> page was not poisoned.
>>
>> Maybe renaming triggered to injection_attempted is more precise here.
> 
> Yes, that would make it clearer. + adding a comment that we want to cleanup even
> when injection was attempted, but failed.

self->triggered indicates whether the memory failure injection has been performed,
and it has the same meaning as injection_attempted. However, injection_attempted is
indeed clearer. So this change makes sense to me.

Thanks both.
.