Re: [PATCH v2 8/9] lib/vsprintf: Remove useless NULL checks

[Andy Shevchenko]

On Tue, 2018-02-27 at 16:50 +0100, Petr Mladek wrote:
> On Fri 2018-02-16 23:07:10, Andy Shevchenko wrote:
> > The pointer can't be NULL since it's first what has been done in the
> > pointer().
> > 
> > Remove useless checks.
> > 
> > Note we leave check for !CONFIG_HAVE_CLK to make compiler
> > to optimize code away when possible.
> > 
> > Cc: Petr Mladek <pmladek@xxxxxxxx>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > ---
> >  lib/vsprintf.c | 13 +------------
> >  1 file changed, 1 insertion(+), 12 deletions(-)
> > 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 97be2d07297a..a49da00b79e7 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -819,10 +819,6 @@ char *hex_string(char *buf, char *end, u8
> > *addr, struct printf_spec spec,
> >  		/* nothing to print */
> >  		return buf;
> >  
> > -	if (ZERO_OR_NULL_PTR(addr))
> 
> This macro matches also values <= 16.

Yes, I know.

This had been discussed with Rasmus and we agreed that printing a result
of kmalloc(0) is rather weird.

Moreover, in couple of cases I added these checks.
 
> >  	switch (fmt[1]) {
> > @@ -1580,9 +1572,6 @@ char *device_node_string(char *buf, char *end,
> > struct device_node *dn,
> >  	if (!IS_ENABLED(CONFIG_OF))
> >  		return string(buf, end, "(!OF)", spec);
> >  
> > -	if ((unsigned long)dn < PAGE_SIZE)
> > -		return string(buf, end, "(null)", spec);
> 
> In this case, "null" was printed for ptr < PAGE_SIZE. The same check
> is also in string() function.

Do we have a uses cases when invalid (non-NULL) pointer is supplied to
print function?

Those call sites have to be fixed.

> Note that it is not only about the printed value. The pointer is later
> derefecend. We will start crashing on dn > 0 && dn < PAGE_SIZE.

Yes.
So, fix the call sites!

> To be honest, I do not feel experienced enough to decide
> about the preferred behavior. On one hand, it is bad when
> printk() would crash the kernel. On the other hand, hiding wide
> range of values under "(null)" string might confuse people.

> Would it make sense to survive and write different strings for
> difference intervals? For example?
> 
>     "(null)"     for ptr == 0
>     "(null-16)"  for ptr > 0 && ptr <= 16
>     "(null-pg)"  for prt > 16 && ptr <= PAGE_SIZE
> 
> In each case, this patch changes the behavior and it should
> be documented in the commit message.

Personally I strongly disagree with blowing code up in such places for
little or none benefit.

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy