linux-kernel - Re: [PATCH 4/5] bugs/x86: Augment warnings output by concatenating 'cond_str' with the regular __FILE__ string in _BUG

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <Z-UcIJAQsNXoxMXG@gmail.com>
Date: Thu, 27 Mar 2025 10:36:32 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Peter Zijlstra <peterz@...radead.org>
Cc: linux-kernel@...r.kernel.org,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH 4/5] bugs/x86: Augment warnings output by concatenating
 'cond_str' with the regular __FILE__ string in _BUG_FLAGS()


* Peter Zijlstra <peterz@...radead.org> wrote:

> On Wed, Mar 26, 2025 at 09:47:49AM +0100, Ingo Molnar wrote:
> > This allows the reuse of the UD2 based 'struct bug_entry' low-overhead
> > _BUG_FLAGS() implementation and string-printing backend, without
> > having to add a new field.
> > 
> > An example:
> > 
> > If we have the following WARN_ON_ONCE() in kernel/sched/core.c:
> > 
> > 	WARN_ON_ONCE(idx < 0 && ptr);
> > 
> > Then previously _BUG_FLAGS() would store this string in bug_entry::file:
> > 
> > 	"kernel/sched/core.c"
> > 
> > After this patch, it would store and print:
> > 
> > 	"[idx < 0 && ptr] kernel/sched/core.c"
> > 
> > Which is an extended string that will be printed in warnings.
> > 
> > Signed-off-by: Ingo Molnar <mingo@...nel.org>
> > ---
> >  arch/x86/include/asm/bug.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
> > index aff1c6b7a7f3..e966199c8ef7 100644
> > --- a/arch/x86/include/asm/bug.h
> > +++ b/arch/x86/include/asm/bug.h
> > @@ -50,7 +50,7 @@ do {									\
> >  		     "\t.org 2b+%c3\n"					\
> >  		     ".popsection\n"					\
> >  		     extra						\
> > -		     : : "i" (__FILE__), "i" (__LINE__),		\
> > +		     : : "i" (cond_str __FILE__), "i" (__LINE__),		\
> >  			 "i" (flags),					\
> >  			 "i" (sizeof(struct bug_entry)));		\
> >  } while (0)
> 
> Sneaky :-)

BTW., any reason why we go all the trouble with the bug_entry::line u16 
number, instead of storing it in the bug_entry::file string with a
:__LINE__ postfix or so?

Using 4 bytes doesn't even save any RAM, given that the average line 
position number within the kernel is around 3 bytes:

 $ for N in $(git grep -lE 'WARN_ON|BUG_ON|WARN\(|BUG\(' -- '*.[ch]'); do echo -n $(($(cat $N | wc -l)/2)) | wc -c; done | sort -n | uniq -c

     10 1
   1209 2
   6645 3
   1582 4
     10 5

( This is the histogram of the length of average line numbers within 
  the kernel's ~9,400 .[ch] source code files that are using these 
  facilities. )

So concatenation would save on complexity, IMHO, and it would give us 
flexibility as well, if we passed in the string from higher layers. We 
wouldn't have to change architecture level code at all for this series 
for example.

Not to mention that some files within the kernel are beyond the 16-bit 
limit already, 38K to 222K lines of code:

  starship:~/tip> wc -l drivers/gpu/drm/amd/include/asic_reg/dcn/dcn_3_2_0_sh_mask.h
  222,948 drivers/gpu/drm/amd/include/asic_reg/dcn/dcn_3_2_0_sh_mask.h

  starship:~/tip> wc -l crypto/testmgr.h
  38,897 crypto/testmgr.h

So u16 line numbers are also a (minor) breakage waiting to happen.

Thanks,

	Ingo