On Wed, Aug 13, 2025 at 1:39 PM Lorenzo Stoakes
<lorenzo.stoa...@oracle.com> wrote:
>
> 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 <sur...@google.com>
> > Tested-by: SeongJae Park <s...@kernel.org>
> > Acked-by: SeongJae Park <s...@kernel.org>
>
> The tests themselves look good, had a good look through. But I've given you
> some nice ASCII diagrams to sprinkle liberally around :)
Thanks for the commentary, Lorenzo, it is great! I think I'll post
them as a follow-up patch since they do not change the functionality
of the test.
>
> Anyway for tests themselves:
>
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoa...@oracle.com>
Thanks!
>
> > ---
> > 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
> >
