[PATCH] selftests/vm: fix va_128TBswitch.sh permissions

[PATCH] selftests/vm: fix va_128TBswitch.sh permissions

[Adam Sindelar]

Restores the +x bit to va_128TBswitch.sh, which got dropped from the
previous patch, somehow.

Fixes: 1afd01d43efc3 ("selftests/vm: Only run 128TBswitch with 5-level
paging")

Signed-off-by: Adam Sindelar <adam@wowsignal.io>
---
 tools/testing/selftests/vm/va_128TBswitch.sh | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 mode change 100644 => 100755 tools/testing/selftests/vm/va_128TBswitch.sh

diff --git a/tools/testing/selftests/vm/va_128TBswitch.sh b/tools/testing/selftests/vm/va_128TBswitch.sh
old mode 100644
new mode 100755
-- 
2.35.1

Re: [PATCH] selftests/vm: fix va_128TBswitch.sh permissions

[Andrew Morton]

On Fri,  8 Jul 2022 11:06:46 +0200 Adam Sindelar <adam@wowsignal.io> wrote:

> Restores the +x bit to va_128TBswitch.sh, which got dropped from the
> previous patch, somehow.
> 
> Fixes: 1afd01d43efc3 ("selftests/vm: Only run 128TBswitch with 5-level
> paging")
> 
> Signed-off-by: Adam Sindelar <adam@wowsignal.io>
> ---
>  tools/testing/selftests/vm/va_128TBswitch.sh | 0
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  mode change 100644 => 100755 tools/testing/selftests/vm/va_128TBswitch.sh
> 
> diff --git a/tools/testing/selftests/vm/va_128TBswitch.sh b/tools/testing/selftests/vm/va_128TBswitch.sh
> old mode 100644
> new mode 100755

Half of tools/testing/selftests/vm/*.sh don't have the x bit set. 
They're invoked via `/bin/sh foo.sh', which is more robust.

Can we hunt down and fix the invoking code?  Might be as simple as

--- a/tools/testing/selftests/vm/run_vmtests.sh~a
+++ a/tools/testing/selftests/vm/run_vmtests.sh
@@ -144,7 +144,7 @@ run_test() {
 		local sep=$(echo -n "$title" | tr "[:graph:][:space:]" -)
 		printf "%s\n%s\n%s\n" "$sep" "$title" "$sep"
 
-		"$@"
+		/bin/sh "$@"
 		local ret=$?
 		if [ $ret -eq 0 ]; then
 			echo "[PASS]"
_

Re: [PATCH] selftests/vm: fix va_128TBswitch.sh permissions

[Adam Sindelar]

On Fri, Jul 08, 2022 at 01:08:01PM -0700, Andrew Morton wrote:
> On Fri,  8 Jul 2022 11:06:46 +0200 Adam Sindelar <adam@wowsignal.io> wrote:
> 
> > Restores the +x bit to va_128TBswitch.sh, which got dropped from the
> > previous patch, somehow.
> > 
> > Fixes: 1afd01d43efc3 ("selftests/vm: Only run 128TBswitch with 5-level
> > paging")
> > 
> > Signed-off-by: Adam Sindelar <adam@wowsignal.io>
> > ---
> >  tools/testing/selftests/vm/va_128TBswitch.sh | 0
> >  1 file changed, 0 insertions(+), 0 deletions(-)
> >  mode change 100644 => 100755 tools/testing/selftests/vm/va_128TBswitch.sh
> > 
> > diff --git a/tools/testing/selftests/vm/va_128TBswitch.sh b/tools/testing/selftests/vm/va_128TBswitch.sh
> > old mode 100644
> > new mode 100755
> 
> Half of tools/testing/selftests/vm/*.sh don't have the x bit set. 
> They're invoked via `/bin/sh foo.sh', which is more robust.
> 
> Can we hunt down and fix the invoking code?  Might be as simple as
> 
> --- a/tools/testing/selftests/vm/run_vmtests.sh~a
> +++ a/tools/testing/selftests/vm/run_vmtests.sh
> @@ -144,7 +144,7 @@ run_test() {
>  		local sep=$(echo -n "$title" | tr "[:graph:][:space:]" -)
>  		printf "%s\n%s\n%s\n" "$sep" "$title" "$sep"
>  
> -		"$@"
> +		/bin/sh "$@"
>  		local ret=$?
>  		if [ $ret -eq 0 ]; then
>  			echo "[PASS]"
> _
> 

I think that would impose the choice of shell on the test scripts. About
half of them start with '#!/bin/sh' and the other half with
'#!/bin/bash'.

Maybe that's something we'd want to do anyway, but it seems like it
could have subtle and unintended side effects if the goal is to fix a
failing test.

(It would also invoke the ELF binaries through /bin/sh, but that
probably doesn't matter, since sh will I think exec right away.)

Re: [PATCH] selftests/vm: fix va_128TBswitch.sh permissions

[Adam Sindelar]

On Sat, Jul 09, 2022 at 10:14:27AM +0200, Adam Sindelar wrote:
> On Fri, Jul 08, 2022 at 01:08:01PM -0700, Andrew Morton wrote:
> > On Fri,  8 Jul 2022 11:06:46 +0200 Adam Sindelar <adam@wowsignal.io> wrote:
> > 
> > > Restores the +x bit to va_128TBswitch.sh, which got dropped from the
> > > previous patch, somehow.
> > > 
> > > Fixes: 1afd01d43efc3 ("selftests/vm: Only run 128TBswitch with 5-level
> > > paging")
> > > 
> > > Signed-off-by: Adam Sindelar <adam@wowsignal.io>
> > > ---
> > >  tools/testing/selftests/vm/va_128TBswitch.sh | 0
> > >  1 file changed, 0 insertions(+), 0 deletions(-)
> > >  mode change 100644 => 100755 tools/testing/selftests/vm/va_128TBswitch.sh
> > > 
> > > diff --git a/tools/testing/selftests/vm/va_128TBswitch.sh b/tools/testing/selftests/vm/va_128TBswitch.sh
> > > old mode 100644
> > > new mode 100755
> > 
> > Half of tools/testing/selftests/vm/*.sh don't have the x bit set. 
> > They're invoked via `/bin/sh foo.sh', which is more robust.
> > 
> > Can we hunt down and fix the invoking code?  Might be as simple as
> > 
> > --- a/tools/testing/selftests/vm/run_vmtests.sh~a
> > +++ a/tools/testing/selftests/vm/run_vmtests.sh
> > @@ -144,7 +144,7 @@ run_test() {
> >  		local sep=$(echo -n "$title" | tr "[:graph:][:space:]" -)
> >  		printf "%s\n%s\n%s\n" "$sep" "$title" "$sep"
> >  
> > -		"$@"
> > +		/bin/sh "$@"
> >  		local ret=$?
> >  		if [ $ret -eq 0 ]; then
> >  			echo "[PASS]"
> > _
> > 
> 
> I think that would impose the choice of shell on the test scripts. About
> half of them start with '#!/bin/sh' and the other half with
> '#!/bin/bash'.
> 
> Maybe that's something we'd want to do anyway, but it seems like it
> could have subtle and unintended side effects if the goal is to fix a
> failing test.
> 
> (It would also invoke the ELF binaries through /bin/sh, but that
> probably doesn't matter, since sh will I think exec right away.)
> 

Having thought about it: invoking the tests using your draft fails to
run the tests that are ELF binaries. `/bin/sh -c "@"` does run everything
but doesn't pass arguments properly. (On my system imposing /bin/sh over
/bin/bash doesn't matter, but I think it's possible for /bin/sh and
whatever shell a script has in its shebang to be incompatible, e.g. with
zsh.)

This might be my own ignorance, but I don't see an obvious way to make
the invoking code correctly handle all tests without branching based on
the contents of the file. We could look inside and act on the shebang,
but then we're reimplementing execve.

To advance a counterargument: as you say, not all *.sh files in
tools/testing/selftests/vm are executable, but all test programs
(arguments to `run_test`) are executable. Currently the test programs
are treated the same whether they're ELF binaries of shell scripts.
Having some test programs not be executable would require treating some
test programs differently from others for the first time.

Re: [PATCH] selftests/vm: fix va_128TBswitch.sh permissions

[David Vernet]

On Fri, Jul 08, 2022 at 01:08:01PM -0700, Andrew Morton wrote:
> On Fri,  8 Jul 2022 11:06:46 +0200 Adam Sindelar <adam@wowsignal.io> wrote:
> 
> > Restores the +x bit to va_128TBswitch.sh, which got dropped from the
> > previous patch, somehow.
> > 
> > Fixes: 1afd01d43efc3 ("selftests/vm: Only run 128TBswitch with 5-level
> > paging")
> > 
> > Signed-off-by: Adam Sindelar <adam@wowsignal.io>
> > ---
> >  tools/testing/selftests/vm/va_128TBswitch.sh | 0
> >  1 file changed, 0 insertions(+), 0 deletions(-)
> >  mode change 100644 => 100755 tools/testing/selftests/vm/va_128TBswitch.sh
> > 
> > diff --git a/tools/testing/selftests/vm/va_128TBswitch.sh b/tools/testing/selftests/vm/va_128TBswitch.sh
> > old mode 100644
> > new mode 100755
> 
> Half of tools/testing/selftests/vm/*.sh don't have the x bit set. 
> They're invoked via `/bin/sh foo.sh', which is more robust.
> 
> Can we hunt down and fix the invoking code?  Might be as simple as
> 
> --- a/tools/testing/selftests/vm/run_vmtests.sh~a
> +++ a/tools/testing/selftests/vm/run_vmtests.sh
> @@ -144,7 +144,7 @@ run_test() {
>  		local sep=$(echo -n "$title" | tr "[:graph:][:space:]" -)
>  		printf "%s\n%s\n%s\n" "$sep" "$title" "$sep"
>  
> -		"$@"
> +		/bin/sh "$@"
>  		local ret=$?

So, I think tools/testing/selftests/vm/ is a bit interesting in that
run_vmtests.sh kind of circumvents the "standard" model for running
kselftests.  In the "standard" model (which I'm putting in quotes because
there's no formal, prescriptive way to use kselftests), testcases are
specified as Makefile targets that are put into one of several possible
Make variables to inform how they should be built and run.  This is
described in more details in [0].

[0]: https://docs.kernel.org/dev-tools/kselftest.html?highlight=kselftest#contributing-new-tests-details

As it relates to vmtests, we have the following relevant variables:

TEST_GEN_FILES
--------------

Contains targets that generate executable ELF binaries that are _not_
automatically run by the kselftests runner, but which are built. These are
the ELF executables that we run in run_vmtests.sh, sometimes with multiple
invocations using different parameters (for e.g. ksm_tests).

TEST_GEN_PROGS
--------------

Contains targets that generate executable ELF binaries that _are_
automatically run by the kselftest runner. Note that some of the executable
targets in the vm suite are not included in run_vmtests.sh, e.g.
soft-dirty, and split_huge_page_test. madv_populate actually is, which
looks like a bug to me (which was introduced in 749d999c06c2 ("selftests:
vm: bring common functions to a new file") and which I'll send out a fix
for).

TEST_PROGS
----------

Contains test shell scripts. These are automatically invoked by th
kselftest runner as well, and (as far as I understand) from the perspective
of kselftest, are the exact same as TEST_GEN_PROGS with the exception that
of course they're not compiled. In the kselftest runner, if the executable
bit is not set, the runner will echo out a warning and then try to parse
the interpreter from the first line of the file to figure out how to invoke
it.

My two cents are that the correct way to structure vm selftests is
something like the following:

1. For any testcase that's compiled directly into an ELF binary, and which
   doesn't need to be invoked with any arguments, convert it to a
   TEST_GEN_PROGS target in tools/testing/selftests/vm/Makefile, and remove
   it from being invoked from run_vmtests.sh.
2. For any testcase that's compiled directly into an ELF binary, but which
   _does_ need to be invoked with multiple arguments and/or needs to be
   invoked in some other special way such as va_128TBswitch, leave it in
   TEST_GEN_FILES, and instead add a separate executable shell script wraps
   it, and which lives in TEST_PROGS. This would be a nice cleanup, because
   it would let us remove some of the custom logic at the top of
   run_vmtests.sh that only applies to some of the testcases.
3. Once (1) and (2) are done for all relevant targets, remove
   run_vmtests.sh and instead rely on the kselftest runner.

This would allow us to remove all custom runner logic from
tools/testing/selftests/vm, and to instead just rely on the kselftest
runner to do the heavy lifting for us.

As it relates specifically to Adam's patch, IMO setting that script include
the executable bit does make sense, as it's in-line with the guidance for
kselftests in general, and I don't think we can assume that all of the
shell scripts will be bin/sh.

FWIW, I'm happy to do the above cleanup if nobody objects (after Adam's
patch lands).

Thanks,
David

Re: [PATCH] selftests/vm: fix va_128TBswitch.sh permissions

[Adam Sindelar]

On Tue, Jul 12, 2022 at 09:17:45AM -0700, David Vernet wrote:
> On Fri, Jul 08, 2022 at 01:08:01PM -0700, Andrew Morton wrote:
> > On Fri,  8 Jul 2022 11:06:46 +0200 Adam Sindelar <adam@wowsignal.io> wrote:
> > 
> > > Restores the +x bit to va_128TBswitch.sh, which got dropped from the
> > > previous patch, somehow.
> > > 
> > > Fixes: 1afd01d43efc3 ("selftests/vm: Only run 128TBswitch with 5-level
> > > paging")
> > > 
> > > Signed-off-by: Adam Sindelar <adam@wowsignal.io>
> > > ---
> > >  tools/testing/selftests/vm/va_128TBswitch.sh | 0
> > >  1 file changed, 0 insertions(+), 0 deletions(-)
> > >  mode change 100644 => 100755 tools/testing/selftests/vm/va_128TBswitch.sh
> > > 
> > > diff --git a/tools/testing/selftests/vm/va_128TBswitch.sh b/tools/testing/selftests/vm/va_128TBswitch.sh
> > > old mode 100644
> > > new mode 100755
> > 
> > Half of tools/testing/selftests/vm/*.sh don't have the x bit set. 
> > They're invoked via `/bin/sh foo.sh', which is more robust.
> > 
> > Can we hunt down and fix the invoking code?  Might be as simple as
> > 
> > --- a/tools/testing/selftests/vm/run_vmtests.sh~a
> > +++ a/tools/testing/selftests/vm/run_vmtests.sh
> > @@ -144,7 +144,7 @@ run_test() {
> >  		local sep=$(echo -n "$title" | tr "[:graph:][:space:]" -)
> >  		printf "%s\n%s\n%s\n" "$sep" "$title" "$sep"
> >  
> > -		"$@"
> > +		/bin/sh "$@"
> >  		local ret=$?
> 
> So, I think tools/testing/selftests/vm/ is a bit interesting in that
> run_vmtests.sh kind of circumvents the "standard" model for running
> kselftests.  In the "standard" model (which I'm putting in quotes because
> there's no formal, prescriptive way to use kselftests), testcases are
> specified as Makefile targets that are put into one of several possible
> Make variables to inform how they should be built and run.  This is
> described in more details in [0].
> 
> [0]: https://docs.kernel.org/dev-tools/kselftest.html?highlight=kselftest#contributing-new-tests-details
> 
> As it relates to vmtests, we have the following relevant variables:
> 
> TEST_GEN_FILES
> --------------
> 
> Contains targets that generate executable ELF binaries that are _not_
> automatically run by the kselftests runner, but which are built. These are
> the ELF executables that we run in run_vmtests.sh, sometimes with multiple
> invocations using different parameters (for e.g. ksm_tests).
> 
> TEST_GEN_PROGS
> --------------
> 
> Contains targets that generate executable ELF binaries that _are_
> automatically run by the kselftest runner. Note that some of the executable
> targets in the vm suite are not included in run_vmtests.sh, e.g.
> soft-dirty, and split_huge_page_test. madv_populate actually is, which
> looks like a bug to me (which was introduced in 749d999c06c2 ("selftests:
> vm: bring common functions to a new file") and which I'll send out a fix
> for).
> 
> TEST_PROGS
> ----------
> 
> Contains test shell scripts. These are automatically invoked by th
> kselftest runner as well, and (as far as I understand) from the perspective
> of kselftest, are the exact same as TEST_GEN_PROGS with the exception that
> of course they're not compiled. In the kselftest runner, if the executable
> bit is not set, the runner will echo out a warning and then try to parse
> the interpreter from the first line of the file to figure out how to invoke
> it.
> 
> My two cents are that the correct way to structure vm selftests is
> something like the following:
> 
> 1. For any testcase that's compiled directly into an ELF binary, and which
>    doesn't need to be invoked with any arguments, convert it to a
>    TEST_GEN_PROGS target in tools/testing/selftests/vm/Makefile, and remove
>    it from being invoked from run_vmtests.sh.
> 2. For any testcase that's compiled directly into an ELF binary, but which
>    _does_ need to be invoked with multiple arguments and/or needs to be
>    invoked in some other special way such as va_128TBswitch, leave it in
>    TEST_GEN_FILES, and instead add a separate executable shell script wraps
>    it, and which lives in TEST_PROGS. This would be a nice cleanup, because
>    it would let us remove some of the custom logic at the top of
>    run_vmtests.sh that only applies to some of the testcases.
> 3. Once (1) and (2) are done for all relevant targets, remove
>    run_vmtests.sh and instead rely on the kselftest runner.
> 
> This would allow us to remove all custom runner logic from
> tools/testing/selftests/vm, and to instead just rely on the kselftest
> runner to do the heavy lifting for us.
> 
> As it relates specifically to Adam's patch, IMO setting that script include
> the executable bit does make sense, as it's in-line with the guidance for
> kselftests in general, and I don't think we can assume that all of the
> shell scripts will be bin/sh.
> 
> FWIW, I'm happy to do the above cleanup if nobody objects (after Adam's
> patch lands).
> 
> Thanks,
> David

Andrew, have you had a chance to think about this? At the moment, the
missing permission bit is causing the test suite to spit out an error.

The original patch that introduced the file marked is as executable,
but that got dropped at some point in the automation that merged it.

This makes the situation worse than it was before the patch that
introduced the wrapper: now no one can run the test.

Thanks,
Adam

Re: [PATCH] selftests/vm: fix va_128TBswitch.sh permissions

[Andrew Morton]

On Fri, 22 Jul 2022 10:11:59 +0200 Adam Sindelar <adam@wowsignal.io> wrote:

> Andrew, have you had a chance to think about this? At the moment, the
> missing permission bit is causing the test suite to spit out an error.

I added "selftests/vm: fix va_128TBswitch.sh permissions" to the mm-stable branch.