Re: [PATCH 1/3] selftests: vm: add process_mrelease tests

[Shuah Khan]

On 5/10/22 10:29 AM, Suren Baghdasaryan wrote:
> On Tue, May 10, 2022 at 8:43 AM Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx> wrote:
> > On 5/9/22 9:00 PM, Suren Baghdasaryan wrote:
> > > Introduce process_mrelease syscall sanity tests. They include tests of
> > > invalid pidfd and flags inputs, attempting to call process_mrelease
> > > with a live process and a valid usage of process_mrelease. Because
> > > process_mrelease has to be used against a process with a pending SIGKILL,
> > > it's possible that the process exits before process_mrelease gets called.
> > > In such cases we retry the test with a victim that allocates twice more
> > > memory up to 1GB. This would require the victim process to spend more
> > > time during exit and process_mrelease has a better chance of catching
> > > the process before it exits.
> >
> > +1 on Mike's comments on improving the change log. List what is getting
> > tested as opposed to describing the test code.
>
> I'll try to improve the description but IMHO it does describe what
> it's testing - the process_mrelease syscall with valid and invalid
> inputs. I could omit the implementation details if that helps.
>
> > > Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > > ---
> > >    tools/testing/selftests/vm/Makefile        |   1 +
> > >    tools/testing/selftests/vm/mrelease_test.c | 176 +++++++++++++++++++++
> > >    tools/testing/selftests/vm/run_vmtests.sh  |  16 ++
> > >    3 files changed, 193 insertions(+)
> > >    create mode 100644 tools/testing/selftests/vm/mrelease_test.c
> >
> > Please update .gitignore with the new executable.
>
> Ack.
>
> > > diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
> > > index 04a49e876a46..733fccbff0ef 100644
> > > --- a/tools/testing/selftests/vm/Makefile
> > > +++ b/tools/testing/selftests/vm/Makefile
> > > @@ -43,6 +43,7 @@ TEST_GEN_FILES += map_populate
> > >    TEST_GEN_FILES += memfd_secret
> > >    TEST_GEN_FILES += mlock-random-test
> > >    TEST_GEN_FILES += mlock2-tests
> > > +TEST_GEN_FILES += mrelease_test
> > >    TEST_GEN_FILES += mremap_dontunmap
> > >    TEST_GEN_FILES += mremap_test
> > >    TEST_GEN_FILES += on-fault-limit
> > > diff --git a/tools/testing/selftests/vm/mrelease_test.c b/tools/testing/selftests/vm/mrelease_test.c
> > > new file mode 100644
> > > index 000000000000..a61061bf8433
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/vm/mrelease_test.c
> > > @@ -0,0 +1,176 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright 2022 Google LLC
> > > + */
> > > +#define _GNU_SOURCE
> > > +#include <errno.h>
> > > +#include <stdio.h>
> > > +#include <stdlib.h>
> > > +#include <sys/wait.h>
> > > +#include <unistd.h>
> > > +
> > > +#include "util.h"
> > > +
> > > +static inline int pidfd_open(pid_t pid, unsigned int flags)
> > > +{
> > > +#ifdef __NR_pidfd_open
> > > +     return syscall(__NR_pidfd_open, pid, flags);
> > > +#else
> > > +     errno = ENOSYS;
> >
> > This isn't an error - this would be skip because this syscall
> > isn't supported.
>
> Ack.
>
> > > +     return -1;
> > > +#endif
> >
> > Key off of syscall return instead of these ifdefs - same comment
> > on all of the ifdefs
>
> Ack. I was using some other test as an example but I guess that was
> not a good model.
>
> > > +}
> > > +
> >
> > I am not seeing any reason for breaking this code up have a separate
> > routine for pidfd_open().
>
> I'm a bit unclear what you mean. Do you mean that userspace headers
> should already define pidfd_open() and I don't need to define it?

Do you need pidfd_open() or can this be part of main? Without the ifdefs,
it is really a one line code.

thanks,
-- Shuah