Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

On Thu 2018-04-05 16:25:41, Rasmus Villemoes wrote:
> On 2018-04-04 10:58, Petr Mladek wrote:
> > There are few printk formats that make sense only with two or more
> > specifiers. Also some specifiers make sense only when a kernel feature
> > is enabled.
> > 
> > The handling of unknown specifiers is strange, inconsistent, and
> > even leaking the address. For example, netdev_bits() prints the
> > non-hashed pointer value or clock() prints "(null)".
> > 
> > The best solution seems to be in flags_string(). It does not print any
> > misleading value. Instead it calls WARN_ONCE() describing the unknown
> > specifier. Therefore it clearly shows the problem and helps to find it.
> >
> I'm not sure it's actually worth WARNing about the unknown variants
> since

I agree that WARN() looks like an overkill for this type of error.
On the other hand, I am not sure how to make it lightweight and useful
at the same time.

We could not be much talkative in the passed buffer because we do not
know how big it is. We could call printk but it might be hard to match
the line with the caller without the backtrace.

The extra printk() message would end up in the per-CPU printk_safe
buffer when the broken vsprintf() comes from printk(). It might be
pushed to the main log buffer "much" later. On the other hand,
the original string would not end in the log buffer at all
when vsprintf() is not called from printk().


> we have static analysis (at least checkpatch and smatch) that
> should catch that.

Yes but not everybody uses it. Also there are things that depends
on CONFIG setting.

Anyway, I think that triggering the WARN() is rather a corner case.
So I would not be much concerned about using WARN().


> Even just git grep -1 -E '%p"$' finds %pt and %po
> which should get fixed before somebody claims those extensions.

I do not understand this much. git grep -1 -E '%p"$' does not find
lines with %pt and %po. Also it seems that %pt and %po are not
part of any format passed to vsprintf.



> But, I don't disagree with trying to fix up the inconsistency, and
> certainly not with fixing netdev_bits(), but it seems you've missed that
> e.g. the "case: 'g'" is completely compiled out for !CONFIG_BLOCK.
> There's also %pOF which is effectively disabled for !CONFIG_OF (which
> obviously makes sense), but with yet a different fallback behaviour.

You are accurate ;-) I was a bit lazy to change them. %pg was "nicely"
compiled out. %pOF was nicely minimalist solution but not usable in
general. I could do it in v5 or in a separate patchset.


> Hm. I think we should somehow distinguish between the cases of "%po" and
> "%pNX", i.e. specifiers/variants that are always bogus, and the cases of
> a %pOF or %pC that somehow happens even though nobody should have a
> struct device_node* or struct clk* to pass.

I think that the existing formulation is fine for the always bogus
specifiers, for example:

  "Unsupported pointer format specifier: %pGa"

What about the following for the other case:

  "Format specifier %%pC supported only with CONFIG_HAVE_CLK\n"

Best Regards,
Petr