From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751600AbeA0DMW (ORCPT ); Fri, 26 Jan 2018 22:12:22 -0500 Received: from mx01.hxt-semitech.com.96.203.223.in-addr.arpa ([223.203.96.7]:43556 "EHLO barracuda.hxt-semitech.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1751349AbeA0DMU (ORCPT ); Fri, 26 Jan 2018 22:12:20 -0500 X-ASG-Debug-ID: 1517022735-093b7e43052c0e0001-xx1T2L X-Barracuda-Envelope-From: shunyong.yang@hxt-semitech.com From: "Yang, Shunyong" To: "linux@rasmusvillemoes.dk" , "andriy.shevchenko@linux.intel.com" CC: "adobriyan@gmail.com" , "linux-kernel@vger.kernel.org" , "pantelis.antoniou@konsulko.com" , "mchehab@kernel.org" , "Zheng, Joey" , "akpm@linux-foundation.org" , "joe@perches.com" , "me@tobin.cc" Subject: Re: [RFC PATCH] vsprintf: add flag ZEROPAD handling before crng is ready Thread-Topic: [RFC PATCH] vsprintf: add flag ZEROPAD handling before crng is ready X-ASG-Orig-Subj: Re: [RFC PATCH] vsprintf: add flag ZEROPAD handling before crng is ready Thread-Index: AQHTloolRcB4+wyUP0yj2718wutxjaOGhhEA Date: Sat, 27 Jan 2018 03:12:24 +0000 Message-ID: <1517022734.6546.22.camel@hxt-semitech.com> References: <1516952396-7423-1-git-send-email-shunyong.yang@hxt-semitech.com> <1516958276.7000.1271.camel@linux.intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [192.168.50.12] Content-Type: text/plain; charset="utf-8" Content-ID: <93B302724EA4FA43B8F16F7A653EA2E7@hxt-semitech.com> MIME-Version: 1.0 X-Barracuda-Connect: localhost[10.128.0.14] X-Barracuda-Start-Time: 1517022735 X-Barracuda-Encrypted: ECDHE-RSA-AES256-SHA X-Barracuda-URL: https://192.168.50.101:443/cgi-mod/mark.cgi X-Barracuda-BRTS-Status: 1 X-Barracuda-Bayes: INNOCENT GLOBAL 0.5072 1.0000 0.7500 X-Barracuda-Spam-Score: 0.75 X-Barracuda-Spam-Status: No, SCORE=0.75 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=9.0 tests= X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.47303 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id w0R3CS7U014010 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 > 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