* [PATCH] vsprintf: avoid misleading "(null)" for %px @ 2018-02-04 17:45 Adam Borowski 2018-02-05 9:44 ` Petr Mladek 0 siblings, 1 reply; 20+ messages in thread From: Adam Borowski @ 2018-02-04 17:45 UTC (permalink / raw) To: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, linux-kernel Cc: Adam Borowski Like %pK already does, print "00000000" instead. This confused people -- the convention is that "(null)" means you tried to dereference a null pointer as opposed to printing the address. Signed-off-by: Adam Borowski <kilobyte@angband.pl> --- lib/vsprintf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 77ee6ced11b1..d7a708f82559 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1849,7 +1849,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, { const int default_width = 2 * sizeof(void *); - if (!ptr && *fmt != 'K') { + if (!ptr && *fmt != 'K' && *fmt != 'x') { /* * Print (null) with the same width as a pointer so it makes * tabular output look nice. -- 2.15.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-04 17:45 [PATCH] vsprintf: avoid misleading "(null)" for %px Adam Borowski @ 2018-02-05 9:44 ` Petr Mladek 2018-02-05 10:03 ` Tobin C. Harding 2018-02-05 18:57 ` Kees Cook 0 siblings, 2 replies; 20+ messages in thread From: Petr Mladek @ 2018-02-05 9:44 UTC (permalink / raw) To: Adam Borowski Cc: Sergey Senozhatsky, Steven Rostedt, linux-kernel, Tobin C. Harding, Andrew Morton, Joe Perches, Kees Cook, Roberts, William C, Linus Torvalds, David Laight, Randy Dunlap, Geert Uytterhoeven Hi, I add people who actively commented on adding %px modifier, see the thread starting at https://lkml.kernel.org/r/1511921105-3647-5-git-send-email-me@tobin.cc Just for reference. It seems to be related to the commit 9f36e2c448007b54 ("printk: use %pK for /proc/kallsyms and /proc/modules"). On Sun 2018-02-04 18:45:21, Adam Borowski wrote: > Like %pK already does, print "00000000" instead. > > This confused people -- the convention is that "(null)" means you tried to > dereference a null pointer as opposed to printing the address. By other words, this avoids regressions when people convert %x to %px. Do I get it right, please? > Signed-off-by: Adam Borowski <kilobyte@angband.pl> > --- > lib/vsprintf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 77ee6ced11b1..d7a708f82559 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -1849,7 +1849,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, > { > const int default_width = 2 * sizeof(void *); > > - if (!ptr && *fmt != 'K') { > + if (!ptr && *fmt != 'K' && *fmt != 'x') { > /* > * Print (null) with the same width as a pointer so it makes > * tabular output look nice. > -- > 2.15.1 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-05 9:44 ` Petr Mladek @ 2018-02-05 10:03 ` Tobin C. Harding 2018-02-05 15:22 ` Adam Borowski 2018-02-05 18:57 ` Kees Cook 1 sibling, 1 reply; 20+ messages in thread From: Tobin C. Harding @ 2018-02-05 10:03 UTC (permalink / raw) To: Petr Mladek Cc: Adam Borowski, Sergey Senozhatsky, Steven Rostedt, linux-kernel, Andrew Morton, Joe Perches, Kees Cook, Roberts, William C, Linus Torvalds, David Laight, Randy Dunlap, Geert Uytterhoeven On Mon, Feb 05, 2018 at 10:44:38AM +0100, Petr Mladek wrote: > Hi, > > I add people who actively commented on adding %px modifier, > see the thread starting at > https://lkml.kernel.org/r/1511921105-3647-5-git-send-email-me@tobin.cc > > Just for reference. It seems to be related to the commit 9f36e2c448007b54 > ("printk: use %pK for /proc/kallsyms and /proc/modules"). > > > On Sun 2018-02-04 18:45:21, Adam Borowski wrote: > > Like %pK already does, print "00000000" instead. > > > > This confused people -- the convention is that "(null)" means you tried to > > dereference a null pointer as opposed to printing the address. > > By other words, this avoids regressions when people convert > %x to %px. Do I get it right, please? > > > Signed-off-by: Adam Borowski <kilobyte@angband.pl> > > --- > > lib/vsprintf.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > > index 77ee6ced11b1..d7a708f82559 100644 > > --- a/lib/vsprintf.c > > +++ b/lib/vsprintf.c > > @@ -1849,7 +1849,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, > > { > > const int default_width = 2 * sizeof(void *); > > > > - if (!ptr && *fmt != 'K') { > > + if (!ptr && *fmt != 'K' && *fmt != 'x') { I don't know if it matters but with this it won't be immediately apparent that a null pointer was printed (since zero could hash to anything). thanks, Tobin. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-05 10:03 ` Tobin C. Harding @ 2018-02-05 15:22 ` Adam Borowski 2018-02-05 16:49 ` Steven Rostedt 0 siblings, 1 reply; 20+ messages in thread From: Adam Borowski @ 2018-02-05 15:22 UTC (permalink / raw) To: Tobin C. Harding Cc: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, linux-kernel, Andrew Morton, Joe Perches, Kees Cook, Roberts, William C, Linus Torvalds, David Laight, Randy Dunlap, Geert Uytterhoeven On Mon, Feb 05, 2018 at 09:03:05PM +1100, Tobin C. Harding wrote: > On Mon, Feb 05, 2018 at 10:44:38AM +0100, Petr Mladek wrote: > > On Sun 2018-02-04 18:45:21, Adam Borowski wrote: > > > Like %pK already does, print "00000000" instead. > > > > > > This confused people -- the convention is that "(null)" means you tried to > > > dereference a null pointer as opposed to printing the address. > > > > By other words, this avoids regressions when people convert > > %x to %px. Do I get it right, please? It's a regression in the sense that it confuses people. %px never could dereference a pointer so the information provided doesn't change, merely its presentation. > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > > > index 77ee6ced11b1..d7a708f82559 100644 > > > --- a/lib/vsprintf.c > > > +++ b/lib/vsprintf.c > > > @@ -1849,7 +1849,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, > > > { > > > const int default_width = 2 * sizeof(void *); > > > > > > - if (!ptr && *fmt != 'K') { > > > + if (!ptr && *fmt != 'K' && *fmt != 'x') { > > I don't know if it matters but with this it won't be immediately > apparent that a null pointer was printed (since zero could hash to > anything). My change touches %px only, where your concern doesn't apply. You're right, though, when it comes to %pK: printk("%%pK: %pK, %%px: %px\n", 0, 0); says %pK: 00000000ba8bdc0a, %px: 0000000000000000 So what should we do? Avoid hashing 0? Print a special value? Meow! -- ⢀⣴⠾⠻⢶⣦⠀ The bill with 3 years prison for mentioning Polish concentration ⣾⠁⢰⠒⠀⣿⡁ camps is back. What about KL Warschau (operating until 1956)? ⢿⡄⠘⠷⠚⠋⠀ Zgoda? Łambinowice? Most ex-German KLs? If those were "soviet ⠈⠳⣄⠀⠀⠀⠀ puppets", Bereza Kartuska? Sikorski's camps in UK (thanks Brits!)? ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-05 15:22 ` Adam Borowski @ 2018-02-05 16:49 ` Steven Rostedt 2018-02-05 17:36 ` Randy Dunlap 0 siblings, 1 reply; 20+ messages in thread From: Steven Rostedt @ 2018-02-05 16:49 UTC (permalink / raw) To: Adam Borowski Cc: Tobin C. Harding, Petr Mladek, Sergey Senozhatsky, linux-kernel, Andrew Morton, Joe Perches, Kees Cook, Roberts, William C, Linus Torvalds, David Laight, Randy Dunlap, Geert Uytterhoeven On Mon, 5 Feb 2018 16:22:19 +0100 Adam Borowski <kilobyte@angband.pl> wrote: > My change touches %px only, where your concern doesn't apply. > > You're right, though, when it comes to %pK: > printk("%%pK: %pK, %%px: %px\n", 0, 0); > says > %pK: 00000000ba8bdc0a, %px: 0000000000000000 > > So what should we do? Avoid hashing 0? Print a special value? My personal opinion is that NULL should stay NULL and not be hashed. What security issue could be leaked by a NULL? I'm not a security person, that's a real question. -- Steve ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-05 16:49 ` Steven Rostedt @ 2018-02-05 17:36 ` Randy Dunlap 2018-02-05 20:19 ` Tobin C. Harding 0 siblings, 1 reply; 20+ messages in thread From: Randy Dunlap @ 2018-02-05 17:36 UTC (permalink / raw) To: Steven Rostedt, Adam Borowski Cc: Tobin C. Harding, Petr Mladek, Sergey Senozhatsky, linux-kernel, Andrew Morton, Joe Perches, Kees Cook, Roberts, William C, Linus Torvalds, David Laight, Geert Uytterhoeven On 02/05/2018 08:49 AM, Steven Rostedt wrote: > On Mon, 5 Feb 2018 16:22:19 +0100 > Adam Borowski <kilobyte@angband.pl> wrote: > >> My change touches %px only, where your concern doesn't apply. >> >> You're right, though, when it comes to %pK: >> printk("%%pK: %pK, %%px: %px\n", 0, 0); >> says >> %pK: 00000000ba8bdc0a, %px: 0000000000000000 >> >> So what should we do? Avoid hashing 0? Print a special value? > > My personal opinion is that NULL should stay NULL and not be hashed. > What security issue could be leaked by a NULL? I'm not a security > person, that's a real question. Agree. -- ~Randy ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-05 17:36 ` Randy Dunlap @ 2018-02-05 20:19 ` Tobin C. Harding 0 siblings, 0 replies; 20+ messages in thread From: Tobin C. Harding @ 2018-02-05 20:19 UTC (permalink / raw) To: Randy Dunlap Cc: Steven Rostedt, Adam Borowski, Petr Mladek, Sergey Senozhatsky, linux-kernel, Andrew Morton, Joe Perches, Kees Cook, Roberts, William C, Linus Torvalds, David Laight, Geert Uytterhoeven On Mon, Feb 05, 2018 at 09:36:03AM -0800, Randy Dunlap wrote: > On 02/05/2018 08:49 AM, Steven Rostedt wrote: > > On Mon, 5 Feb 2018 16:22:19 +0100 > > Adam Borowski <kilobyte@angband.pl> wrote: > > > >> My change touches %px only, where your concern doesn't apply. > >> > >> You're right, though, when it comes to %pK: > >> printk("%%pK: %pK, %%px: %px\n", 0, 0); > >> says > >> %pK: 00000000ba8bdc0a, %px: 0000000000000000 > >> > >> So what should we do? Avoid hashing 0? Print a special value? > > > > My personal opinion is that NULL should stay NULL and not be hashed. > > What security issue could be leaked by a NULL? I'm not a security > > person, that's a real question. > > Agree. While these views seem valid I don't think we are going to get much love trying to change %pK to give a smidgen more information when %pK is arguably out of favour :) Tobin ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-05 9:44 ` Petr Mladek 2018-02-05 10:03 ` Tobin C. Harding @ 2018-02-05 18:57 ` Kees Cook 2018-02-05 20:15 ` Tobin C. Harding 1 sibling, 1 reply; 20+ messages in thread From: Kees Cook @ 2018-02-05 18:57 UTC (permalink / raw) To: Petr Mladek Cc: Adam Borowski, Sergey Senozhatsky, Steven Rostedt, LKML, Tobin C. Harding, Andrew Morton, Joe Perches, Roberts, William C, Linus Torvalds, David Laight, Randy Dunlap, Geert Uytterhoeven On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek <pmladek@suse.com> wrote: > Hi, > > I add people who actively commented on adding %px modifier, > see the thread starting at > https://lkml.kernel.org/r/1511921105-3647-5-git-send-email-me@tobin.cc > > Just for reference. It seems to be related to the commit 9f36e2c448007b54 > ("printk: use %pK for /proc/kallsyms and /proc/modules"). > > > On Sun 2018-02-04 18:45:21, Adam Borowski wrote: >> Like %pK already does, print "00000000" instead. >> >> This confused people -- the convention is that "(null)" means you tried to >> dereference a null pointer as opposed to printing the address. > > By other words, this avoids regressions when people convert > %x to %px. Do I get it right, please? Nothing should be converting from %x to %px, it's %p to %px. %p print "(null)" for 0x0, so it would be surprising for a conversion from %p to %px to change that. (Though generally speaking "(null)" is never useful...) -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-05 18:57 ` Kees Cook @ 2018-02-05 20:15 ` Tobin C. Harding 2018-02-05 20:32 ` Kees Cook 0 siblings, 1 reply; 20+ messages in thread From: Tobin C. Harding @ 2018-02-05 20:15 UTC (permalink / raw) To: Kees Cook Cc: Petr Mladek, Adam Borowski, Sergey Senozhatsky, Steven Rostedt, LKML, Andrew Morton, Joe Perches, Roberts, William C, Linus Torvalds, David Laight, Randy Dunlap, Geert Uytterhoeven On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote: > On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek <pmladek@suse.com> wrote: > > Hi, > > > > I add people who actively commented on adding %px modifier, > > see the thread starting at > > https://lkml.kernel.org/r/1511921105-3647-5-git-send-email-me@tobin.cc > > > > Just for reference. It seems to be related to the commit 9f36e2c448007b54 > > ("printk: use %pK for /proc/kallsyms and /proc/modules"). > > > > > > On Sun 2018-02-04 18:45:21, Adam Borowski wrote: > >> Like %pK already does, print "00000000" instead. > >> > >> This confused people -- the convention is that "(null)" means you tried to > >> dereference a null pointer as opposed to printing the address. > > > > By other words, this avoids regressions when people convert > > %x to %px. Do I get it right, please? > > Nothing should be converting from %x to %px, it's %p to %px. %p print > "(null)" for 0x0, so it would be surprising for a conversion from %p > to %px to change that. (Though generally speaking "(null)" is never > useful...) Leaving aside what is converting to %px. If we consider that using %px is meant to convey to us that we _really_ want the address, in hex hence the 'x', then it is not surprising that we will get "00000000"'s for a null pointer, right? Yes it is different to before but since we are changing the specifier does this not imply that there may be some change? In what is now to be expected fashion for %p the discussion appears to have split into two different things - what to do with %px and what to do with %pK :) thanks, Tobin. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-05 20:15 ` Tobin C. Harding @ 2018-02-05 20:32 ` Kees Cook 2018-02-05 20:58 ` Adam Borowski 0 siblings, 1 reply; 20+ messages in thread From: Kees Cook @ 2018-02-05 20:32 UTC (permalink / raw) To: Tobin C. Harding Cc: Petr Mladek, Adam Borowski, Sergey Senozhatsky, Steven Rostedt, LKML, Andrew Morton, Joe Perches, Roberts, William C, Linus Torvalds, David Laight, Randy Dunlap, Geert Uytterhoeven On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding <me@tobin.cc> wrote: > On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote: >> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek <pmladek@suse.com> wrote: >> > Hi, >> > >> > I add people who actively commented on adding %px modifier, >> > see the thread starting at >> > https://lkml.kernel.org/r/1511921105-3647-5-git-send-email-me@tobin.cc >> > >> > Just for reference. It seems to be related to the commit 9f36e2c448007b54 >> > ("printk: use %pK for /proc/kallsyms and /proc/modules"). >> > >> > >> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote: >> >> Like %pK already does, print "00000000" instead. >> >> >> >> This confused people -- the convention is that "(null)" means you tried to >> >> dereference a null pointer as opposed to printing the address. >> > >> > By other words, this avoids regressions when people convert >> > %x to %px. Do I get it right, please? >> >> Nothing should be converting from %x to %px, it's %p to %px. %p print >> "(null)" for 0x0, so it would be surprising for a conversion from %p >> to %px to change that. (Though generally speaking "(null)" is never >> useful...) > > Leaving aside what is converting to %px. If we consider that using %px > is meant to convey to us that we _really_ want the address, in hex hence > the 'x', then it is not surprising that we will get "00000000"'s for a > null pointer, right? Yes it is different to before but since we are > changing the specifier does this not imply that there may be some > change? I personally prefer 0000s, but if we're going to change this, we need to be aware of the difference. > In what is now to be expected fashion for %p the discussion appears to > have split into two different things - what to do with %px and what to > do with %pK :) I say leave %pK alone. :) -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-05 20:32 ` Kees Cook @ 2018-02-05 20:58 ` Adam Borowski 2018-02-05 22:22 ` Tobin C. Harding 2018-02-07 15:03 ` Petr Mladek 0 siblings, 2 replies; 20+ messages in thread From: Adam Borowski @ 2018-02-05 20:58 UTC (permalink / raw) To: Kees Cook Cc: Tobin C. Harding, Petr Mladek, Sergey Senozhatsky, Steven Rostedt, LKML, Andrew Morton, Joe Perches, Roberts, William C, Linus Torvalds, David Laight, Randy Dunlap, Geert Uytterhoeven On Tue, Feb 06, 2018 at 07:32:32AM +1100, Kees Cook wrote: > On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding <me@tobin.cc> wrote: > > On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote: > >> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek <pmladek@suse.com> wrote: > >> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote: > >> >> Like %pK already does, print "00000000" instead. > >> >> > >> >> This confused people -- the convention is that "(null)" means you tried to > >> >> dereference a null pointer as opposed to printing the address. > > > > Leaving aside what is converting to %px. If we consider that using %px > > is meant to convey to us that we _really_ want the address, in hex hence > > the 'x', then it is not surprising that we will get "00000000"'s for a > > null pointer, right? Yes it is different to before but since we are > > changing the specifier does this not imply that there may be some > > change? > > I personally prefer 0000s, but if we're going to change this, we need > to be aware of the difference. It's easy to paint this bikeshed any color you guys want to: there's an "if" already. My preference is also 0000; NULL would be good, too -- I just don't want (null) as that has a special meaning in usual userspace implementations; (null) also fits well most other modes of %p as they show some object the argument points to. Confusion = wasted debugging time. This is consistent with what we had before, with %pK special-cased. > > In what is now to be expected fashion for %p the discussion appears to > > have split into two different things - what to do with %px and what to > > do with %pK :) > > I say leave %pK alone. :) As in, printing some random (hashed) value? Let's recap: Currently: not-null null %pponies object's description (null) %px address (null) %pK hash hash I'd propose: not-null null %pponies object's description (null) %px address 00000000 %pK hash 00000000 The initial patch in this thread changes printk("%px",0) from (null) to 00000000; what Tobin complained about is that printk("%pK",0) prints a random value. Meow! -- ⢀⣴⠾⠻⢶⣦⠀ The bill with 3 years prison for mentioning Polish concentration ⣾⠁⢰⠒⠀⣿⡁ camps is back. What about KL Warschau (operating until 1956)? ⢿⡄⠘⠷⠚⠋⠀ Zgoda? Łambinowice? Most ex-German KLs? If those were "soviet ⠈⠳⣄⠀⠀⠀⠀ puppets", Bereza Kartuska? Sikorski's camps in UK (thanks Brits!)? ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-05 20:58 ` Adam Borowski @ 2018-02-05 22:22 ` Tobin C. Harding 2018-02-06 18:43 ` Andy Shevchenko 2018-02-07 15:03 ` Petr Mladek 1 sibling, 1 reply; 20+ messages in thread From: Tobin C. Harding @ 2018-02-05 22:22 UTC (permalink / raw) To: Adam Borowski Cc: Kees Cook, Petr Mladek, Sergey Senozhatsky, Steven Rostedt, LKML, Andrew Morton, Joe Perches, Roberts, William C, Linus Torvalds, David Laight, Randy Dunlap, Geert Uytterhoeven On Mon, Feb 05, 2018 at 09:58:17PM +0100, Adam Borowski wrote: > On Tue, Feb 06, 2018 at 07:32:32AM +1100, Kees Cook wrote: > > On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding <me@tobin.cc> wrote: > > > On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote: > > >> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek <pmladek@suse.com> wrote: > > >> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote: > > >> >> Like %pK already does, print "00000000" instead. > > >> >> > > >> >> This confused people -- the convention is that "(null)" means you tried to > > >> >> dereference a null pointer as opposed to printing the address. > > > > > > Leaving aside what is converting to %px. If we consider that using %px > > > is meant to convey to us that we _really_ want the address, in hex hence > > > the 'x', then it is not surprising that we will get "00000000"'s for a > > > null pointer, right? Yes it is different to before but since we are > > > changing the specifier does this not imply that there may be some > > > change? > > > > I personally prefer 0000s, but if we're going to change this, we need > > to be aware of the difference. > > It's easy to paint this bikeshed any color you guys want to: there's an "if" > already. My preference is also 0000; NULL would be good, too -- I just > don't want (null) as that has a special meaning in usual userspace > implementations; (null) also fits well most other modes of %p as they show > some object the argument points to. Confusion = wasted debugging time. > > This is consistent with what we had before, with %pK special-cased. > > > > In what is now to be expected fashion for %p the discussion appears to > > > have split into two different things - what to do with %px and what to > > > do with %pK :) > > > > I say leave %pK alone. :) > > As in, printing some random (hashed) value? > > > Let's recap: > > Currently: > not-null null > %pponies object's description (null) > %px address (null) > %pK hash hash > > I'd propose: > not-null null > %pponies object's description (null) > %px address 00000000 > %pK hash 00000000 > > The initial patch in this thread changes printk("%px",0) from (null) to > 00000000; what Tobin complained about is that printk("%pK",0) prints a > random value. Epic fail on my behalf, my first comment was _wrong_ and brought %pK into the discussion - bad Tobin, please crawl back under your rock. The original patch is good IMO and I AFAICT in everyone else's. Tobin ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-05 22:22 ` Tobin C. Harding @ 2018-02-06 18:43 ` Andy Shevchenko 0 siblings, 0 replies; 20+ messages in thread From: Andy Shevchenko @ 2018-02-06 18:43 UTC (permalink / raw) To: Tobin C. Harding Cc: Adam Borowski, Kees Cook, Petr Mladek, Sergey Senozhatsky, Steven Rostedt, LKML, Andrew Morton, Joe Perches, Roberts, William C, Linus Torvalds, David Laight, Randy Dunlap, Geert Uytterhoeven On Tue, Feb 6, 2018 at 12:22 AM, Tobin C. Harding <me@tobin.cc> wrote: > The original patch is good IMO and I AFAICT in everyone else's. The original patch misses test cases. Without them is problematic to follow what's going on with printing. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-05 20:58 ` Adam Borowski 2018-02-05 22:22 ` Tobin C. Harding @ 2018-02-07 15:03 ` Petr Mladek 2018-02-07 15:11 ` Geert Uytterhoeven 1 sibling, 1 reply; 20+ messages in thread From: Petr Mladek @ 2018-02-07 15:03 UTC (permalink / raw) To: Adam Borowski Cc: Kees Cook, Tobin C. Harding, Sergey Senozhatsky, Steven Rostedt, LKML, Andrew Morton, Joe Perches, Roberts, William C, Linus Torvalds, David Laight, Randy Dunlap, Geert Uytterhoeven On Mon 2018-02-05 21:58:17, Adam Borowski wrote: > On Tue, Feb 06, 2018 at 07:32:32AM +1100, Kees Cook wrote: > > On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding <me@tobin.cc> wrote: > > > On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote: > > >> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek <pmladek@suse.com> wrote: > > >> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote: > > >> >> Like %pK already does, print "00000000" instead. > > >> >> > > >> >> This confused people -- the convention is that "(null)" means you tried to > > >> >> dereference a null pointer as opposed to printing the address. > > > > > > Leaving aside what is converting to %px. If we consider that using %px > > > is meant to convey to us that we _really_ want the address, in hex hence > > > the 'x', then it is not surprising that we will get "00000000"'s for a > > > null pointer, right? Yes it is different to before but since we are > > > changing the specifier does this not imply that there may be some > > > change? > > > > I personally prefer 0000s, but if we're going to change this, we need > > to be aware of the difference. > > It's easy to paint this bikeshed any color you guys want to: there's an "if" > already. My preference is also 0000; NULL would be good, too -- I just > don't want (null) as that has a special meaning in usual userspace > implementations; (null) also fits well most other modes of %p as they show > some object the argument points to. Confusion = wasted debugging time. > Let's recap: > > Currently: > not-null null > %pponies object's description (null) > %px address (null) > %pK hash hash > > I'd propose: > not-null null > %pponies object's description (null) > %px address 00000000 > %pK hash 00000000 It makes sense to me[*], so Reviewed-by: Petr Mladek <pmladek@suse.com> It seems that all people agree with this change. But there was also some confusion. I am going to give it few more days before I push it to Linus. It means waiting for 4.16-rc3 because I will be without reliable internet next week. Anyone, feel free to push it faster. [*] I made some archaeology: The "(null)" string was added by the commit d97106ab53f812910 ("Make %p print '(null)' for NULL pointers"). It was a generic solution to prevent eventual crashes, see https://lkml.kernel.org/r/1230979341-23029-1-git-send-email-xyzzy@speakeasy.org >From this point, printing 00000000 for %px looks perfectly fine because it does not crash. In fact, it would have made perfect sense to print 00000000 for pure %p because it did not crash. But nobody has cared about the eventual confusion yet. I am not sure if it makes sense to change the pure %p handling now. Note that printing "(null)" has the advantage that we get this string instead of the hash ;-) Best Regards, Petr ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-07 15:03 ` Petr Mladek @ 2018-02-07 15:11 ` Geert Uytterhoeven 2018-02-07 15:41 ` Petr Mladek 0 siblings, 1 reply; 20+ messages in thread From: Geert Uytterhoeven @ 2018-02-07 15:11 UTC (permalink / raw) To: Petr Mladek Cc: Adam Borowski, Kees Cook, Tobin C. Harding, Sergey Senozhatsky, Steven Rostedt, LKML, Andrew Morton, Joe Perches, Roberts, William C, Linus Torvalds, David Laight, Randy Dunlap Hi Petr, On Wed, Feb 7, 2018 at 4:03 PM, Petr Mladek <pmladek@suse.com> wrote: > On Mon 2018-02-05 21:58:17, Adam Borowski wrote: >> On Tue, Feb 06, 2018 at 07:32:32AM +1100, Kees Cook wrote: >> > On Tue, Feb 6, 2018 at 7:15 AM, Tobin C. Harding <me@tobin.cc> wrote: >> > > On Tue, Feb 06, 2018 at 05:57:17AM +1100, Kees Cook wrote: >> > >> On Mon, Feb 5, 2018 at 8:44 PM, Petr Mladek <pmladek@suse.com> wrote: >> > >> > On Sun 2018-02-04 18:45:21, Adam Borowski wrote: >> > >> >> Like %pK already does, print "00000000" instead. >> > >> >> >> > >> >> This confused people -- the convention is that "(null)" means you tried to >> > >> >> dereference a null pointer as opposed to printing the address. >> > > >> > > Leaving aside what is converting to %px. If we consider that using %px >> > > is meant to convey to us that we _really_ want the address, in hex hence >> > > the 'x', then it is not surprising that we will get "00000000"'s for a >> > > null pointer, right? Yes it is different to before but since we are >> > > changing the specifier does this not imply that there may be some >> > > change? >> > >> > I personally prefer 0000s, but if we're going to change this, we need >> > to be aware of the difference. >> >> It's easy to paint this bikeshed any color you guys want to: there's an "if" >> already. My preference is also 0000; NULL would be good, too -- I just >> don't want (null) as that has a special meaning in usual userspace >> implementations; (null) also fits well most other modes of %p as they show >> some object the argument points to. Confusion = wasted debugging time. > >> Let's recap: >> >> Currently: >> not-null null >> %pponies object's description (null) >> %px address (null) >> %pK hash hash >> >> I'd propose: >> not-null null >> %pponies object's description (null) >> %px address 00000000 >> %pK hash 00000000 > > It makes sense to me[*], so > > Reviewed-by: Petr Mladek <pmladek@suse.com> > > It seems that all people agree with this change. But there was also some > confusion. I am going to give it few more days before I push it to > Linus. It means waiting for 4.16-rc3 because I will be without > reliable internet next week. Anyone, feel free to push it faster. > > > [*] I made some archaeology: > > The "(null)" string was added by the commit d97106ab53f812910 > ("Make %p print '(null)' for NULL pointers"). > > It was a generic solution to prevent eventual crashes, see > https://lkml.kernel.org/r/1230979341-23029-1-git-send-email-xyzzy@speakeasy.org > > From this point, printing 00000000 for %px looks perfectly fine because > it does not crash. > > In fact, it would have made perfect sense to print 00000000 for pure > %p because it did not crash. But nobody has cared about the eventual > confusion yet. > > I am not sure if it makes sense to change the pure %p handling > now. Note that printing "(null)" has the advantage that we > get this string instead of the hash ;-) Note that "(null)" is also used for printing strings, where you do dereference the pointer, unlike for printing pointers. In addition, "(null)" for strings is not just printed for real NULL pointers, but also for anything pointing within the first page of virtual memory. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-07 15:11 ` Geert Uytterhoeven @ 2018-02-07 15:41 ` Petr Mladek 2018-02-07 15:48 ` Geert Uytterhoeven 2018-02-08 15:29 ` Andy Shevchenko 0 siblings, 2 replies; 20+ messages in thread From: Petr Mladek @ 2018-02-07 15:41 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Adam Borowski, Kees Cook, Tobin C. Harding, Sergey Senozhatsky, Steven Rostedt, LKML, Andrew Morton, Joe Perches, Roberts, William C, Linus Torvalds, David Laight, Randy Dunlap On Wed 2018-02-07 16:11:13, Geert Uytterhoeven wrote: > Hi Petr, > > On Wed, Feb 7, 2018 at 4:03 PM, Petr Mladek <pmladek@suse.com> wrote: > > [*] I made some archaeology: > > > > The "(null)" string was added by the commit d97106ab53f812910 > > ("Make %p print '(null)' for NULL pointers"). > > > > It was a generic solution to prevent eventual crashes, see > > https://lkml.kernel.org/r/1230979341-23029-1-git-send-email-xyzzy@speakeasy.org > > > > From this point, printing 00000000 for %px looks perfectly fine because > > it does not crash. > > > > In fact, it would have made perfect sense to print 00000000 for pure > > %p because it did not crash. But nobody has cared about the eventual > > confusion yet. > > > > I am not sure if it makes sense to change the pure %p handling > > now. Note that printing "(null)" has the advantage that we > > get this string instead of the hash ;-) > > Note that "(null)" is also used for printing strings, where you do dereference > the pointer, unlike for printing pointers. > In addition, "(null)" for strings is not just printed for real NULL > pointers, but > also for anything pointing within the first page of virtual memory. We are on the safe side. "(null)" for "%s" is handled separately, see string() function in lib/vsprintf.c. To make it clear. I was talking about "%p" format that is handled in the pointer() function in lib/vsprintf.c. The "(null)" makes sense only for the many modifiers that do deference of the pointer, e.g. "%pa", "%pE", "%ph". It makes less sense for the pure "%p" used without any modifier. Well, it actually started to makes some sense after we started printing the hash instead of the real pointer value. Best Regards, Petr ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-07 15:41 ` Petr Mladek @ 2018-02-07 15:48 ` Geert Uytterhoeven 2018-02-08 15:29 ` Andy Shevchenko 1 sibling, 0 replies; 20+ messages in thread From: Geert Uytterhoeven @ 2018-02-07 15:48 UTC (permalink / raw) To: Petr Mladek Cc: Adam Borowski, Kees Cook, Tobin C. Harding, Sergey Senozhatsky, Steven Rostedt, LKML, Andrew Morton, Joe Perches, Roberts, William C, Linus Torvalds, David Laight, Randy Dunlap Hi Petr, On Wed, Feb 7, 2018 at 4:41 PM, Petr Mladek <pmladek@suse.com> wrote: > On Wed 2018-02-07 16:11:13, Geert Uytterhoeven wrote: >> On Wed, Feb 7, 2018 at 4:03 PM, Petr Mladek <pmladek@suse.com> wrote: >> > [*] I made some archaeology: >> > >> > The "(null)" string was added by the commit d97106ab53f812910 >> > ("Make %p print '(null)' for NULL pointers"). >> > >> > It was a generic solution to prevent eventual crashes, see >> > https://lkml.kernel.org/r/1230979341-23029-1-git-send-email-xyzzy@speakeasy.org >> > >> > From this point, printing 00000000 for %px looks perfectly fine because >> > it does not crash. >> > >> > In fact, it would have made perfect sense to print 00000000 for pure >> > %p because it did not crash. But nobody has cared about the eventual >> > confusion yet. >> > >> > I am not sure if it makes sense to change the pure %p handling >> > now. Note that printing "(null)" has the advantage that we >> > get this string instead of the hash ;-) >> >> Note that "(null)" is also used for printing strings, where you do dereference >> the pointer, unlike for printing pointers. >> In addition, "(null)" for strings is not just printed for real NULL >> pointers, but >> also for anything pointing within the first page of virtual memory. > > We are on the safe side. "(null)" for "%s" is handled > separately, see string() function in lib/vsprintf.c. I know. > To make it clear. I was talking about "%p" format that is handled > in the pointer() function in lib/vsprintf.c. The "(null)" makes > sense only for the many modifiers that do deference of > the pointer, e.g. "%pa", "%pE", "%ph". It makes less sense > for the pure "%p" used without any modifier. Well, it actually > started to makes some sense after we started printing > the hash instead of the real pointer value. Sure. I agree with all of that. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-07 15:41 ` Petr Mladek 2018-02-07 15:48 ` Geert Uytterhoeven @ 2018-02-08 15:29 ` Andy Shevchenko 2018-02-09 12:03 ` Petr Mladek 1 sibling, 1 reply; 20+ messages in thread From: Andy Shevchenko @ 2018-02-08 15:29 UTC (permalink / raw) To: Petr Mladek Cc: Geert Uytterhoeven, Adam Borowski, Kees Cook, Tobin C. Harding, Sergey Senozhatsky, Steven Rostedt, LKML, Andrew Morton, Joe Perches, Roberts, William C, Linus Torvalds, David Laight, Randy Dunlap On Wed, Feb 7, 2018 at 5:41 PM, Petr Mladek <pmladek@suse.com> wrote: > On Wed 2018-02-07 16:11:13, Geert Uytterhoeven wrote: > To make it clear. I was talking about "%p" format that is handled > in the pointer() function in lib/vsprintf.c. The "(null)" makes > sense only for the many modifiers that do deference of > the pointer, e.g. "%pa", "%pE", "%ph". JFYI: I have a patch to eliminate those for %pE & %ph. Google for "lib/vsprintf: Remove useless NULL checks" as a first in the series for new extension to print times. I just need time to address comments and resend the series. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-08 15:29 ` Andy Shevchenko @ 2018-02-09 12:03 ` Petr Mladek 2018-02-14 14:35 ` Andy Shevchenko 0 siblings, 1 reply; 20+ messages in thread From: Petr Mladek @ 2018-02-09 12:03 UTC (permalink / raw) To: Andy Shevchenko Cc: Geert Uytterhoeven, Adam Borowski, Kees Cook, Tobin C. Harding, Sergey Senozhatsky, Steven Rostedt, LKML, Andrew Morton, Joe Perches, Roberts, William C, Linus Torvalds, David Laight, Randy Dunlap On Thu 2018-02-08 17:29:14, Andy Shevchenko wrote: > On Wed, Feb 7, 2018 at 5:41 PM, Petr Mladek <pmladek@suse.com> wrote: > > On Wed 2018-02-07 16:11:13, Geert Uytterhoeven wrote: > > > To make it clear. I was talking about "%p" format that is handled > > in the pointer() function in lib/vsprintf.c. The "(null)" makes > > sense only for the many modifiers that do deference of > > the pointer, e.g. "%pa", "%pE", "%ph". > > JFYI: I have a patch to eliminate those for %pE & %ph. > > Google for "lib/vsprintf: Remove useless NULL checks" as a first in > the series for new extension to print times. I am slightly confused. IMHO, it makes sense to printk "(null)" for %pE and %ph. Or do you just want to avoid the duplicit check in hex_string() and escaped_string()? Well, it might be better to discuss this once you send the patch. > I just need time to address comments and resend the series. Good to know. Best Regards, Petr ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vsprintf: avoid misleading "(null)" for %px 2018-02-09 12:03 ` Petr Mladek @ 2018-02-14 14:35 ` Andy Shevchenko 0 siblings, 0 replies; 20+ messages in thread From: Andy Shevchenko @ 2018-02-14 14:35 UTC (permalink / raw) To: Petr Mladek Cc: Geert Uytterhoeven, Adam Borowski, Kees Cook, Tobin C. Harding, Sergey Senozhatsky, Steven Rostedt, LKML, Andrew Morton, Joe Perches, Roberts, William C, Linus Torvalds, David Laight, Randy Dunlap On Fri, Feb 9, 2018 at 2:03 PM, Petr Mladek <pmladek@suse.com> wrote: > On Thu 2018-02-08 17:29:14, Andy Shevchenko wrote: >> On Wed, Feb 7, 2018 at 5:41 PM, Petr Mladek <pmladek@suse.com> wrote: >> > On Wed 2018-02-07 16:11:13, Geert Uytterhoeven wrote: >> >> > To make it clear. I was talking about "%p" format that is handled >> > in the pointer() function in lib/vsprintf.c. The "(null)" makes >> > sense only for the many modifiers that do deference of >> > the pointer, e.g. "%pa", "%pE", "%ph". >> >> JFYI: I have a patch to eliminate those for %pE & %ph. >> >> Google for "lib/vsprintf: Remove useless NULL checks" as a first in >> the series for new extension to print times. > > I am slightly confused. IMHO, it makes sense to printk "(null)" > for %pE and %ph. Yes, but isn't it done by if (!ptr) check in the very beginning of the pointer() helper? > Or do you just want to avoid the duplicit check in hex_string() > and escaped_string()? And that is as well. > Well, it might be better to discuss this once you send the patch. I can Cc you, though the patch is pretty independent on the series. I can send it separately. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-02-14 14:35 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-04 17:45 [PATCH] vsprintf: avoid misleading "(null)" for %px Adam Borowski 2018-02-05 9:44 ` Petr Mladek 2018-02-05 10:03 ` Tobin C. Harding 2018-02-05 15:22 ` Adam Borowski 2018-02-05 16:49 ` Steven Rostedt 2018-02-05 17:36 ` Randy Dunlap 2018-02-05 20:19 ` Tobin C. Harding 2018-02-05 18:57 ` Kees Cook 2018-02-05 20:15 ` Tobin C. Harding 2018-02-05 20:32 ` Kees Cook 2018-02-05 20:58 ` Adam Borowski 2018-02-05 22:22 ` Tobin C. Harding 2018-02-06 18:43 ` Andy Shevchenko 2018-02-07 15:03 ` Petr Mladek 2018-02-07 15:11 ` Geert Uytterhoeven 2018-02-07 15:41 ` Petr Mladek 2018-02-07 15:48 ` Geert Uytterhoeven 2018-02-08 15:29 ` Andy Shevchenko 2018-02-09 12:03 ` Petr Mladek 2018-02-14 14:35 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).