| Message ID | 20210617231027.3908585-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/lkdtm: Use /bin/sh not $SHELL | expand |
On 18/06/2021 00:10, Kees Cook wrote: > Some environments (e.g. kerneci.org) do not set $SHELL for their test > environment. There's no need to use $SHELL here anyway, so just replace > it with hard-coded /bin/sh instead. Without this, the LKDTM tests would > never actually run on kerneci.org. There's a bit more to it... The lkdtm tests make use of the process substitution feature with the <() syntax which is specific to Bash. The tests run by KernelCI use Debian, where /bin/sh points to /bin/dash by default which doesn't support this feature. So one way to fix it would be: (/bin/bash -c 'cat <(echo '"$test"') >'"$TRIGGER") However, this might break others' workflows. In fact the LAVA jobs run by KernelCI do define the $SHELL environment variable except it's defined to be /bin/sh - and that means /bin/dash gets called and we're back to the issue explained above. I've manually run a modified test job which defines SHELL=/bin/bash and that works: https://lava.collabora.co.uk/scheduler/job/4055547#L2835 So to avoid hitting the same issue in other places, as it seems like there is an implicit dependency on Bash, we can just change KernelCI kselftest jobs to always export SHELL=/bin/bash. I suppose an even better fix would be to use standard shell features that would work with any /bin/sh implementation, but this is there to kill the sub-shell rather than the main script process so I'm not entirely sure if we can easily do that differently. Maybe we can pipe the output to cat rather than the substitution syntax, e.g.: (/bin/sh -c '(echo '"$test"') | cat >'"$TRIGGER") || true So I think the "safest" solution is to not change the kselftest script and export SHELL=/bin/bash in the KernelCI jobs. If the pipe approach is good enough at catching signals then it could be done on top of this patch as it's standard and should work with any /bin/sh implementation. What do you think? Thanks, Guillaume > Fixes: 46d1a0f03d66 ("selftests/lkdtm: Add tests for LKDTM targets") > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > tools/testing/selftests/lkdtm/run.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/lkdtm/run.sh b/tools/testing/selftests/lkdtm/run.sh > index bb7a1775307b..968ff3cf5667 100755 > --- a/tools/testing/selftests/lkdtm/run.sh > +++ b/tools/testing/selftests/lkdtm/run.sh > @@ -79,7 +79,7 @@ dmesg > "$DMESG" > # Most shells yell about signals and we're expecting the "cat" process > # to usually be killed by the kernel. So we have to run it in a sub-shell > # and silence errors. > -($SHELL -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true > +(/bin/sh -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true > > # Record and dump the results > dmesg | comm --nocheck-order -13 "$DMESG" - > "$LOG" || true >
On Fri, Jun 18, 2021 at 08:29:57PM +0100, Guillaume Tucker wrote: > There's a bit more to it... The lkdtm tests make use of the > process substitution feature with the <() syntax which is > specific to Bash. The tests run by KernelCI use Debian, where > /bin/sh points to /bin/dash by default which doesn't support this > feature. So one way to fix it would be: > > (/bin/bash -c 'cat <(echo '"$test"') >'"$TRIGGER") Argh. I always forget that <() is a bash-ism. Thank you for tracking this down! > However, this might break others' workflows. > > In fact the LAVA jobs run by KernelCI do define the $SHELL > environment variable except it's defined to be /bin/sh - and that > means /bin/dash gets called and we're back to the issue explained > above. > > I've manually run a modified test job which defines > SHELL=/bin/bash and that works: > > https://lava.collabora.co.uk/scheduler/job/4055547#L2835 Yay!! > So to avoid hitting the same issue in other places, as it seems > like there is an implicit dependency on Bash, we can just change > KernelCI kselftest jobs to always export SHELL=/bin/bash. > > I suppose an even better fix would be to use standard shell > features that would work with any /bin/sh implementation, but > this is there to kill the sub-shell rather than the main script > process so I'm not entirely sure if we can easily do that > differently. Maybe we can pipe the output to cat rather than the > substitution syntax, e.g.: > > (/bin/sh -c '(echo '"$test"') | cat >'"$TRIGGER") || true Yeah, this is the right fix. There's no reason anything should depend on bash; I was just not thinking when I wrote this originally. :) > So I think the "safest" solution is to not change the kselftest > script and export SHELL=/bin/bash in the KernelCI jobs. If the > pipe approach is good enough at catching signals then it could be > done on top of this patch as it's standard and should work with > any /bin/sh implementation. What do you think? If you set SHELL=/bin/bash for now, the lkdtm tests should work as they are, and once the v2 patch lands, they'll continue to work, and SHELL=/bin/bash can be removed. Thank you so much! -Kees
diff --git a/tools/testing/selftests/lkdtm/run.sh b/tools/testing/selftests/lkdtm/run.sh index bb7a1775307b..968ff3cf5667 100755 --- a/tools/testing/selftests/lkdtm/run.sh +++ b/tools/testing/selftests/lkdtm/run.sh @@ -79,7 +79,7 @@ dmesg > "$DMESG" # Most shells yell about signals and we're expecting the "cat" process # to usually be killed by the kernel. So we have to run it in a sub-shell # and silence errors. -($SHELL -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true +(/bin/sh -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true # Record and dump the results dmesg | comm --nocheck-order -13 "$DMESG" - > "$LOG" || true
Some environments (e.g. kerneci.org) do not set $SHELL for their test environment. There's no need to use $SHELL here anyway, so just replace it with hard-coded /bin/sh instead. Without this, the LKDTM tests would never actually run on kerneci.org. Fixes: 46d1a0f03d66 ("selftests/lkdtm: Add tests for LKDTM targets") Cc: stable@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- tools/testing/selftests/lkdtm/run.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)