| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7d3b4b0c-f905-4622-95a8-e4d076dc71d4@lucifer.local>
Date: Wed, 13 Aug 2025 14:38:47 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Suren Baghdasaryan <surenb@...gle.com>
Cc: akpm@...ux-foundation.org, Liam.Howlett@...cle.com, david@...hat.com,
vbabka@...e.cz, peterx@...hat.com, jannh@...gle.com,
hannes@...xchg.org, mhocko@...nel.org, paulmck@...nel.org,
shuah@...nel.org, adobriyan@...il.com, brauner@...nel.org,
josef@...icpanda.com, yebin10@...wei.com, linux@...ssschuh.net,
willy@...radead.org, osalvador@...e.de, andrii@...nel.org,
ryan.roberts@....com, christophe.leroy@...roup.eu,
tjmercier@...gle.com, kaleshsingh@...gle.com, aha310510@...il.com,
linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-mm@...ck.org, linux-kselftest@...r.kernel.org,
SeongJae Park <sj@...nel.org>
Subject: Re: [PATCH v4 1/3] selftests/proc: test PROCMAP_QUERY ioctl while
vma is concurrently modified
On Fri, Aug 08, 2025 at 08:28:47AM -0700, Suren Baghdasaryan wrote:
> Extend /proc/pid/maps tearing tests to verify PROCMAP_QUERY ioctl operation
> correctness while the vma is being concurrently modified.
>
General comment, but I really feel like this stuff is mm-specific. Yes it uses
proc, but it's using it to check for mm functionality.
I mean I'd love for these to be in the mm self tests but I get obviously why
they're in the proc ones...
> Signed-off-by: Suren Baghdasaryan <surenb@...gle.com>
> Tested-by: SeongJae Park <sj@...nel.org>
> Acked-by: SeongJae Park <sj@...nel.org>
The tests themselves look good, had a good look through. But I've given you
some nice ASCII diagrams to sprinkle liberally around :)
Anyway for tests themselves:
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> ---
> tools/testing/selftests/proc/proc-maps-race.c | 65 +++++++++++++++++++
> 1 file changed, 65 insertions(+)
>
> diff --git a/tools/testing/selftests/proc/proc-maps-race.c b/tools/testing/selftests/proc/proc-maps-race.c
> index 94bba4553130..a546475db550 100644
> --- a/tools/testing/selftests/proc/proc-maps-race.c
> +++ b/tools/testing/selftests/proc/proc-maps-race.c
> @@ -32,6 +32,8 @@
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
> +#include <linux/fs.h>
> +#include <sys/ioctl.h>
> #include <sys/mman.h>
> #include <sys/stat.h>
> #include <sys/types.h>
> @@ -317,6 +319,25 @@ static bool capture_mod_pattern(FIXTURE_DATA(proc_maps_race) *self,
> strcmp(restored_first_line->text, self->first_line.text) == 0;
> }
>
> +static bool query_addr_at(int maps_fd, void *addr,
> + unsigned long *vma_start, unsigned long *vma_end)
> +{
> + struct procmap_query q;
> +
> + memset(&q, 0, sizeof(q));
> + q.size = sizeof(q);
> + /* Find the VMA at the split address */
> + q.query_addr = (unsigned long long)addr;
> + q.query_flags = 0;
> + if (ioctl(maps_fd, PROCMAP_QUERY, &q))
> + return false;
> +
> + *vma_start = q.vma_start;
> + *vma_end = q.vma_end;
> +
> + return true;
> +}
> +
> static inline bool split_vma(FIXTURE_DATA(proc_maps_race) *self)
> {
> return mmap(self->mod_info->addr, self->page_size, self->mod_info->prot | PROT_EXEC,
> @@ -559,6 +580,8 @@ TEST_F(proc_maps_race, test_maps_tearing_from_split)
> do {
> bool last_line_changed;
> bool first_line_changed;
> + unsigned long vma_start;
> + unsigned long vma_end;
>
> ASSERT_TRUE(read_boundary_lines(self, &new_last_line, &new_first_line));
>
> @@ -595,6 +618,19 @@ TEST_F(proc_maps_race, test_maps_tearing_from_split)
> first_line_changed = strcmp(new_first_line.text, self->first_line.text) != 0;
> ASSERT_EQ(last_line_changed, first_line_changed);
>
> + /* Check if PROCMAP_QUERY ioclt() finds the right VMA */
Typo ioclt -> ioctl.
I think a little misleading, we're just testing whether we find a VMA at
mod_info->addr + self->page_size.
> + ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
> + &vma_start, &vma_end));
> + /*
> + * The vma at the split address can be either the same as
> + * original one (if read before the split) or the same as the
> + * first line in the second page (if read after the split).
> + */
> + ASSERT_TRUE((vma_start == self->last_line.start_addr &&
> + vma_end == self->last_line.end_addr) ||
> + (vma_start == split_first_line.start_addr &&
> + vma_end == split_first_line.end_addr));
> +
So I'd make things clearer here with a comment like:
We are mmap()'ing a distinct VMA over the start of a 3 page
mapping, which will cause the first page to be unmapped, and we can
observe two states:
read
|
v
|---------|------------------|
| | |
| A | B | or:
| | |
|---------|------------------|
|
v
|----------------------------|
| |
| A |
| |
|----------------------------|
If we see entries in /proc/$pid/maps it'll be:
7fa86aa15000-7fa86aa16000 rw-p 00000000 00:00 0 (A)
7fa86aa16000-7fa86aa18000 rw-p 00000000 00:00 0 (B)
Or:
7fa86aa15000-7fa86aa18000 rw-p 00000000 00:00 0 (A)
So we assert that the reported range is equivalent to one of these.
Obviously you can mix this in where you feel it makes sense.
> clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
> end_test_iteration(&end_ts, self->verbose);
> } while (end_ts.tv_sec - start_ts.tv_sec < self->duration_sec);
> @@ -636,6 +672,9 @@ TEST_F(proc_maps_race, test_maps_tearing_from_resize)
> clock_gettime(CLOCK_MONOTONIC_COARSE, &start_ts);
> start_test_loop(&start_ts, self->verbose);
> do {
> + unsigned long vma_start;
> + unsigned long vma_end;
> +
> ASSERT_TRUE(read_boundary_lines(self, &new_last_line, &new_first_line));
>
> /* Check if we read vmas after shrinking it */
> @@ -662,6 +701,16 @@ TEST_F(proc_maps_race, test_maps_tearing_from_resize)
> "Expand result invalid", self));
> }
>
> + /* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> + ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr, &vma_start, &vma_end));
Same comments as above.
> + /*
> + * The vma should stay at the same address and have either the
> + * original size of 3 pages or 1 page if read after shrinking.
> + */
> + ASSERT_TRUE(vma_start == self->last_line.start_addr &&
> + (vma_end - vma_start == self->page_size * 3 ||
> + vma_end - vma_start == self->page_size));
So I'd make things clearer here with a comment like:
We are shrinking and expanding a VMA from 1 page to 3 pages:
read
|
v
|---------|
| |
| A |
| |
|---------|
|
v
|----------------------------|
| |
| A |
| |
|----------------------------|
If we see entries in /proc/$pid/maps it'll be:
7fa86aa15000-7fa86aa16000 rw-p 00000000 00:00 0 (A)
Or:
7fa86aa15000-7fa86aa18000 rw-p 00000000 00:00 0 (A)
So we assert that the reported range is equivalent to one of these.
> +
> clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
> end_test_iteration(&end_ts, self->verbose);
> } while (end_ts.tv_sec - start_ts.tv_sec < self->duration_sec);
> @@ -703,6 +752,9 @@ TEST_F(proc_maps_race, test_maps_tearing_from_remap)
> clock_gettime(CLOCK_MONOTONIC_COARSE, &start_ts);
> start_test_loop(&start_ts, self->verbose);
> do {
> + unsigned long vma_start;
> + unsigned long vma_end;
> +
> ASSERT_TRUE(read_boundary_lines(self, &new_last_line, &new_first_line));
>
> /* Check if we read vmas after remapping it */
> @@ -729,6 +781,19 @@ TEST_F(proc_maps_race, test_maps_tearing_from_remap)
> "Remap restore result invalid", self));
> }
>
> + /* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> + ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
> + &vma_start, &vma_end));
Same comments as above.
> + /*
> + * The vma should either stay at the same address and have the
> + * original size of 3 pages or we should find the remapped vma
> + * at the remap destination address with size of 1 page.
> + */
> + ASSERT_TRUE((vma_start == self->last_line.start_addr &&
> + vma_end - vma_start == self->page_size * 3) ||
> + (vma_start == self->last_line.start_addr + self->page_size &&
> + vma_end - vma_start == self->page_size));
> +
Again be good to have more explanation here, similar comments to abov.
We are mremap()'ing the last page of the next VMA (B) into the
midle of the current one (A) (using MREMAP_DONTUNMAP leaving the
last page of the original VMA zapped but in place:
read
|
v R/W R/O
|----------------------------| |------------------.---------|
| | | . |
| A | | B . |
| | | . |
|----------------------------| |------------------.---------|
This will unmap the middle of A, splitting it in two, before
placing a copy of B there (Which has different prot bits than A):
|
v R/W R/O R/W R/O
|---------|---------|--------| |----------------------------|
| | | | | |
| A1 | B2 | A2 | | B |
| | | | | |
|---------|---------|--------| |----------------------------|
But then we 'patch' B2 back to R/W prot bits, causing B2 to get
merged:
|
v R/W R/O
|----------------------------| |----------------------------|
| | | |
| A | | B |
| | | |
|----------------------------| |----------------------------|
If we see entries in /proc/$pid/maps it'll be:
7fa86aa15000-7fa86aa18000 rw-p 00000000 00:00 0 (A)
7fa86aa19000-7fa86aa20000 r--p 00000000 00:00 0 (B)
Or:
7fa86aa15000-7fa86aa16000 rw-p 00000000 00:00 0 (A1)
7fa86aa16000-7fa86aa17000 r--p 00000000 00:00 0 (B2)
7fa86aa17000-7fa86aa18000 rw-p 00000000 00:00 0 (A3)
7fa86aa19000-7fa86aa20000 r--p 00000000 00:00 0 (B)
We are always examining the first line, so we simply assert that
this remains in place and we observe 1 page or 3 pages.
> clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
> end_test_iteration(&end_ts, self->verbose);
> } while (end_ts.tv_sec - start_ts.tv_sec < self->duration_sec);
> --
> 2.50.1.703.g449372360f-goog
>
Powered by blists - more mailing lists