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 <[email protected]>
>> ---
>> 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
