selftests/lkdtm: Use "comm" instead of "diff" for dmesg
diff mbox series

Message ID 202006261358.3E8AA623A9@keescook
State New
Headers show
Series
  • selftests/lkdtm: Use "comm" instead of "diff" for dmesg
Related show

Commit Message

Kees Cook June 26, 2020, 8:59 p.m. UTC
Instead of full GNU diff (which smaller boot environments may not have),
use "comm" which is more available.

Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Link: https://lore.kernel.org/lkml/CA+G9fYtHP+Gg+BrR_GkBMxu2oOi-_e9pATtpb6TVRswv1G1r1Q@mail.gmail.com
Fixes: f131d9edc29d ("selftests/lkdtm: Don't clear dmesg when running tests")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/lkdtm/run.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michael Ellerman June 27, 2020, 11:51 a.m. UTC | #1
Kees Cook <keescook@chromium.org> writes:
> Instead of full GNU diff (which smaller boot environments may not have),
> use "comm" which is more available.

Although using "comm" requires CONFIG_PRINTK_TIME=y doesn't it?

Which is probably fine, but should be mentioned.

And I guess for completeness you could add:

diff --git a/tools/testing/selftests/lkdtm/config b/tools/testing/selftests/lkdtm/config
index d874990e442b..ae88bfb163ff 100644
--- a/tools/testing/selftests/lkdtm/config
+++ b/tools/testing/selftests/lkdtm/config
@@ -1 +1,2 @@
 CONFIG_LKDTM=y
+CONFIG_PRINTK_TIME=y


cheers

> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Link: https://lore.kernel.org/lkml/CA+G9fYtHP+Gg+BrR_GkBMxu2oOi-_e9pATtpb6TVRswv1G1r1Q@mail.gmail.com
> Fixes: f131d9edc29d ("selftests/lkdtm: Don't clear dmesg when running tests")
> 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 8383eb89d88a..5fe23009ae13 100755
> --- a/tools/testing/selftests/lkdtm/run.sh
> +++ b/tools/testing/selftests/lkdtm/run.sh
> @@ -82,7 +82,7 @@ dmesg > "$DMESG"
>  ($SHELL -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true
>  
>  # Record and dump the results
> -dmesg | diff --changed-group-format='%>' --unchanged-group-format='' "$DMESG" - > "$LOG" || true
> +dmesg | comm -13 "$DMESG" - > "$LOG" || true
>  
>  cat "$LOG"
>  # Check for expected output
> -- 
> 2.25.1
>
>
> -- 
> Kees Cook
Kees Cook June 27, 2020, 3:52 p.m. UTC | #2
On Sat, Jun 27, 2020 at 09:51:31PM +1000, Michael Ellerman wrote:
> Kees Cook <keescook@chromium.org> writes:
> > Instead of full GNU diff (which smaller boot environments may not have),
> > use "comm" which is more available.
> 
> Although using "comm" requires CONFIG_PRINTK_TIME=y doesn't it?

No, it doesn't seem to. "comm" doesn't carry about the line prefixes.
AIUI, the only reason for a mention of "sort" is because of how "comm"
does its line pairing. i.e. as soon as it goes out of sync, it starts
accounting for the disjunction between files. But that's exactly what we
want it doing, and the prefix doesn't matter.
Michael Ellerman June 29, 2020, 5:50 a.m. UTC | #3
Kees Cook <keescook@chromium.org> writes:
> On Sat, Jun 27, 2020 at 09:51:31PM +1000, Michael Ellerman wrote:
>> Kees Cook <keescook@chromium.org> writes:
>> > Instead of full GNU diff (which smaller boot environments may not have),
>> > use "comm" which is more available.
>> 
>> Although using "comm" requires CONFIG_PRINTK_TIME=y doesn't it?
>
> No, it doesn't seem to. "comm" doesn't carry about the line prefixes.
> AIUI, the only reason for a mention of "sort" is because of how "comm"
> does its line pairing. i.e. as soon as it goes out of sync, it starts
> accounting for the disjunction between files. But that's exactly what we
> want it doing, and the prefix doesn't matter.

OK, if it works.

cheers
Joe Lawrence June 30, 2020, 6:53 p.m. UTC | #4
On Fri, Jun 26, 2020 at 01:59:43PM -0700, Kees Cook wrote:
> Instead of full GNU diff (which smaller boot environments may not have),
> use "comm" which is more available.
> 
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Link: https://lore.kernel.org/lkml/CA+G9fYtHP+Gg+BrR_GkBMxu2oOi-_e9pATtpb6TVRswv1G1r1Q@mail.gmail.com
> Fixes: f131d9edc29d ("selftests/lkdtm: Don't clear dmesg when running tests")
> 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 8383eb89d88a..5fe23009ae13 100755
> --- a/tools/testing/selftests/lkdtm/run.sh
> +++ b/tools/testing/selftests/lkdtm/run.sh
> @@ -82,7 +82,7 @@ dmesg > "$DMESG"
>  ($SHELL -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true
>  
>  # Record and dump the results
> -dmesg | diff --changed-group-format='%>' --unchanged-group-format='' "$DMESG" - > "$LOG" || true
> +dmesg | comm -13 "$DMESG" - > "$LOG" || true
>  
>  cat "$LOG"
>  # Check for expected output

I'm not familiar with running lkdtm tests, but I copied the same fixup
for the livepatching selftests and "comm" slides in nicely over there,
so,

Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

-- Joe
Kees Cook Sept. 9, 2020, 7:49 p.m. UTC | #5
On Fri, Jun 26, 2020 at 01:59:43PM -0700, Kees Cook wrote:
> Instead of full GNU diff (which smaller boot environments may not have),
> use "comm" which is more available.
> 
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Link: https://lore.kernel.org/lkml/CA+G9fYtHP+Gg+BrR_GkBMxu2oOi-_e9pATtpb6TVRswv1G1r1Q@mail.gmail.com
> Fixes: f131d9edc29d ("selftests/lkdtm: Don't clear dmesg when running tests")
> Signed-off-by: Kees Cook <keescook@chromium.org>

Shuah, this really needs to land to fix lkdtm tests on busybox. Can
you add this to -next? (Or is it better to direct this to Greg for the
lkdtm tree?)

Thanks!

-Kees

> ---
>  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 8383eb89d88a..5fe23009ae13 100755
> --- a/tools/testing/selftests/lkdtm/run.sh
> +++ b/tools/testing/selftests/lkdtm/run.sh
> @@ -82,7 +82,7 @@ dmesg > "$DMESG"
>  ($SHELL -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true
>  
>  # Record and dump the results
> -dmesg | diff --changed-group-format='%>' --unchanged-group-format='' "$DMESG" - > "$LOG" || true
> +dmesg | comm -13 "$DMESG" - > "$LOG" || true
>  
>  cat "$LOG"
>  # Check for expected output
> -- 
> 2.25.1
> 
> 
> -- 
> Kees Cook
Joe Lawrence Sept. 9, 2020, 8:29 p.m. UTC | #6
On 9/9/20 3:49 PM, Kees Cook wrote:
> 
> On Fri, Jun 26, 2020 at 01:59:43PM -0700, Kees Cook wrote:
>> Instead of full GNU diff (which smaller boot environments may not have),
>> use "comm" which is more available.
>>
>> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
>> Link: https://lore.kernel.org/lkml/CA+G9fYtHP+Gg+BrR_GkBMxu2oOi-_e9pATtpb6TVRswv1G1r1Q@mail.gmail.com
>> Fixes: f131d9edc29d ("selftests/lkdtm: Don't clear dmesg when running tests")
>> Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> Shuah, this really needs to land to fix lkdtm tests on busybox. Can
> you add this to -next? (Or is it better to direct this to Greg for the
> lkdtm tree?)
> 
> Thanks!
> 
> -Kees
> 
>> ---
>>   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 8383eb89d88a..5fe23009ae13 100755
>> --- a/tools/testing/selftests/lkdtm/run.sh
>> +++ b/tools/testing/selftests/lkdtm/run.sh
>> @@ -82,7 +82,7 @@ dmesg > "$DMESG"
>>   ($SHELL -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true
>>   
>>   # Record and dump the results
>> -dmesg | diff --changed-group-format='%>' --unchanged-group-format='' "$DMESG" - > "$LOG" || true
>> +dmesg | comm -13 "$DMESG" - > "$LOG" || true
>>   
>>   cat "$LOG"
>>   # Check for expected output
>> -- 
>> 2.25.1
>>
>>
>> -- 
>> Kees Cook
> 

Hi Kees,

You may want to consider a similar follow up to the one Miroslav made to 
the livepatching equivalent:

https://lore.kernel.org/live-patching/nycvar.YFH.7.76.2008271528000.27422@cbobk.fhfr.pm/T/#m1c17812d2c005dd57e9a299a4a492026a156619e

basically 'comm' will complain if two lines from dmesg have the same 
timestamp prefix and their text portions are not sorted.

Regards,

-- Joe
Shuah Khan Sept. 9, 2020, 8:49 p.m. UTC | #7
On 9/9/20 1:49 PM, Kees Cook wrote:
> 
> On Fri, Jun 26, 2020 at 01:59:43PM -0700, Kees Cook wrote:
>> Instead of full GNU diff (which smaller boot environments may not have),
>> use "comm" which is more available.
>>
>> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
>> Link: https://lore.kernel.org/lkml/CA+G9fYtHP+Gg+BrR_GkBMxu2oOi-_e9pATtpb6TVRswv1G1r1Q@mail.gmail.com
>> Fixes: f131d9edc29d ("selftests/lkdtm: Don't clear dmesg when running tests")
>> Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> Shuah, this really needs to land to fix lkdtm tests on busybox. Can
> you add this to -next? (Or is it better to direct this to Greg for the
> lkdtm tree?)
> 

Kees, Thanks for the ping. I can queue this up in -next

Greg, would you like me to take this through selftest tree?

In case you want to take this through lkdtm tree:

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah
Kees Cook Sept. 9, 2020, 9:12 p.m. UTC | #8
On Wed, Sep 09, 2020 at 04:29:50PM -0400, Joe Lawrence wrote:
> On 9/9/20 3:49 PM, Kees Cook wrote:
> > 
> > On Fri, Jun 26, 2020 at 01:59:43PM -0700, Kees Cook wrote:
> > > Instead of full GNU diff (which smaller boot environments may not have),
> > > use "comm" which is more available.
> > > 
> > > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> > > Link: https://lore.kernel.org/lkml/CA+G9fYtHP+Gg+BrR_GkBMxu2oOi-_e9pATtpb6TVRswv1G1r1Q@mail.gmail.com
> > > Fixes: f131d9edc29d ("selftests/lkdtm: Don't clear dmesg when running tests")
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > 
> > Shuah, this really needs to land to fix lkdtm tests on busybox. Can
> > you add this to -next? (Or is it better to direct this to Greg for the
> > lkdtm tree?)
> > 
> > Thanks!
> > 
> > -Kees
> > 
> > > ---
> > >   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 8383eb89d88a..5fe23009ae13 100755
> > > --- a/tools/testing/selftests/lkdtm/run.sh
> > > +++ b/tools/testing/selftests/lkdtm/run.sh
> > > @@ -82,7 +82,7 @@ dmesg > "$DMESG"
> > >   ($SHELL -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true
> > >   # Record and dump the results
> > > -dmesg | diff --changed-group-format='%>' --unchanged-group-format='' "$DMESG" - > "$LOG" || true
> > > +dmesg | comm -13 "$DMESG" - > "$LOG" || true
> > >   cat "$LOG"
> > >   # Check for expected output
> > > -- 
> > > 2.25.1
> > > 
> > > 
> > > -- 
> > > Kees Cook
> > 
> 
> Hi Kees,
> 
> You may want to consider a similar follow up to the one Miroslav made to the
> livepatching equivalent:
> 
> https://lore.kernel.org/live-patching/nycvar.YFH.7.76.2008271528000.27422@cbobk.fhfr.pm/T/#m1c17812d2c005dd57e9a299a4a492026a156619e
> 
> basically 'comm' will complain if two lines from dmesg have the same
> timestamp prefix and their text portions are not sorted.

Ah-ha! Thank you. I will send a v2. :)
Shuah Khan Sept. 9, 2020, 9:18 p.m. UTC | #9
On 9/9/20 2:49 PM, Shuah Khan wrote:
> On 9/9/20 1:49 PM, Kees Cook wrote:
>>
>> On Fri, Jun 26, 2020 at 01:59:43PM -0700, Kees Cook wrote:
>>> Instead of full GNU diff (which smaller boot environments may not have),
>>> use "comm" which is more available.
>>>
>>> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
>>> Link: 
>>> https://lore.kernel.org/lkml/CA+G9fYtHP+Gg+BrR_GkBMxu2oOi-_e9pATtpb6TVRswv1G1r1Q@mail.gmail.com 
>>>
>>> Fixes: f131d9edc29d ("selftests/lkdtm: Don't clear dmesg when running 
>>> tests")
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>
>> Shuah, this really needs to land to fix lkdtm tests on busybox. Can
>> you add this to -next? (Or is it better to direct this to Greg for the
>> lkdtm tree?)
>>
> 
> Kees, Thanks for the ping. I can queue this up in -next
> 
> Greg, would you like me to take this through selftest tree?
> 
> In case you want to take this through lkdtm tree:
> 
> Acked-by: Shuah Khan <skhan@linuxfoundation.org>
> 

Kees,

Just saw your reply. Will wait for v2.

thanks,
-- Shuah

Patch
diff mbox series

diff --git a/tools/testing/selftests/lkdtm/run.sh b/tools/testing/selftests/lkdtm/run.sh
index 8383eb89d88a..5fe23009ae13 100755
--- a/tools/testing/selftests/lkdtm/run.sh
+++ b/tools/testing/selftests/lkdtm/run.sh
@@ -82,7 +82,7 @@  dmesg > "$DMESG"
 ($SHELL -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true
 
 # Record and dump the results
-dmesg | diff --changed-group-format='%>' --unchanged-group-format='' "$DMESG" - > "$LOG" || true
+dmesg | comm -13 "$DMESG" - > "$LOG" || true
 
 cat "$LOG"
 # Check for expected output