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/[email protected]/
> >
> > 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/[email protected]/
Sorry for the messy patch arrangement before, I'll respin this patch strictly
following your requirements.
> >
> > > 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().
Sure, I will use pkey_assert() and adjust it in the next revision.
> > (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!)
Sorry for that! I only use AI to polish my English expressions, and all the
ideas
are totally my own.
> >
> > > > @@ -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().
Got it. I will remove the assert here completely, add standard error reporting
and return EXIT_FAILURE directly on failure.
Thanks again for your patient guidance and rigorous review!
Best regards,
Hongfu
