Re: [PATCH -next] lib/vsprintf: Make function pointer_string static

[Steven Rostedt]

[ added Joe ]

On Mon, 29 Apr 2019 16:30:37 +0200
Petr Mladek <pmladek@xxxxxxxx> wrote:

> On Mon 2019-04-29 09:13:20, Steven Rostedt wrote:
> > On Mon, 29 Apr 2019 13:08:01 +0200
> > Petr Mladek <pmladek@xxxxxxxx> wrote:
> >   
> > > > Looks like commit "vsprintf: Do not check address of well-known
> > > > strings" removed the: "static noinline_for_stack"
> > > > 
> > > > Does pointer_string() need that still?    
> > > 
> > > Heh, it was removed by mistake and well hidden in the diff.
> > > 
> > > I have pushed Yue's fix into printk.git, branch
> > > for-5.2-vsprintf-hardening
> > > 
> > > Thanks for the patch.  
> > 
> > But doesn't it still need the "noinline_for_stack", that doesn't look
> > like it changed.  
> 
> Good question. I have just double checked it. And pointer_string() with
> "noinline_for_stack" does not make any difference in the stack
> usage here.
> 
> 
> I actually played with this before:
> 
> "noinline_for_stack" is a black magic added by
> the commit cf3b429b03e827c7180 ("vsprintf.c: use noinline_for_stack").

>From what I understand, "noinline_for_stack" is just noinline and the
"for_stack" part is just to document that the noinline is used for
stack purposes. If the compiler doesn't inline the function without the
noinline, then it wont make any difference.

The point was to not inline the function because it can be used in
stack critical areas, and that it's better to do the call than to
increase the stack.

Although the commit you posted is back in 2010, and compilers and I
think even our stack size has changed since then. This may not be a
critical issue today, and just making it static and letting the
compiler decide might be the right solution.

Let's add Yue's patch as is for now, and perhaps revisit this as a
separate thread.

-- Steve

> 
> It is evidently useful in some cases. But I somehow doubt
> that it really makes things better when used everywhere.
> Therefore I have got a bit relaxed and omitted it in most
> newly added functions that did not affect the results.
> 
> They are the same before and after the patchset:
> 
> pmladek@pathway:/prace/kernel/linux-printk> objdump -d lib/vsprintf.o | perl scripts/checkstack.pl
> 0x00000e12 symbol_string [vsprintf.o]:                  248
> 0x00000e6d symbol_string [vsprintf.o]:                  248
> 0x000012fb ip6_addr_string_sa [vsprintf.o]:             112
> 0x00001415 ip6_addr_string_sa [vsprintf.o]:             112
> 0x000028c6 resource_string.isra.9 [vsprintf.o]:         104
> 0x00002964 resource_string.isra.9 [vsprintf.o]:         104
> 
> 
> Would you like to fix this clearly, for example, rebase and
> put both "static noinline_for_stack" back or add yet
> another commit or?
> 
> IMHO, it is not too important. Anyway, I am open for any
> advice. I do not want to create more mess.
> 
> Best Regards,
> Petr