Re: [RFC PATCH] vsprintf: add flag ZEROPAD handling before crng is ready

[Yang, Shunyong]

Hi, Rasmus and Andy,
 Thanks for your feedback. I add some information below.

On Fri, 2018-01-26 at 10:43 +0100, Rasmus Villemoes wrote:
> On 26 January 2018 at 10:17, Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > 
> > +Rasmus
> Thanks.
> 
> > 
> > On Fri, 2018-01-26 at 15:39 +0800, Yang Shunyong wrote:
> > > 
> > > Before crng is ready, output of "%p" composes of "(ptrval)" and
> > > left padding spaces for alignment as no random address can be
> > > generated. This seems a little strange sometimes.
> > > For example, when irq domain names are built with "%p", the nodes
> > > under /sys/kernel/debug/irq/domains like this,
> > > 
> > > [root@y irq]# ls domains/
> > > defaultÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂirqchip@ÂÂÂÂÂÂÂÂ(ptrval)-2
> > > irqchip@ÂÂÂÂÂÂÂÂ(ptrval)-4ÂÂ\_SB_.TCS0.QIC1ÂÂ\_SB_.TCS0.QIC3
> > > irqchip@ÂÂÂÂÂÂÂÂ(ptrval)ÂÂirqchip@ÂÂÂÂÂÂÂÂ(ptrval)-3
> > > \_SB_.TCS0.QIC0ÂÂÂÂÂÂÂÂÂÂÂÂÂ\_SB_.TCS0.QIC2
> > > 
> > > The name "irqchip@ÂÂÂÂÂÂÂÂ(ptrval)-2" is not so readable in
> > > console
> > > output.
> > Yes, this is not best output.
> > 
> > > 
> > > This patch adds ZEROPAD handling in widen_string() and
> > > move_right().
> > > When ZEROPAD is set in spec, it will use '0' for padding. If not
> > > set, it will use ' '.
> > > This patch also sets ZEROPAD in ptr_to_id() before crgn is ready.
> Yew.
> 
> > 
> > Have you added specific test cases to see what's going on for
> > patterns
> > like
> > 
> > printf("%0s\n", "ÂÂÂÂ(my string)");
> [That's not really relevant, since we'll never have those (gcc says
> "warning: '0' flag used with â%sâ").]
> 
> > 
> > > 
> > > @@ -1702,6 +1709,8 @@ static char *ptr_to_id(char *buf, char
> > > *end,
> > > void *ptr, struct printf_spec spec)
> > > 
> > > ÂÂÂÂÂÂif (unlikely(!have_filled_random_ptr_key)) {
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂspec.field_width = default_width;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂspec.flags |= ZEROPAD;
> > > +
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/* string length must be less than default_width */
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn string(buf, end, "(ptrval)", spec);
> > > ÂÂÂÂÂÂ}
> So why not just use a string literal with the right width to begin
> with, e.g. =====(ptrval)===== or whatever manual padding left or
> right
> seems appropriate. Space-padding is not nice, but 0-padding isn't
> much

Agree. 0-padding isn't much better. I prefers 'x' more as it means
"unknown" and "don't care" sometimes.
For "(ptrval)", shall we change to something indicating the output is
hidden? Such as following candidate,
[HIDDEN]
[HASHED]

Then, on a 32-bit system, the output is "[HIDDEN]". On a 64-bit system,
it is "[xxxxHIDDENxxxx]" or "[====HIDDEN====]".
"[...]" is offen used in dmesg to represent address range and more
readable in this case. But I am not sure whether there are other issues
compare to "(...)".

> better. That way you only affect the uncommon case of %p before
> have_filled_random_ptr_key instead of adding a few instructions to
> all
> %s users.
> 

Agree. My original idea was to reuse existing code. Exactly as you
said, this is the uncommon case, change should be minimized.

> While at it, it may be worth looking into whether the irqdomain
> output
> actually needs the @%p thing or if one could improve that instead.
> 

I am inclined to make this patch a small enhancement to Tobin's "%p"
series. As,
 1. I am not sure whether someone may use "%p" in early stage
unintentionally in the future.
 2. In some race condition or different kernel configuration, is there
possibility some code with "%p" run before crng is ready on one
machine, but after on another?
 3. I noticed other outputs in dmesg have the same issue (I deleted
the timestamp to make log in one line in the mail),

Virtual kernel memory layout:
ÂÂÂÂmodules : 0xffff000000000000 - 0xffff000008000000ÂÂÂ(ÂÂÂ128 MB)
ÂÂÂÂvmalloc : 0xffff000008000000 - 0xffff7dffbfff0000ÂÂÂ(129022 GB)
ÂÂÂÂÂÂ.text : 0xÂÂÂÂÂÂÂÂ(ptrval) - 0xÂÂÂÂÂÂÂÂ(ptrval)ÂÂÂ(ÂÂ9600 KB)
ÂÂÂÂ.rodata : 0xÂÂÂÂÂÂÂÂ(ptrval) - 0xÂÂÂÂÂÂÂÂ(ptrval)ÂÂÂ(ÂÂ4544 KB)

...

Remapping and enabling EFI services.
 EFI remap 0x0000000000200000 =>ÂÂÂÂÂÂÂÂÂ(ptrval)
 EFI remap 0x0000000003080000 =>ÂÂÂÂÂÂÂÂÂ(ptrval)
 EFI remap 0x0000000006300000 =>ÂÂÂÂÂÂÂÂÂ(ptrval)
...

Thanks
Shunyong