Re: [PATCH v4 1/3] selftests/proc: test PROCMAP_QUERY ioctl while vma is concurrently modified

[Suren Baghdasaryan <surenb@google.com>]

On Wed, Aug 13, 2025 at 1:39 PM Lorenzo Stoakes
<lorenzo.stoakes@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 <surenb@google.com>
> > Tested-by: SeongJae Park <sj@kernel.org>
> > Acked-by: SeongJae Park <sj@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.stoakes@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
> >