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

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

[Yang Shunyong]

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.
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.
Following is the output after applying the patch,

[root@y irq]# ls domains/
default                   irqchip@00000000(ptrval)-2
irqchip@00000000(ptrval)-4  \_SB_.TCS0.QIC1  \_SB_.TCS0.QIC3
irqchip@00000000(ptrval)  irqchip@00000000(ptrval)-3  \_SB_.TCS0.QIC0
\_SB_.TCS0.QIC2

I am not sure whether crng can get enough random data at very early
stage of system startup (eg. before irq system in the case above)
and the effort to port current random driver to work at that time.
So, this issue may exist some time.
I use '0' for padding in this patch. This should be ok because output
of "%pK" is all '0's when kptr_restrict = 2. I don't know whether
other character, such as 'x', may be more preferable.

Signed-off-by: Yang Shunyong <shunyong.yang@hxt-semitech.com>
---
 lib/vsprintf.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 01c3957b2de6..e0b6e1edae31 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -535,14 +535,18 @@ char *special_hex_number(char *buf, char *end, unsigned long long num, int size)
 	return number(buf, end, num, spec);
 }
 
-static void move_right(char *buf, char *end, unsigned len, unsigned spaces)
+static void move_right(char *buf, char *end, unsigned int len,
+		       unsigned int spaces, struct printf_spec spec)
 {
 	size_t size;
+	char pad;
+
+	pad = (spec.flags & ZEROPAD) ? '0' : ' ';
 	if (buf >= end)	/* nowhere to put anything */
 		return;
 	size = end - buf;
 	if (size <= spaces) {
-		memset(buf, ' ', size);
+		memset(buf, pad, size);
 		return;
 	}
 	if (len) {
@@ -550,7 +554,7 @@ static void move_right(char *buf, char *end, unsigned len, unsigned spaces)
 			len = size - spaces;
 		memmove(buf + spaces, buf, len);
 	}
-	memset(buf, ' ', spaces);
+	memset(buf, pad, spaces);
 }
 
 /*
@@ -565,18 +569,21 @@ static void move_right(char *buf, char *end, unsigned len, unsigned spaces)
 char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
 {
 	unsigned spaces;
+	char pad;
 
 	if (likely(n >= spec.field_width))
 		return buf;
 	/* we want to pad the sucker */
 	spaces = spec.field_width - n;
 	if (!(spec.flags & LEFT)) {
-		move_right(buf - n, end, n, spaces);
+		move_right(buf - n, end, n, spaces, spec);
 		return buf + spaces;
 	}
+
+	pad = (spec.flags & ZEROPAD) ? '0' : ' ';
 	while (spaces--) {
 		if (buf < end)
-			*buf = ' ';
+			*buf = pad;
 		++buf;
 	}
 	return buf;
@@ -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);
 	}
-- 
1.8.3.1

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

[Andy Shevchenko]

+Rasmus

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.

Did you run tests?

Have you added specific test cases to see what's going on for patterns
like

printf("%0s\n", "    (my string)");

?

> Following is the output after applying the patch,
> 
> [root@y irq]# ls domains/
> default                   irqchip@00000000(ptrval)-2
> irqchip@00000000(ptrval)-4  \_SB_.TCS0.QIC1  \_SB_.TCS0.QIC3
> irqchip@00000000(ptrval)  irqchip@00000000(ptrval)-3  \_SB_.TCS0.QIC0
> \_SB_.TCS0.QIC2
> 

So, for me it looks like curing symptoms. After all, it's debugfs, no
one prevents you to fix an output of the certain nodes there.

> I am not sure whether crng can get enough random data at very early
> stage of system startup (eg. before irq system in the case above)
> and the effort to port current random driver to work at that time.
> So, this issue may exist some time.
> I use '0' for padding in this patch. This should be ok because output
> of "%pK" is all '0's when kptr_restrict = 2. I don't know whether
> other character, such as 'x', may be more preferable.
> 
> Signed-off-by: Yang Shunyong <shunyong.yang@hxt-semitech.com>
> ---
>  lib/vsprintf.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 01c3957b2de6..e0b6e1edae31 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -535,14 +535,18 @@ char *special_hex_number(char *buf, char *end,
> unsigned long long num, int size)
>  	return number(buf, end, num, spec);
>  }
>  
> -static void move_right(char *buf, char *end, unsigned len, unsigned
> spaces)
> +static void move_right(char *buf, char *end, unsigned int len,
> +		       unsigned int spaces, struct printf_spec spec)
>  {
>  	size_t size;
> +	char pad;
> +
> +	pad = (spec.flags & ZEROPAD) ? '0' : ' ';
>  	if (buf >= end)	/* nowhere to put anything */
>  		return;
>  	size = end - buf;
>  	if (size <= spaces) {
> -		memset(buf, ' ', size);
> +		memset(buf, pad, size);
>  		return;
>  	}
>  	if (len) {
> @@ -550,7 +554,7 @@ static void move_right(char *buf, char *end,
> unsigned len, unsigned spaces)
>  			len = size - spaces;
>  		memmove(buf + spaces, buf, len);
>  	}
> -	memset(buf, ' ', spaces);
> +	memset(buf, pad, spaces);
>  }
>  
>  /*
> @@ -565,18 +569,21 @@ static void move_right(char *buf, char *end,
> unsigned len, unsigned spaces)
>  char *widen_string(char *buf, int n, char *end, struct printf_spec
> spec)
>  {
>  	unsigned spaces;
> +	char pad;
>  
>  	if (likely(n >= spec.field_width))
>  		return buf;
>  	/* we want to pad the sucker */
>  	spaces = spec.field_width - n;
>  	if (!(spec.flags & LEFT)) {
> -		move_right(buf - n, end, n, spaces);
> +		move_right(buf - n, end, n, spaces, spec);
>  		return buf + spaces;
>  	}
> +
> +	pad = (spec.flags & ZEROPAD) ? '0' : ' ';
>  	while (spaces--) {
>  		if (buf < end)
> -			*buf = ' ';
> +			*buf = pad;
>  		++buf;
>  	}
>  	return buf;
> @@ -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);
>  	}

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Rasmus Villemoes]

On 26 January 2018 at 10:17, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> 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
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.

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.

Rasmus

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@linux.intel.com> 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