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 <[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.
>
> 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.
.
