Re: [PATCH v3 2/2] mm: selftests: Adjust the MADV_COLLAPSE uffd-minor selftests

[James Houghton]

On Mon, Sep 21, 2026 at 12:00 PM David Hildenbrand (Arm)
<david@xxxxxxxxxx> wrote:
>
> On 9/10/26 04:34, James Houghton wrote:
> > The behavior of MADV_COLLAPSE with uffd-minor VMAs has changed:
> > MADV_COLLAPSE will no longer install PTEs where none existed before.
>
> Misleading: MADV_COLLAPSE installed a PMD not PTEs?

Yeah, that's fair. I've updated it to this:

  The behavior of MADV_COLLAPSE with uffd-minor VMAs has changed.
  Previously, MADV_COLLAPSE would install PMDs in parts of a VMA where
  PTEs may be missing, essentially suppressing userfaultfd minor faults.
  Now, MADV_COLLAPSE returns -EINVAL for uffd-minor VMAs. Update the
  selftest to demonstrate this new behavior.

>
> > Update the selftest to demonstrate this new behavior.
> >
> > On an unpatched kernel, the test will hit "unexpected memory contents
> > after collapse".
> >
> > If the selftest is left unpatched but the kernel is patched, the
> > selftest will SKIP when it gets EINVAL back from MADV_COLLAPSE.
> >
> > Signed-off-by: James Houghton <jthoughton@xxxxxxxxxx>
> > ---
> >  tools/testing/selftests/mm/uffd-unit-tests.c | 75 ++++++++++++++++++--
> >  1 file changed, 68 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c
> > index ef9b3956bdcf..6f2360f9b75d 100644
> > --- a/tools/testing/selftests/mm/uffd-unit-tests.c
> > +++ b/tools/testing/selftests/mm/uffd-unit-tests.c
> > @@ -518,19 +518,34 @@ static void uffd_wp_fork_pin_with_event_test(uffd_global_test_opts_t *gopts, uff
> >       uffd_wp_fork_pin_test_common(gopts, args, true);
> >  }
> >
> > -static void check_memory_contents(uffd_global_test_opts_t *gopts, char *p)
> > +static int __check_memory_contents(unsigned long offset,
> > +                                unsigned long nr_pages,
> > +                                uffd_global_test_opts_t *gopts,
> > +                                char *p)
>
> Two tabs look better ;)

Fixed (I think).

>
> >  {
> >       unsigned long i, j;
> >       uint8_t expected_byte;
> >
> > -     for (i = 0; i < gopts->nr_pages; ++i) {
> > +     if (nr_pages + offset < nr_pages)
> > +             err("overflow in memory check");
> > +     if (nr_pages + offset > gopts->nr_pages)
> > +             err("out of bounds memory check");
> > +
> > +     for (i = offset; i < offset + nr_pages; ++i) {
> >               expected_byte = ~((uint8_t)(i % ((uint8_t)-1)));
> >               for (j = 0; j < gopts->page_size; j++) {
> >                       uint8_t v = *(uint8_t *)(p + (i * gopts->page_size) + j);
> >                       if (v != expected_byte)
> > -                             err("unexpected page contents");
> > +                             return 1;
> >               }
> >       }
> > +
> > +     return 0;
> > +}
> > +
> > +static int check_memory_contents(uffd_global_test_opts_t *gopts, char *p)
> > +{
> > +     return __check_memory_contents(0, gopts->nr_pages, gopts, p);
> >  }
> >
> >  static void uffd_minor_test_common(uffd_global_test_opts_t *gopts, bool test_collapse, bool test_wp)
> > @@ -539,6 +554,8 @@ static void uffd_minor_test_common(uffd_global_test_opts_t *gopts, bool test_col
> >       pthread_t uffd_mon;
> >       char c = '\0';
> >       struct uffd_args args = { 0 };
> > +     unsigned long checked = 0;
> > +     bool bad_contents;
>
> Through in an empty line. Or better
>
> struct uffd_args args = { .gopts = gopts };

Done. Thanks.

> >
> >       /*
> > @@ -564,24 +581,65 @@ static void uffd_minor_test_common(uffd_global_test_opts_t *gopts, bool test_col
> >       if (pthread_create(&uffd_mon, NULL, uffd_poll_thread, &args))
> >               err("uffd_poll_thread create");
> >
> > +     if (test_collapse) {
> > +             /*
> > +              * Read just a single page and try collapsing. The collapse
> > +              * should either be rejected or be a no-op.
> > +              */
> > +             if (__check_memory_contents(0, 1, gopts, gopts->area_dst_alias))
> > +                     err("unexpected memory contents before collapse");
> > +
> > +             /* MADV_COLLAPSE might return EINVAL for uffd-minor VMAs. */
> > +             madvise(gopts->area_dst_alias, gopts->nr_pages * gopts->page_size,
> > +                     MADV_COLLAPSE);
>
> Add an explicit check for expected errnos?

Done (just EINVAL).

>
> > +             /*
> > +              * If the above collapse mapped pages that were not explicitly
> > +              * CONTINUE'd, the below __check_memory_contents() will not
> > +              * fault on some pages, resulting in incorrect contents. The
> > +              * PTE for the first page may get retracted, so avoid checking
> > +              * that page, as we might take a second fault, flipping the
> > +              * contents a second time.
> > +              */
> > +             checked = 1;
>
> I'm not an expert on this code. But a trivial check to see whether there is
> something in the page tables that shouldn't be there is through the help of
> pagemap_is_populated().
>
> Maybe that would help to simplify things, not sure.

I've reworded the explanation here. I think it's better just to have
this `checked` stuff rather than checking `pagemap_is_populated()`.
Either way, we need to conditionally skip checking the first page,
that's the real annoyance.

New explanation/comment:

  If the above collapse mapped pages that were not explicitly
  CONTINUE'd, the below __check_memory_contents() will not
  fault on some pages, resulting in incorrect contents.

  In the correct case, when MADV_COLLAPSE doesn't map pages
  that were not mapped before, it may still retract existing
  PTEs for the region. Reaccesses to previously userfault'ed
  pages will trigger a *second* userfault and flip the
  contents again, which appears as a missed userfault. Ignore
  the page we already accessed to avoid this.