| Message ID | 20210619025834.2505201-1-keescook@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] selftests/lkdtm: Use /bin/sh not $SHELL | expand |
From: Guillaume Tucker > Sent: 23 June 2021 13:40 ... > > diff --git a/tools/testing/selftests/lkdtm/run.sh b/tools/testing/selftests/lkdtm/run.sh > > index bb7a1775307b..0f9f22ac004b 100755 > > --- a/tools/testing/selftests/lkdtm/run.sh > > +++ b/tools/testing/selftests/lkdtm/run.sh > > @@ -78,8 +78,9 @@ 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 > > +# to avoid terminating this script. Leave stderr alone, just in case > > +# something _else_ happens. > > +(/bin/sh -c '(echo '"$test"') | cat >'"$TRIGGER") || true I was having trouble parsing that command - and I'm good at shell scripts. I think the extra subshell the 'echo' is in doesn't help. In fact, is either subshell needed? Surely: /bin/sh -c "echo '$test' | cat >$trigger" || true will work just as well? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Jun 23, 2021 at 10:35:50PM +0800, kernel test robot wrote: > > > Greeting, > > FYI, we noticed the following commit (built with gcc-9): > > commit: 84d8cf25b0f80da0ac229214864654a7662ec7e4 ("[PATCH v2] selftests/lkdtm: Use /bin/sh not $SHELL") > url: https://github.com/0day-ci/linux/commits/Kees-Cook/selftests-lkdtm-Use-bin-sh-not-SHELL/20210619-105959 > base: https://git.kernel.org/cgit/linux/kernel/git/shuah/linux-kselftest.git next > > in testcase: kernel-selftests > version: kernel-selftests-x86_64-f8879e85-1_20210618 > with following parameters: > > group: lkdtm > ucode: 0xe2 Heh. Yes, this is working as intended. :) Most of the lkdtm tests will trigger Oopses, and this is by design: it is checking that the kernel catches bad conditions and freaks out appropriately. -Kees -- Kees Cook
On Wed, Jun 23, 2021 at 01:39:57PM +0100, Guillaume Tucker wrote: > On 19/06/2021 03:58, Kees Cook wrote: > > Some environments do not set $SHELL when running tests. There's no need > > to use $SHELL here anyway, so just replace it with hard-coded path > > instead. Additionally avoid using bash-isms in the command, so that > > regular /bin/sh can be used. > > > > Suggested-by: Guillaume Tucker <guillaume.tucker@collabora.com> > > Fixes: 46d1a0f03d66 ("selftests/lkdtm: Add tests for LKDTM targets") > > Cc: stable@vger.kernel.org > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > Tested-by: "kernelci.org bot" <bot@kernelci.org> > > > Sample staging results with this patch applied on top of > next-20210622: > > https://staging.kernelci.org/test/plan/id/60d2dbdc3cfb88da0924bf41/ > > Full log: > > https://storage.staging.kernelci.org/kernelci/staging-next/staging-next-20210623.0/x86_64/x86_64_defconfig+x86-chromebook+kselftest/clang-13/lab-collabora/kselftest-lkdtm-asus-C523NA-A20057-coral.html Awesome! This looks great. :) What's needed to build these kernels will different CONFIGs? I see a bunch of things (commonly found in distro kernels) that are not set: CONFIG_SLAB_FREELIST_HARDENED=y CONFIG_FORTIFY_SOURCE=y CONFIG_HARDENED_USERCOPY=y # CONFIG_HARDENED_USERCOPY_FALLBACK is not set Should I add these to the kselftest "config" file for LKDTM? Thanks again for the help with this! -Kees -- Kees Cook
+kernelci +collabora On 23/06/2021 15:38, Kees Cook wrote: > On Wed, Jun 23, 2021 at 01:39:57PM +0100, Guillaume Tucker wrote: >> On 19/06/2021 03:58, Kees Cook wrote: >>> Some environments do not set $SHELL when running tests. There's no need >>> to use $SHELL here anyway, so just replace it with hard-coded path >>> instead. Additionally avoid using bash-isms in the command, so that >>> regular /bin/sh can be used. >>> >>> Suggested-by: Guillaume Tucker <guillaume.tucker@collabora.com> >>> Fixes: 46d1a0f03d66 ("selftests/lkdtm: Add tests for LKDTM targets") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Kees Cook <keescook@chromium.org> >> >> >> Tested-by: "kernelci.org bot" <bot@kernelci.org> >> >> >> Sample staging results with this patch applied on top of >> next-20210622: >> >> https://staging.kernelci.org/test/plan/id/60d2dbdc3cfb88da0924bf41/ >> >> Full log: >> >> https://storage.staging.kernelci.org/kernelci/staging-next/staging-next-20210623.0/x86_64/x86_64_defconfig+x86-chromebook+kselftest/clang-13/lab-collabora/kselftest-lkdtm-asus-C523NA-A20057-coral.html > > Awesome! This looks great. :) > > What's needed to build these kernels will different CONFIGs? I see a > bunch of things (commonly found in distro kernels) that are not set: > > CONFIG_SLAB_FREELIST_HARDENED=y > CONFIG_FORTIFY_SOURCE=y > CONFIG_HARDENED_USERCOPY=y > # CONFIG_HARDENED_USERCOPY_FALLBACK is not set > > Should I add these to the kselftest "config" file for LKDTM? Yes, that's the current way to do it. KernelCI is simply concatenating all the config files found under tools/testing/selftests into one big kselftest fragment which is then merged with the defconfig. We could enable arbitrary things for KernelCI but of course it's much better to not do that and stick to what's in the kernel tree. If you do send such a patch, please CC kernelci@groups.io or myself and we can give it a spin on staging.kernelci.org as well. Best wishes, Guillaume
On Wed, Jun 23, 2021 at 01:43:04PM +0000, David Laight wrote: > From: Guillaume Tucker > > Sent: 23 June 2021 13:40 > ... > > > diff --git a/tools/testing/selftests/lkdtm/run.sh b/tools/testing/selftests/lkdtm/run.sh > > > index bb7a1775307b..0f9f22ac004b 100755 > > > --- a/tools/testing/selftests/lkdtm/run.sh > > > +++ b/tools/testing/selftests/lkdtm/run.sh > > > @@ -78,8 +78,9 @@ 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 > > > +# to avoid terminating this script. Leave stderr alone, just in case > > > +# something _else_ happens. > > > +(/bin/sh -c '(echo '"$test"') | cat >'"$TRIGGER") || true > > I was having trouble parsing that command - and I'm good > at shell scripts. > I think the extra subshell the 'echo' is in doesn't help. > In fact, is either subshell needed? > Surely: > /bin/sh -c "echo '$test' | cat >$trigger" || true > will work just as well? Ah yeah, and I just tested it to double check, it can be even simpler: echo "$test" | /bin/sh -c "cat >$TRIGGER" || true I'll adjust and resend... -Kees -- Kees Cook
From: Kees Cook > Sent: 23 June 2021 17:19 > > On Wed, Jun 23, 2021 at 01:43:04PM +0000, David Laight wrote: > > From: Guillaume Tucker > > > Sent: 23 June 2021 13:40 > > ... > > > > diff --git a/tools/testing/selftests/lkdtm/run.sh b/tools/testing/selftests/lkdtm/run.sh > > > > index bb7a1775307b..0f9f22ac004b 100755 > > > > --- a/tools/testing/selftests/lkdtm/run.sh > > > > +++ b/tools/testing/selftests/lkdtm/run.sh > > > > @@ -78,8 +78,9 @@ 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 > > > > +# to avoid terminating this script. Leave stderr alone, just in case > > > > +# something _else_ happens. > > > > +(/bin/sh -c '(echo '"$test"') | cat >'"$TRIGGER") || true > > > > I was having trouble parsing that command - and I'm good > > at shell scripts. > > I think the extra subshell the 'echo' is in doesn't help. > > In fact, is either subshell needed? > > Surely: > > /bin/sh -c "echo '$test' | cat >$trigger" || true > > will work just as well? > > Ah yeah, and I just tested it to double check, it can be even simpler: > > echo "$test" | /bin/sh -c "cat >$TRIGGER" || true You can probably even do: echo "$test" | /bin/sh -c cat >$TRIGGER || true (moving the redirect to the outer shell). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Jun 23, 2021 at 04:27:47PM +0000, David Laight wrote: > From: Kees Cook > > Sent: 23 June 2021 17:19 > > > > On Wed, Jun 23, 2021 at 01:43:04PM +0000, David Laight wrote: > > > From: Guillaume Tucker > > > > Sent: 23 June 2021 13:40 > > > ... > > > > > diff --git a/tools/testing/selftests/lkdtm/run.sh b/tools/testing/selftests/lkdtm/run.sh > > > > > index bb7a1775307b..0f9f22ac004b 100755 > > > > > --- a/tools/testing/selftests/lkdtm/run.sh > > > > > +++ b/tools/testing/selftests/lkdtm/run.sh > > > > > @@ -78,8 +78,9 @@ 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 > > > > > +# to avoid terminating this script. Leave stderr alone, just in case > > > > > +# something _else_ happens. > > > > > +(/bin/sh -c '(echo '"$test"') | cat >'"$TRIGGER") || true > > > > > > I was having trouble parsing that command - and I'm good > > > at shell scripts. > > > I think the extra subshell the 'echo' is in doesn't help. > > > In fact, is either subshell needed? > > > Surely: > > > /bin/sh -c "echo '$test' | cat >$trigger" || true > > > will work just as well? > > > > Ah yeah, and I just tested it to double check, it can be even simpler: > > > > echo "$test" | /bin/sh -c "cat >$TRIGGER" || true > > You can probably even do: > > echo "$test" | /bin/sh -c cat >$TRIGGER || true > > (moving the redirect to the outer shell). Actually, it looks like the "write" is already happening in the exec'd process, so this can just be: echo "$test" | cat >$TRIGGER || true But it still can't be: echo "$test" >$TRIGGER which is what I had over-engineered a solution to. :) -- Kees Cook
From: Kees Cook > Sent: 23 June 2021 23:47 > > On Wed, Jun 23, 2021 at 04:27:47PM +0000, David Laight wrote: ... > > You can probably even do: > > > > echo "$test" | /bin/sh -c cat >$TRIGGER || true > > > > (moving the redirect to the outer shell). > > Actually, it looks like the "write" is already happening in the exec'd > process, so this can just be: > > echo "$test" | cat >$TRIGGER || true > > But it still can't be: > > echo "$test" >$TRIGGER > > which is what I had over-engineered a solution to. :) That one fails because echo is the shell builtin. But: /bin/echo "$test" >$TRIGGER should be fine. Quite where the original came from I not sure I want to find out. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/tools/testing/selftests/lkdtm/run.sh b/tools/testing/selftests/lkdtm/run.sh index bb7a1775307b..0f9f22ac004b 100755 --- a/tools/testing/selftests/lkdtm/run.sh +++ b/tools/testing/selftests/lkdtm/run.sh @@ -78,8 +78,9 @@ 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 +# to avoid terminating this script. Leave stderr alone, just in case +# something _else_ happens. +(/bin/sh -c '(echo '"$test"') | cat >'"$TRIGGER") || true # Record and dump the results dmesg | comm --nocheck-order -13 "$DMESG" - > "$LOG" || true
Some environments do not set $SHELL when running tests. There's no need to use $SHELL here anyway, so just replace it with hard-coded path instead. Additionally avoid using bash-isms in the command, so that regular /bin/sh can be used. Suggested-by: Guillaume Tucker <guillaume.tucker@collabora.com> 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 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)