[PATCH] selftests/lkdtm: Use /bin/sh not $SHELL

[PATCH] selftests/lkdtm: Use /bin/sh not $SHELL

[Kees Cook]

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(-)

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
-- 
2.25.1

Re: [PATCH] selftests/lkdtm: Use /bin/sh not $SHELL

[Guillaume Tucker]

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
> 

Re: [PATCH] selftests/lkdtm: Use /bin/sh not $SHELL

[Kees Cook]

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

-- 
Kees Cook