linux-kselftest.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Guillaume Tucker <guillaume.tucker@collabora.com>
To: Kees Cook <keescook@chromium.org>, Shuah Khan <shuah@kernel.org>
Cc: stable@vger.kernel.org, linux-kselftest@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] selftests/lkdtm: Use /bin/sh not $SHELL
Date: Fri, 18 Jun 2021 20:29:57 +0100	[thread overview]
Message-ID: <57775fe8-d9c2-4004-b8c5-0247faf33aa4@collabora.com> (raw)
In-Reply-To: <20210617231027.3908585-1-keescook@chromium.org>

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
> 


  reply	other threads:[~2021-06-18 19:30 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-17 23:10 [PATCH] selftests/lkdtm: Use /bin/sh not $SHELL Kees Cook
2021-06-18 19:29 ` Guillaume Tucker [this message]
2021-06-19  3:02   ` Kees Cook

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=57775fe8-d9c2-4004-b8c5-0247faf33aa4@collabora.com \
    --to=guillaume.tucker@collabora.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=shuah@kernel.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).