Re: [PATCH v6 2/7] selftests: x86: test_mremap_vdso: skip if vdso is msealed

[Jeff Xu <jeffxu@chromium.org>]

On Mon, Feb 24, 2025 at 1:05 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> * jeffxu@chromium.org <jeffxu@chromium.org> [250224 12:45]:
> > From: Jeff Xu <jeffxu@chromium.org>
> >
> > Add code to detect if the vdso is memory sealed, skip the test
> > if it is.
>
> It also skips the test if fopen fails on smaps, but maybe that's super
> rare?
>
If we can't access /proc/pid/smaps, that's an "environment" issue
unrelated to this test, therefore skip().

> >
> > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > ---
> >  .../testing/selftests/x86/test_mremap_vdso.c  | 38 +++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> >
> > diff --git a/tools/testing/selftests/x86/test_mremap_vdso.c b/tools/testing/selftests/x86/test_mremap_vdso.c
> > index d53959e03593..c68077c56b22 100644
> > --- a/tools/testing/selftests/x86/test_mremap_vdso.c
> > +++ b/tools/testing/selftests/x86/test_mremap_vdso.c
> > @@ -14,6 +14,7 @@
> >  #include <errno.h>
> >  #include <unistd.h>
> >  #include <string.h>
> > +#include <stdbool.h>
> >
> >  #include <sys/mman.h>
> >  #include <sys/auxv.h>
> > @@ -55,13 +56,50 @@ static int try_to_remap(void *vdso_addr, unsigned long size)
> >
> >  }
> >
> > +#define VDSO_NAME "[vdso]"
> > +#define VMFLAGS "VmFlags:"
> > +#define MSEAL_FLAGS "sl"
> > +#define MAX_LINE_LEN 512
> > +
> > +bool vdso_sealed(FILE *maps)
> > +{
> > +     char line[MAX_LINE_LEN];
> > +     bool has_vdso = false;
> > +
> > +     while (fgets(line, sizeof(line), maps)) {
> > +             if (strstr(line, VDSO_NAME))
> > +                     has_vdso = true;
> > +
> > +             if (has_vdso && !strncmp(line, VMFLAGS, strlen(VMFLAGS))) {
> > +                     if (strstr(line, MSEAL_FLAGS))
> > +                             return true;
> > +
> > +                     return false;
> > +             }
> > +     }
> > +
> > +     return false;
> > +}
> > +
> >  int main(int argc, char **argv, char **envp)
> >  {
> >       pid_t child;
> > +     FILE *maps;
> >
> >       ksft_print_header();
> >       ksft_set_plan(1);
> >
> > +     maps = fopen("/proc/self/smaps", "r");
> > +     if (!maps) {
> > +             ksft_test_result_skip("Could not open /proc/self/smaps\n");
>
> fork() below prints errno, should this also print errno?
>
Sure.

> > +             return 0;
>
> I guess the logic here is that you might fail later because it's sealed
> but you don't know?  Is this rare enough not to matter?
>
yes. We should check the "sl" flag of vdso before continuing, or the
results will be unpredictable.

Android currently includes this selftest. In addition, you or someone
commented that  the selftest framework can randomly turn on kernel
config and run selftest, so it is necessary to add this check to skip.

> > +     }
> > +
> > +     if (vdso_sealed(maps)) {
> > +             ksft_test_result_skip("vdso is sealed\n");
> > +             return 0;
> > +     }
> > +
>
> This file also has an #ifdef __i386__ later,  Using it here would
> prevent unnecessary scanning of the maps file.  Probably not a big deal?
>
> Do you need to close the maps file?
>
OK, I will add close for good coding practice.

Thanks
-Jeff