Re: [PATCH v2] selftests/mm: Fix incorrect mmap() error handling with NULL instead of MAP_FAILED

[Hongfu Li]

Hi David,
Thanks for your review.

> > mmap() returns MAP_FAILED, which is defined as (void *)-1, on error,
> > not NULL.  Several selftests incorrectly check the return value of
> > mmap() using !ptr or ptr == NULL, which would erroneously treat
> > MAP_FAILED as a valid pointer since MAP_FAILED is non-zero and
> > non-NULL.  This can lead to segfaults when mmap() actually fails
> > under memory pressure.
> 
> Well, your patch also adds more checks where we previously didn't have any checks?

Yes, I added a few checks for the mmap return value, as they were not
handled previously. Perhaps I should also adjust the commit message to
match this change.

> > 
> > Signed-off-by: Hongfu Li <lihongfu@xxxxxxxxxx>
> > Reviewed-by: Dev Jain <dev.jain@xxxxxxx>
> > ---
> > v2:
> > - Add missing mmap() return value checks in pkey_sighandler_tests.c
> >   and protection_keys.c
> > ---
> 
> [...]
> 
> > diff --git a/tools/testing/selftests/mm/protection_keys.c b/tools/testing/selftests/mm/protection_keys.c
> > index 2085982dba69..580bf1668c71 100644
> > --- a/tools/testing/selftests/mm/protection_keys.c
> > +++ b/tools/testing/selftests/mm/protection_keys.c
> > @@ -1217,6 +1217,7 @@ static void arch_force_pkey_reg_init(void)
> >  	 * doing the XSAVE size enumeration dance.
> >  	 */
> >  	buf = mmap(NULL, 1*MB, PROT_READ|PROT_WRITE, MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
> > +	pkey_assert(buf != (void *)-1);
> 
> What's the reason for not using MAP_FAILED?

I tried to keep consistency with the existing mmap handling pattern in protection_keys.c.
That said, using MAP_FAILED would indeed be more appropriate.
I'll adjust this and send out a v3 shortly.

Best regards,
Hongfu