+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 <[email protected]>
> ---
> 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 <[email protected]>
Intel Finland Oy
