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

[Lorenzo Stoakes <ljs@kernel.org>]

On Tue, May 19, 2026 at 05:16:26PM +0800, Hongfu Li wrote:
> 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@kylinos.cn/
>
> 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.

Hmm ok so you sent a v2 that was rejected [1], you were given feedback for a
respin but the v1 has been taken + not updated?... That's really not how the
process is supposed to work :/

Bit of a mess, Andrew - maybe best to keep the v1 then, and Hongfu - you can
respin this as requested?

[1]: https://lore.kernel.org/all/20260513095609.789935-1-lihongfu@kylinos.cn/

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

Why would we not want more information here? This argument doesn't hold any
water, please use pkey_assert().

(BTW This reads like an AI generated sentence. We're fine with you using AI to
assist with English for instance, but please make sure it's your own thoughts!)

>
> > > @@ -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?

OK that's gross, please just replace it with a test failure kmsg_xxx() whatever
it is, and a return EXIT_FAILURE; or something since you're in main().

>
> Best regards,
> Hongfu

Cheers, Lorenzo