Re: [PATCH] selftests/mm: add missing mmap() return checks in pkey tests

[Hongfu Li]

Hi Lorenzo, 
Thanks for the review comments.

> Hmm you're sending this separete from the other MAP_FAILED checks, and not
> referencing that in any way? (original patch at [0]).
> 
> Please just send this as a 2 patch series _with a cover letter_ and both patches
> in-reply-to the cover letter.
> 
> Also make sure to propagate tags correctly.
> 
> [0]:https://lore.kernel.org/all/20260513095609.789935-1-lihongfu@xxxxxxxxxx/

The first patch has already been merged into the mm-new branch:
https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/commit/?h=mm-new&id=ffe64def0071989cff47b5525d38f5e558c637c3

For this reason, I split this one out separately to avoid confusion.

> On Mon, May 18, 2026 at 04:21:20PM +0800, Hongfu Li wrote:
> > Several mmap() calls lack error checks and would crash on failure.
> > Add the missing checks. Also replace bare (void *)-1 with the
> 
> Well you're assert()'ing so you're causing a crash on failure anyway?
> 
> I'd just say that you are adding missing checks against the mmap() return value,
> as well as improving readability and consistency by replacing (void *)-1 with
> MAP_FAILED in instances where that was used rather than MAP_FAILED.

Thanks for pointing this out, I will correct it in v2.

> > diff --git a/tools/testing/selftests/mm/pkey_sighandler_tests.c b/tools/testing/selftests/mm/pkey_sighandler_tests.c
> > index 302fef54049c..4637809192f9 100644
> > --- a/tools/testing/selftests/mm/pkey_sighandler_tests.c
> > +++ b/tools/testing/selftests/mm/pkey_sighandler_tests.c
> > @@ -317,6 +317,7 @@ static void test_sigsegv_handler_with_different_pkey_for_stack(void)
> >  	/* Set up alternate signal stack that will use the default MPK */
> >  	sigstack.ss_sp = mmap(0, STACK_SIZE, PROT_READ | PROT_WRITE,
> >  			      MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> > +	assert(sigstack.ss_sp != MAP_FAILED);
> 
> Why not pkey_assert()?
> 
> >  	sigstack.ss_flags = 0;
> >  	sigstack.ss_size = STACK_SIZE;
> >
> > @@ -490,6 +491,7 @@ static void test_pkru_sigreturn(void)
> >  	/* Set up alternate signal stack that will use the default MPK */
> >  	sigstack.ss_sp = mmap(0, STACK_SIZE, PROT_READ | PROT_WRITE,
> >  			      MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> > +	assert(sigstack.ss_sp != MAP_FAILED);
> 
> Why not pkey_assert()?

protection_keys.c executes numerous tests in loops across multiple iterations, 
so the test_nr and iteration_nr printed by pkey_assert help easily locate the 
exact failed test case and iteration.
In contrast, pkey_sighandler_tests.c consists of only a few standalone test 
functions invoked once each, so plain assert providing file and line information
should suffice to locate failures.

> > @@ -1775,7 +1776,7 @@ int main(void)
> >  		printf("running PKEY tests for unsupported CPU/OS\n");
> >
> >  		ptr  = mmap(NULL, size, PROT_NONE, MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
> > -		assert(ptr != (void *)-1);
> > +		assert(ptr != MAP_FAILED);
> 
> Probably best to convert to pkey_assert() at the same time?

This is a pre-test initialization path that runs before the test
loop, so test_nr and iteration_nr (used in pkey_assert for diagnostic
output) are not yet set up at this point.  
Would using plain assert() here be more appropriate?

Best regards,
Hongfu