From: Lorenzo Stoakes <ljs@kernel.org>
To: Hongfu Li <lihongfu@kylinos.cn>
Cc: akpm@linux-foundation.org, david@kernel.org, liam@infradead.org,
vbabka@kernel.org, rppt@kernel.org, surenb@google.com,
mhocko@suse.com, shuah@kernel.org, linux-mm@kvack.org,
linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
Dev Jain <dev.jain@arm.com>
Subject: Re: [PATCH v2] selftests/mm: Fix incorrect mmap() error handling with NULL instead of MAP_FAILED
Date: Wed, 13 May 2026 11:57:59 +0100 [thread overview]
Message-ID: <agRX7BXxuv-VWqN4@lucifer> (raw)
In-Reply-To: <20260513095609.789935-1-lihongfu@kylinos.cn>
On Wed, May 13, 2026 at 05:56:09PM +0800, Hongfu Li wrote:
> 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.
>
> Signed-off-by: Hongfu Li <lihongfu@kylinos.cn>
> Reviewed-by: Dev Jain <dev.jain@arm.com>
You forgot my tag :)
Also, please do not send a respin right away if you can, multiple revisions in
one day can be tricky to deal with.
As per David's points - please update the commit message to reflect the fact
you've added a missing assert too and replace the (void *)-1 with MAP_FAILED.
Cheers, Lorenzo
> ---
> v2:
> - Add missing mmap() return value checks in pkey_sighandler_tests.c
> and protection_keys.c
> ---
> tools/testing/selftests/mm/ksm_tests.c | 2 +-
> tools/testing/selftests/mm/madv_populate.c | 2 +-
> tools/testing/selftests/mm/pkey_sighandler_tests.c | 2 ++
> tools/testing/selftests/mm/protection_keys.c | 1 +
> tools/testing/selftests/mm/soft-dirty.c | 4 ++--
> tools/testing/selftests/mm/vm_util.c | 2 +-
> 6 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/mm/ksm_tests.c b/tools/testing/selftests/mm/ksm_tests.c
> index a0b48b839d54..ed481f817282 100644
> --- a/tools/testing/selftests/mm/ksm_tests.c
> +++ b/tools/testing/selftests/mm/ksm_tests.c
> @@ -174,7 +174,7 @@ static void *allocate_memory(void *ptr, int prot, int mapping, char data, size_
> {
> void *map_ptr = mmap(ptr, map_size, PROT_WRITE, mapping, -1, 0);
>
> - if (!map_ptr) {
> + if (map_ptr == MAP_FAILED) {
> perror("mmap");
> return NULL;
> }
> diff --git a/tools/testing/selftests/mm/madv_populate.c b/tools/testing/selftests/mm/madv_populate.c
> index 88050e0f829a..7fce5d0b622b 100644
> --- a/tools/testing/selftests/mm/madv_populate.c
> +++ b/tools/testing/selftests/mm/madv_populate.c
> @@ -34,7 +34,7 @@ static void sense_support(void)
>
> addr = mmap(0, pagesize, PROT_READ | PROT_WRITE,
> MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
> - if (!addr)
> + if (addr == MAP_FAILED)
> ksft_exit_fail_msg("mmap failed\n");
>
> ret = madvise(addr, pagesize, MADV_POPULATE_READ);
> 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);
> 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);
> sigstack.ss_flags = 0;
> sigstack.ss_size = STACK_SIZE;
>
> 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);
Should use MAP_FAILED here as David pointed out.
>
> /* These __builtins require compiling with -mxsave */
>
> diff --git a/tools/testing/selftests/mm/soft-dirty.c b/tools/testing/selftests/mm/soft-dirty.c
> index bcfcac99b436..67c26c265880 100644
> --- a/tools/testing/selftests/mm/soft-dirty.c
> +++ b/tools/testing/selftests/mm/soft-dirty.c
> @@ -143,7 +143,7 @@ static void test_mprotect(int pagemap_fd, int pagesize, bool anon)
> if (anon) {
> map = mmap(NULL, pagesize, PROT_READ|PROT_WRITE,
> MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
> - if (!map)
> + if (map == MAP_FAILED)
> ksft_exit_fail_msg("anon mmap failed\n");
> } else {
> test_fd = open(fname, O_RDWR | O_CREAT, 0664);
> @@ -155,7 +155,7 @@ static void test_mprotect(int pagemap_fd, int pagesize, bool anon)
> ftruncate(test_fd, pagesize);
> map = mmap(NULL, pagesize, PROT_READ|PROT_WRITE,
> MAP_SHARED, test_fd, 0);
> - if (!map)
> + if (map == MAP_FAILED)
> ksft_exit_fail_msg("file mmap failed\n");
> }
>
> diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c
> index db94564f4431..63aaa2d9ec0b 100644
> --- a/tools/testing/selftests/mm/vm_util.c
> +++ b/tools/testing/selftests/mm/vm_util.c
> @@ -463,7 +463,7 @@ bool softdirty_supported(void)
> /* New mappings are expected to be marked with VM_SOFTDIRTY (sd). */
> addr = mmap(0, pagesize, PROT_READ | PROT_WRITE,
> MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
> - if (!addr)
> + if (addr == MAP_FAILED)
> ksft_exit_fail_msg("mmap failed\n");
>
> supported = check_vmflag(addr, "sd");
> --
> 2.25.1
>
next prev parent reply other threads:[~2026-05-13 10:58 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-13 9:56 [PATCH v2] selftests/mm: Fix incorrect mmap() error handling with NULL instead of MAP_FAILED Hongfu Li
2026-05-13 10:05 ` David Hildenbrand (Arm)
2026-05-13 11:15 ` Hongfu Li
2026-05-13 10:57 ` Lorenzo Stoakes [this message]
2026-05-13 11:20 ` Hongfu Li
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=agRX7BXxuv-VWqN4@lucifer \
--to=ljs@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=david@kernel.org \
--cc=dev.jain@arm.com \
--cc=liam@infradead.org \
--cc=lihongfu@kylinos.cn \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@suse.com \
--cc=rppt@kernel.org \
--cc=shuah@kernel.org \
--cc=surenb@google.com \
--cc=vbabka@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox