linux-kernel - Re: [PATCH] x86/tools: fix line number reported for malformed lines

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 21 Feb 2024 22:49:32 +0100
From: Valentin Obst <kernel@...entinobst.de>
To: miguel.ojeda.sandonis@...il.com
Cc: a.hindborg@...sung.com,
	david@...dahead.eu,
	gregkh@...uxfoundation.org,
	hpa@...or.com,
	john.m.baublitz@...il.com,
	kernel@...entinobst.de,
	linux-kernel@...r.kernel.org,
	mhiramat@...nel.org,
	mingo@...nel.org,
	mingo@...hat.com,
	ojeda@...nel.org,
	peterz@...radead.org,
	sergio.collado@...il.com,
	stable@...r.kernel.org,
	tglx@...utronix.de,
	x86@...nel.org
Subject: Re: [PATCH] x86/tools: fix line number reported for malformed lines

> On Wed, Feb 21, 2024 at 10:00 AM Valentin Obst <kernel@...entinobst.de> wrote:
> >
> > While debugging the `X86_DECODER_SELFTEST` failure first reported in [1],
> > [In this case the line causing the failure is interpreted as two lines by
> > the tool (due to its length, but this is fixed by [1, 2]), and the second
> > line is reported. Still the spatial closeness between the reported line and
> > the line causing the failure would have made debugging a lot easier.]
>
> Thanks Valentin, John et al. for digging into this (and the related
> issue) -- very much appreciated.
>
> It looks good to me:
>
> Reviewed-by: Miguel Ojeda <ojeda@...nel.org>
> Tested-by: Miguel Ojeda <ojeda@...nel.org>

Thanks!

>
> This should probably have a Fixes tag -- from a quick look, the
> original test did not seem to have the problem because `insns` was
> equivalent to the number of lines since there was no `if ... {
> continue; }` for the symbol case. At some point that branch was added,
> so that was not true anymore, thus that one should probably be the
> Fixes tag, though please double-check:
>
>     Fixes: 35039eb6b199 ("x86: Show symbol name if insn decoder test failed")

Cross checked this as well, can confirm your assessment. Thanks for
bringing this up.

>
> It is a minor issue for sure, so perhaps not worth backporting, but
> nevertheless the hash is in a very old kernel, and thus the issue
> applies to all stable kernels. So it does not hurt flagging it to the
> stable team:
>
>     Cc: stable@...r.kernel.org
>
> In addition, John reported the original issue, but this one was found
> due to that one, and I am not exactly sure who did what here. Please
> consider whether one of the following (or similar) may be fair:
>
>     Reported-by: John Baublitz <john.m.baublitz@...il.com>
>     Debugged-by: John Baublitz <john.m.baublitz@...il.com>

Absolutely, without him reporting the test failure and narrowing down the
config I'd have never looked at this file. Adding him for **both** is fair.
(This particular fix was not discussed on Zulip though, its just something
I noticed along the way.)

>
> An extra Link to the discussion in Zulip could be nice too:
>
>     Link: https://rust-for-linux.zulipchat.com/#narrow/stream/291565-Help/topic/insn_decoder_test.20failure/near/421075039

Didn't add it because the discussion does not mention this particular
issue, but it might indeed be good for some context.

Will this need a v2, or are all of the 'Fixes', 'Reported-By',
'Debugged-By', 'Tested-By', 'Reviewed-By' and 'Link' tags something that
maintainers may add when merging?

    - Best Valentin

>
> Finally, a nit: links are typically written like the following -- you
> can still use bracket references at the end:
>
>     Link: https://lore.kernel.org/lkml/Y9ES4UKl%2F+DtvAVS@gmail.com/T/ [1]
>     Link: https://lore.kernel.org/rust-for-linux/20231119180145.157455-1-sergio.collado@gmail.com/
> [2]
>
> Cheers,
> Miguel