* [PATCH] vsprintf: sanely handle NULL passed to %pe [not found] <CAHk-=wjEd-gZ1g52kgi_g8gq-QCF2E01TkQd5Hmj4W5aThLw3A@mail.gmail.com> @ 2020-02-19 8:21 ` Rasmus Villemoes 2020-02-19 9:35 ` Andy Shevchenko 2020-02-19 11:20 ` Ilya Dryomov 0 siblings, 2 replies; 18+ messages in thread From: Rasmus Villemoes @ 2020-02-19 8:21 UTC (permalink / raw) To: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Rasmus Villemoes, Jonathan Corbet Cc: Ilya Dryomov, Kees Cook, Tobin C . Harding, Linus Torvalds, linux-doc, linux-kernel Extend %pe to pretty-print NULL in addition to ERR_PTRs, i.e. everything IS_ERR_OR_NULL(). Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- Something like this? The actual code change is +2,-1 with another +1 for a test case. Documentation/core-api/printk-formats.rst | 9 +++++---- lib/errname.c | 4 ++++ lib/test_printf.c | 1 + lib/vsprintf.c | 4 ++-- 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index 8ebe46b1af39..964b55291445 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -86,10 +86,11 @@ Error Pointers %pe -ENOSPC -For printing error pointers (i.e. a pointer for which IS_ERR() is true) -as a symbolic error name. Error values for which no symbolic name is -known are printed in decimal, while a non-ERR_PTR passed as the -argument to %pe gets treated as ordinary %p. +For printing error pointers (i.e. a pointer for which IS_ERR() is +true) as a symbolic error name. Error values for which no symbolic +name is known are printed in decimal. A NULL pointer is printed as +NULL. All other pointer values (i.e. anything !IS_ERR_OR_NULL()) get +treated as ordinary %p. Symbols/Function Pointers ------------------------- diff --git a/lib/errname.c b/lib/errname.c index 0c4d3e66170e..7757bc00f564 100644 --- a/lib/errname.c +++ b/lib/errname.c @@ -11,9 +11,13 @@ * allocated errnos (with EHWPOISON = 257 on parisc, and EDQUOT = 1133 * on mips), so this wastes a bit of space on those - though we * special case the EDQUOT case. + * + * For the benefit of %pe being able to print any ERR_OR_NULL pointer + * symbolically, 0 is also treated specially. */ #define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = "-" #err static const char *names_0[] = { + [0] = "NULL", E(E2BIG), E(EACCES), E(EADDRINUSE), diff --git a/lib/test_printf.c b/lib/test_printf.c index 2d9f520d2f27..3a37d0e9e735 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -641,6 +641,7 @@ errptr(void) test("[-EIO ]", "[%-8pe]", ERR_PTR(-EIO)); test("[ -EIO]", "[%8pe]", ERR_PTR(-EIO)); test("-EPROBE_DEFER", "%pe", ERR_PTR(-EPROBE_DEFER)); + test("[NULL]", "[%pe]", NULL); #endif } diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 7c488a1ce318..b7118d78eb20 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -2247,8 +2247,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, case 'x': return pointer_string(buf, end, ptr, spec); case 'e': - /* %pe with a non-ERR_PTR gets treated as plain %p */ - if (!IS_ERR(ptr)) + /* %pe with a non-ERR_OR_NULL ptr gets treated as plain %p */ + if (!IS_ERR_OR_NULL(ptr)) break; return err_ptr(buf, end, ptr, spec); } -- 2.23.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] vsprintf: sanely handle NULL passed to %pe 2020-02-19 8:21 ` [PATCH] vsprintf: sanely handle NULL passed to %pe Rasmus Villemoes @ 2020-02-19 9:35 ` Andy Shevchenko 2020-02-19 11:20 ` Ilya Dryomov 1 sibling, 0 replies; 18+ messages in thread From: Andy Shevchenko @ 2020-02-19 9:35 UTC (permalink / raw) To: Rasmus Villemoes Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Jonathan Corbet, Ilya Dryomov, Kees Cook, Tobin C . Harding, Linus Torvalds, Linux Documentation List, Linux Kernel Mailing List On Wed, Feb 19, 2020 at 10:24 AM Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > > Extend %pe to pretty-print NULL in addition to ERR_PTRs, > i.e. everything IS_ERR_OR_NULL(). > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> One nit below, though. > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > Something like this? The actual code change is +2,-1 with another +1 > for a test case. > > Documentation/core-api/printk-formats.rst | 9 +++++---- > lib/errname.c | 4 ++++ > lib/test_printf.c | 1 + > lib/vsprintf.c | 4 ++-- > 4 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst > index 8ebe46b1af39..964b55291445 100644 > --- a/Documentation/core-api/printk-formats.rst > +++ b/Documentation/core-api/printk-formats.rst > @@ -86,10 +86,11 @@ Error Pointers > > %pe -ENOSPC > > -For printing error pointers (i.e. a pointer for which IS_ERR() is true) > -as a symbolic error name. Error values for which no symbolic name is > -known are printed in decimal, while a non-ERR_PTR passed as the > -argument to %pe gets treated as ordinary %p. > +For printing error pointers (i.e. a pointer for which IS_ERR() is > +true) as a symbolic error name. Error values for which no symbolic Why to reformat these lines? > +name is known are printed in decimal. A NULL pointer is printed as > +NULL. All other pointer values (i.e. anything !IS_ERR_OR_NULL()) get > +treated as ordinary %p. > > Symbols/Function Pointers > ------------------------- > diff --git a/lib/errname.c b/lib/errname.c > index 0c4d3e66170e..7757bc00f564 100644 > --- a/lib/errname.c > +++ b/lib/errname.c > @@ -11,9 +11,13 @@ > * allocated errnos (with EHWPOISON = 257 on parisc, and EDQUOT = 1133 > * on mips), so this wastes a bit of space on those - though we > * special case the EDQUOT case. > + * > + * For the benefit of %pe being able to print any ERR_OR_NULL pointer > + * symbolically, 0 is also treated specially. > */ > #define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = "-" #err > static const char *names_0[] = { > + [0] = "NULL", > E(E2BIG), > E(EACCES), > E(EADDRINUSE), > diff --git a/lib/test_printf.c b/lib/test_printf.c > index 2d9f520d2f27..3a37d0e9e735 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -641,6 +641,7 @@ errptr(void) > test("[-EIO ]", "[%-8pe]", ERR_PTR(-EIO)); > test("[ -EIO]", "[%8pe]", ERR_PTR(-EIO)); > test("-EPROBE_DEFER", "%pe", ERR_PTR(-EPROBE_DEFER)); > + test("[NULL]", "[%pe]", NULL); > #endif > } > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 7c488a1ce318..b7118d78eb20 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -2247,8 +2247,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, > case 'x': > return pointer_string(buf, end, ptr, spec); > case 'e': > - /* %pe with a non-ERR_PTR gets treated as plain %p */ > - if (!IS_ERR(ptr)) > + /* %pe with a non-ERR_OR_NULL ptr gets treated as plain %p */ > + if (!IS_ERR_OR_NULL(ptr)) > break; > return err_ptr(buf, end, ptr, spec); > } > -- > 2.23.0 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] vsprintf: sanely handle NULL passed to %pe 2020-02-19 8:21 ` [PATCH] vsprintf: sanely handle NULL passed to %pe Rasmus Villemoes 2020-02-19 9:35 ` Andy Shevchenko @ 2020-02-19 11:20 ` Ilya Dryomov 2020-02-19 11:25 ` Andy Shevchenko 2020-02-19 11:53 ` Rasmus Villemoes 1 sibling, 2 replies; 18+ messages in thread From: Ilya Dryomov @ 2020-02-19 11:20 UTC (permalink / raw) To: Rasmus Villemoes Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Jonathan Corbet, Kees Cook, Tobin C . Harding, Linus Torvalds, linux-doc, LKML On Wed, Feb 19, 2020 at 9:21 AM Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > > Extend %pe to pretty-print NULL in addition to ERR_PTRs, > i.e. everything IS_ERR_OR_NULL(). > > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > Something like this? The actual code change is +2,-1 with another +1 > for a test case. > > Documentation/core-api/printk-formats.rst | 9 +++++---- > lib/errname.c | 4 ++++ > lib/test_printf.c | 1 + > lib/vsprintf.c | 4 ++-- > 4 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst > index 8ebe46b1af39..964b55291445 100644 > --- a/Documentation/core-api/printk-formats.rst > +++ b/Documentation/core-api/printk-formats.rst > @@ -86,10 +86,11 @@ Error Pointers > > %pe -ENOSPC > > -For printing error pointers (i.e. a pointer for which IS_ERR() is true) > -as a symbolic error name. Error values for which no symbolic name is > -known are printed in decimal, while a non-ERR_PTR passed as the > -argument to %pe gets treated as ordinary %p. > +For printing error pointers (i.e. a pointer for which IS_ERR() is > +true) as a symbolic error name. Error values for which no symbolic > +name is known are printed in decimal. A NULL pointer is printed as > +NULL. All other pointer values (i.e. anything !IS_ERR_OR_NULL()) get > +treated as ordinary %p. > > Symbols/Function Pointers > ------------------------- > diff --git a/lib/errname.c b/lib/errname.c > index 0c4d3e66170e..7757bc00f564 100644 > --- a/lib/errname.c > +++ b/lib/errname.c > @@ -11,9 +11,13 @@ > * allocated errnos (with EHWPOISON = 257 on parisc, and EDQUOT = 1133 > * on mips), so this wastes a bit of space on those - though we > * special case the EDQUOT case. > + * > + * For the benefit of %pe being able to print any ERR_OR_NULL pointer > + * symbolically, 0 is also treated specially. > */ > #define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = "-" #err > static const char *names_0[] = { > + [0] = "NULL", > E(E2BIG), > E(EACCES), > E(EADDRINUSE), > diff --git a/lib/test_printf.c b/lib/test_printf.c > index 2d9f520d2f27..3a37d0e9e735 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -641,6 +641,7 @@ errptr(void) > test("[-EIO ]", "[%-8pe]", ERR_PTR(-EIO)); > test("[ -EIO]", "[%8pe]", ERR_PTR(-EIO)); > test("-EPROBE_DEFER", "%pe", ERR_PTR(-EPROBE_DEFER)); > + test("[NULL]", "[%pe]", NULL); > #endif > } > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 7c488a1ce318..b7118d78eb20 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -2247,8 +2247,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, > case 'x': > return pointer_string(buf, end, ptr, spec); > case 'e': > - /* %pe with a non-ERR_PTR gets treated as plain %p */ > - if (!IS_ERR(ptr)) > + /* %pe with a non-ERR_OR_NULL ptr gets treated as plain %p */ > + if (!IS_ERR_OR_NULL(ptr)) > break; FWIW I was about to post a patch that just special cases NULL here. I think changing errname() to return "NULL" for 0 is overkill. People will sooner or later discover that function and start using it in contexts that don't have anything to do with pointers. Returning _some_ string for 0 (instead of NULL) makes it very close to standard strerror(), and "NULL" for 0 (i.e. success) seems rather odd. Thanks, Ilya ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] vsprintf: sanely handle NULL passed to %pe 2020-02-19 11:20 ` Ilya Dryomov @ 2020-02-19 11:25 ` Andy Shevchenko 2020-02-19 11:29 ` Ilya Dryomov 2020-02-19 11:53 ` Rasmus Villemoes 1 sibling, 1 reply; 18+ messages in thread From: Andy Shevchenko @ 2020-02-19 11:25 UTC (permalink / raw) To: Ilya Dryomov Cc: Rasmus Villemoes, Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Jonathan Corbet, Kees Cook, Tobin C . Harding, Linus Torvalds, Linux Documentation List, LKML On Wed, Feb 19, 2020 at 1:21 PM Ilya Dryomov <idryomov@gmail.com> wrote: > On Wed, Feb 19, 2020 at 9:21 AM Rasmus Villemoes > <linux@rasmusvillemoes.dk> wrote: > > > > Extend %pe to pretty-print NULL in addition to ERR_PTRs, > > i.e. everything IS_ERR_OR_NULL(). ... > > + [0] = "NULL", > > + test("[NULL]", "[%pe]", NULL); > FWIW I was about to post a patch that just special cases NULL here. > > I think changing errname() to return "NULL" for 0 is overkill. > People will sooner or later discover that function and start using it > in contexts that don't have anything to do with pointers. Returning > _some_ string for 0 (instead of NULL) makes it very close to standard > strerror(), and "NULL" for 0 (i.e. success) seems rather odd. %pe is specifically for _pointers_. I don't see a point in your comment. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] vsprintf: sanely handle NULL passed to %pe 2020-02-19 11:25 ` Andy Shevchenko @ 2020-02-19 11:29 ` Ilya Dryomov 0 siblings, 0 replies; 18+ messages in thread From: Ilya Dryomov @ 2020-02-19 11:29 UTC (permalink / raw) To: Andy Shevchenko Cc: Rasmus Villemoes, Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Jonathan Corbet, Kees Cook, Tobin C . Harding, Linus Torvalds, Linux Documentation List, LKML On Wed, Feb 19, 2020 at 12:25 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, Feb 19, 2020 at 1:21 PM Ilya Dryomov <idryomov@gmail.com> wrote: > > On Wed, Feb 19, 2020 at 9:21 AM Rasmus Villemoes > > <linux@rasmusvillemoes.dk> wrote: > > > > > > Extend %pe to pretty-print NULL in addition to ERR_PTRs, > > > i.e. everything IS_ERR_OR_NULL(). > > ... > > > > + [0] = "NULL", > > > > + test("[NULL]", "[%pe]", NULL); > > > FWIW I was about to post a patch that just special cases NULL here. > > > > I think changing errname() to return "NULL" for 0 is overkill. > > People will sooner or later discover that function and start using it > > in contexts that don't have anything to do with pointers. Returning > > _some_ string for 0 (instead of NULL) makes it very close to standard > > strerror(), and "NULL" for 0 (i.e. success) seems rather odd. > > %pe is specifically for _pointers_. I don't see a point in your comment. %pe is for pointers, but errname() in lib/errname.c will likely grow more users in the future. Thanks, Ilya ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] vsprintf: sanely handle NULL passed to %pe 2020-02-19 11:20 ` Ilya Dryomov 2020-02-19 11:25 ` Andy Shevchenko @ 2020-02-19 11:53 ` Rasmus Villemoes 2020-02-19 13:48 ` Petr Mladek 1 sibling, 1 reply; 18+ messages in thread From: Rasmus Villemoes @ 2020-02-19 11:53 UTC (permalink / raw) To: Ilya Dryomov Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Jonathan Corbet, Kees Cook, Tobin C . Harding, Linus Torvalds, linux-doc, LKML On 19/02/2020 12.20, Ilya Dryomov wrote: > On Wed, Feb 19, 2020 at 9:21 AM Rasmus Villemoes > <linux@rasmusvillemoes.dk> wrote: >> >> Extend %pe to pretty-print NULL in addition to ERR_PTRs, >> i.e. everything IS_ERR_OR_NULL(). >> >> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> >> --- >> Something like this? The actual code change is +2,-1 with another +1 >> for a test case. >> >> Documentation/core-api/printk-formats.rst | 9 +++++---- >> lib/errname.c | 4 ++++ >> lib/test_printf.c | 1 + >> lib/vsprintf.c | 4 ++-- >> 4 files changed, 12 insertions(+), 6 deletions(-) >> >> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst >> index 8ebe46b1af39..964b55291445 100644 >> --- a/Documentation/core-api/printk-formats.rst >> +++ b/Documentation/core-api/printk-formats.rst >> @@ -86,10 +86,11 @@ Error Pointers >> >> %pe -ENOSPC >> >> -For printing error pointers (i.e. a pointer for which IS_ERR() is true) >> -as a symbolic error name. Error values for which no symbolic name is >> -known are printed in decimal, while a non-ERR_PTR passed as the >> -argument to %pe gets treated as ordinary %p. >> +For printing error pointers (i.e. a pointer for which IS_ERR() is >> +true) as a symbolic error name. Error values for which no symbolic >> +name is known are printed in decimal. A NULL pointer is printed as >> +NULL. All other pointer values (i.e. anything !IS_ERR_OR_NULL()) get >> +treated as ordinary %p. >> >> Symbols/Function Pointers >> ------------------------- >> diff --git a/lib/errname.c b/lib/errname.c >> index 0c4d3e66170e..7757bc00f564 100644 >> --- a/lib/errname.c >> +++ b/lib/errname.c >> @@ -11,9 +11,13 @@ >> * allocated errnos (with EHWPOISON = 257 on parisc, and EDQUOT = 1133 >> * on mips), so this wastes a bit of space on those - though we >> * special case the EDQUOT case. >> + * >> + * For the benefit of %pe being able to print any ERR_OR_NULL pointer >> + * symbolically, 0 is also treated specially. >> */ >> #define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = "-" #err >> static const char *names_0[] = { >> + [0] = "NULL", >> E(E2BIG), >> E(EACCES), >> E(EADDRINUSE), >> diff --git a/lib/test_printf.c b/lib/test_printf.c >> index 2d9f520d2f27..3a37d0e9e735 100644 >> --- a/lib/test_printf.c >> +++ b/lib/test_printf.c >> @@ -641,6 +641,7 @@ errptr(void) >> test("[-EIO ]", "[%-8pe]", ERR_PTR(-EIO)); >> test("[ -EIO]", "[%8pe]", ERR_PTR(-EIO)); >> test("-EPROBE_DEFER", "%pe", ERR_PTR(-EPROBE_DEFER)); >> + test("[NULL]", "[%pe]", NULL); >> #endif >> } >> >> diff --git a/lib/vsprintf.c b/lib/vsprintf.c >> index 7c488a1ce318..b7118d78eb20 100644 >> --- a/lib/vsprintf.c >> +++ b/lib/vsprintf.c >> @@ -2247,8 +2247,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, >> case 'x': >> return pointer_string(buf, end, ptr, spec); >> case 'e': >> - /* %pe with a non-ERR_PTR gets treated as plain %p */ >> - if (!IS_ERR(ptr)) >> + /* %pe with a non-ERR_OR_NULL ptr gets treated as plain %p */ >> + if (!IS_ERR_OR_NULL(ptr)) >> break; > > FWIW I was about to post a patch that just special cases NULL here. > > I think changing errname() to return "NULL" for 0 is overkill. > People will sooner or later discover that function and start using it > in contexts that don't have anything to do with pointers. Returning > _some_ string for 0 (instead of NULL) makes it very close to standard > strerror(), and "NULL" for 0 (i.e. success) seems rather odd. I see what you mean, but I don't share your assumption that errname() will ever grow callers other than the one in vsprintf.c. But I don't have any strong opinion either way. Perhaps this on top of my patch --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -619,7 +619,7 @@ static char *err_ptr(char *buf, char *end, void *ptr, struct printf_spec spec) { int err = PTR_ERR(ptr); - const char *sym = errname(err); + const char *sym = err ? errname(err) : "NULL"; if (sym) return string_nocheck(buf, end, sym, spec); instead of the change(s) in errname.c? And then the test case for '"%pe", NULL' should also be moved outside CONFIG_SYMBOLIC_ERRNAME. BTW., your original patch for %p lacks corresponding update of test_vsprintf.c. Please add appropriate test cases. Rasmus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] vsprintf: sanely handle NULL passed to %pe 2020-02-19 11:53 ` Rasmus Villemoes @ 2020-02-19 13:48 ` Petr Mladek 2020-02-19 13:56 ` Rasmus Villemoes 0 siblings, 1 reply; 18+ messages in thread From: Petr Mladek @ 2020-02-19 13:48 UTC (permalink / raw) To: Rasmus Villemoes Cc: Ilya Dryomov, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Jonathan Corbet, Kees Cook, Tobin C . Harding, Linus Torvalds, linux-doc, LKML On Wed 2020-02-19 12:53:22, Rasmus Villemoes wrote: > On 19/02/2020 12.20, Ilya Dryomov wrote: > > On Wed, Feb 19, 2020 at 9:21 AM Rasmus Villemoes > > <linux@rasmusvillemoes.dk> wrote: > >> > >> Extend %pe to pretty-print NULL in addition to ERR_PTRs, > >> i.e. everything IS_ERR_OR_NULL(). > >> > >> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> > >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > >> --- > >> Something like this? The actual code change is +2,-1 with another +1 > >> for a test case. > >> > >> Documentation/core-api/printk-formats.rst | 9 +++++---- > >> lib/errname.c | 4 ++++ > >> lib/test_printf.c | 1 + > >> lib/vsprintf.c | 4 ++-- > >> 4 files changed, 12 insertions(+), 6 deletions(-) > >> > >> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst > >> index 8ebe46b1af39..964b55291445 100644 > >> --- a/Documentation/core-api/printk-formats.rst > >> +++ b/Documentation/core-api/printk-formats.rst > >> @@ -86,10 +86,11 @@ Error Pointers > >> > >> %pe -ENOSPC > >> > >> -For printing error pointers (i.e. a pointer for which IS_ERR() is true) > >> -as a symbolic error name. Error values for which no symbolic name is > >> -known are printed in decimal, while a non-ERR_PTR passed as the > >> -argument to %pe gets treated as ordinary %p. > >> +For printing error pointers (i.e. a pointer for which IS_ERR() is > >> +true) as a symbolic error name. Error values for which no symbolic > >> +name is known are printed in decimal. A NULL pointer is printed as > >> +NULL. All other pointer values (i.e. anything !IS_ERR_OR_NULL()) get > >> +treated as ordinary %p. > >> > >> Symbols/Function Pointers > >> ------------------------- > >> diff --git a/lib/errname.c b/lib/errname.c > >> index 0c4d3e66170e..7757bc00f564 100644 > >> --- a/lib/errname.c > >> +++ b/lib/errname.c > >> @@ -11,9 +11,13 @@ > >> * allocated errnos (with EHWPOISON = 257 on parisc, and EDQUOT = 1133 > >> * on mips), so this wastes a bit of space on those - though we > >> * special case the EDQUOT case. > >> + * > >> + * For the benefit of %pe being able to print any ERR_OR_NULL pointer > >> + * symbolically, 0 is also treated specially. > >> */ > >> #define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = "-" #err > >> static const char *names_0[] = { > >> + [0] = "NULL", > >> E(E2BIG), > >> E(EACCES), > >> E(EADDRINUSE), > >> diff --git a/lib/test_printf.c b/lib/test_printf.c > >> index 2d9f520d2f27..3a37d0e9e735 100644 > >> --- a/lib/test_printf.c > >> +++ b/lib/test_printf.c > >> @@ -641,6 +641,7 @@ errptr(void) > >> test("[-EIO ]", "[%-8pe]", ERR_PTR(-EIO)); > >> test("[ -EIO]", "[%8pe]", ERR_PTR(-EIO)); > >> test("-EPROBE_DEFER", "%pe", ERR_PTR(-EPROBE_DEFER)); > >> + test("[NULL]", "[%pe]", NULL); > >> #endif > >> } > >> > >> diff --git a/lib/vsprintf.c b/lib/vsprintf.c > >> index 7c488a1ce318..b7118d78eb20 100644 > >> --- a/lib/vsprintf.c > >> +++ b/lib/vsprintf.c > >> @@ -2247,8 +2247,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, > >> case 'x': > >> return pointer_string(buf, end, ptr, spec); > >> case 'e': > >> - /* %pe with a non-ERR_PTR gets treated as plain %p */ > >> - if (!IS_ERR(ptr)) > >> + /* %pe with a non-ERR_OR_NULL ptr gets treated as plain %p */ > >> + if (!IS_ERR_OR_NULL(ptr)) > >> break; > > > > FWIW I was about to post a patch that just special cases NULL here. > > > > I think changing errname() to return "NULL" for 0 is overkill. > > People will sooner or later discover that function and start using it > > in contexts that don't have anything to do with pointers. Returning > > _some_ string for 0 (instead of NULL) makes it very close to standard > > strerror(), and "NULL" for 0 (i.e. success) seems rather odd. > > I see what you mean, but I don't share your assumption that errname() > will ever grow callers other than the one in vsprintf.c. But I don't > have any strong opinion either way. Perhaps this on top of my patch > > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -619,7 +619,7 @@ static char *err_ptr(char *buf, char *end, void *ptr, > struct printf_spec spec) > { > int err = PTR_ERR(ptr); > - const char *sym = errname(err); > + const char *sym = err ? errname(err) : "NULL"; I like this more than adding "NULL" errname. > if (sym) > return string_nocheck(buf, end, sym, spec); > > instead of the change(s) in errname.c? And then the test case for > '"%pe", NULL' should also be moved outside CONFIG_SYMBOLIC_ERRNAME. The test should go into null_pointer() instead of errptr(). Could you send updated patch, please? ;-) > BTW., your original patch for %p lacks corresponding update of > test_vsprintf.c. Please add appropriate test cases. Good point. The existing test_hashed() is rather weak and it did not catch this change. It would be nice to make test_hash() more powerful. Anyway, the minimal udpate would be: diff --git a/lib/test_printf.c b/lib/test_printf.c index 2d9f520d2f27..1726a678bccd 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -333,7 +333,7 @@ test_hashed(const char *fmt, const void *p) static void __init null_pointer(void) { - test_hashed("%p", NULL); + test(ZEROS "00000000", "%p", NULL); test(ZEROS "00000000", "%px", NULL); test("(null)", "%pE", NULL); } Best Regards, Petr ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] vsprintf: sanely handle NULL passed to %pe 2020-02-19 13:48 ` Petr Mladek @ 2020-02-19 13:56 ` Rasmus Villemoes 2020-02-19 14:45 ` Petr Mladek 0 siblings, 1 reply; 18+ messages in thread From: Rasmus Villemoes @ 2020-02-19 13:56 UTC (permalink / raw) To: Petr Mladek Cc: Ilya Dryomov, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Jonathan Corbet, Kees Cook, Tobin C . Harding, Linus Torvalds, linux-doc, LKML On 19/02/2020 14.48, Petr Mladek wrote: > On Wed 2020-02-19 12:53:22, Rasmus Villemoes wrote: >> --- a/lib/vsprintf.c >> +++ b/lib/vsprintf.c >> @@ -619,7 +619,7 @@ static char *err_ptr(char *buf, char *end, void *ptr, >> struct printf_spec spec) >> { >> int err = PTR_ERR(ptr); >> - const char *sym = errname(err); >> + const char *sym = err ? errname(err) : "NULL"; > > I like this more than adding "NULL" errname. OK. >> if (sym) >> return string_nocheck(buf, end, sym, spec); >> >> instead of the change(s) in errname.c? And then the test case for >> '"%pe", NULL' should also be moved outside CONFIG_SYMBOLIC_ERRNAME. > > The test should go into null_pointer() instead of errptr(). Eh, no, the behaviour of %pe is tested by errptr(). I'll keep it that way. But I should add a #else section that tests how %pe behaves without CONFIG_SYMBOLIC_ERRNAME - though that's orthogonal to this patch. > Could you send updated patch, please? ;-) I'll wait a day or two for more comments. It doesn't seem very urgent. >> BTW., your original patch for %p lacks corresponding update of >> test_vsprintf.c. Please add appropriate test cases. > > Good point. The existing test_hashed() is rather weak > and it did not catch this change. > > It would be nice to make test_hash() more powerful. > Anyway, the minimal udpate would be: > > diff --git a/lib/test_printf.c b/lib/test_printf.c > index 2d9f520d2f27..1726a678bccd 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -333,7 +333,7 @@ test_hashed(const char *fmt, const void *p) > static void __init > null_pointer(void) > { > - test_hashed("%p", NULL); > + test(ZEROS "00000000", "%p", NULL); No, it most certainly also needs to check a few "%p", ERR_PTR(-4) cases (where one of course has to use explicit integers and not E* constants). Rasmus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] vsprintf: sanely handle NULL passed to %pe 2020-02-19 13:56 ` Rasmus Villemoes @ 2020-02-19 14:45 ` Petr Mladek 2020-02-19 15:38 ` Andy Shevchenko 2020-02-19 15:40 ` Rasmus Villemoes 0 siblings, 2 replies; 18+ messages in thread From: Petr Mladek @ 2020-02-19 14:45 UTC (permalink / raw) To: Rasmus Villemoes Cc: Ilya Dryomov, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Jonathan Corbet, Kees Cook, Tobin C . Harding, Linus Torvalds, linux-doc, LKML On Wed 2020-02-19 14:56:32, Rasmus Villemoes wrote: > On 19/02/2020 14.48, Petr Mladek wrote: > > On Wed 2020-02-19 12:53:22, Rasmus Villemoes wrote: > >> --- a/lib/vsprintf.c > >> +++ b/lib/vsprintf.c > > The test should go into null_pointer() instead of errptr(). > > Eh, no, the behaviour of %pe is tested by errptr(). I'll keep it that > way. But I should add a #else section that tests how %pe behaves without > CONFIG_SYMBOLIC_ERRNAME - though that's orthogonal to this patch. OK, we should agree on some structure first. We already have two top level functions that test how a particular pointer is printed using different pointer modifiers: null_pointer(); -> NULL with %p, %pX, %pE invalid_pointer(); -> random pointer with %p, %pX, %pE Following this logic, errptr() should test how a pointer from IS_ERR() range is printed using different pointer formats. I am open to crate another logic but it must be consistent. If you want to check %pe with NULL in errptr(), you have to split the other two functions per-modifier. IMHO, it is not worth it. Sigh, I should have been more strict[*]. The function should have been called err_ptr() and located right below null_pointer(). [*] I am still trying to find a right balance between preventing nitpicking, bikeshedding, enforcing my style, and creating a mess. > > Could you send updated patch, please? ;-) > > I'll wait a day or two for more comments. It doesn't seem very urgent. Sure. > >> BTW., your original patch for %p lacks corresponding update of > >> test_vsprintf.c. Please add appropriate test cases. > > > > diff --git a/lib/test_printf.c b/lib/test_printf.c > > index 2d9f520d2f27..1726a678bccd 100644 > > --- a/lib/test_printf.c > > +++ b/lib/test_printf.c > > @@ -333,7 +333,7 @@ test_hashed(const char *fmt, const void *p) > > static void __init > > null_pointer(void) > > { > > - test_hashed("%p", NULL); > > + test(ZEROS "00000000", "%p", NULL); > > No, it most certainly also needs to check a few "%p", ERR_PTR(-4) cases > (where one of course has to use explicit integers and not E* constants). Yes, it would be great to add checks for %p, %px for IS_ERR() range. But it is different story. The above change is for the original patch and it was about NULL pointer handling. Best Regards, Petr ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] vsprintf: sanely handle NULL passed to %pe 2020-02-19 14:45 ` Petr Mladek @ 2020-02-19 15:38 ` Andy Shevchenko 2020-02-19 15:40 ` Rasmus Villemoes 1 sibling, 0 replies; 18+ messages in thread From: Andy Shevchenko @ 2020-02-19 15:38 UTC (permalink / raw) To: Petr Mladek Cc: Rasmus Villemoes, Ilya Dryomov, Steven Rostedt, Sergey Senozhatsky, Jonathan Corbet, Kees Cook, Tobin C . Harding, Linus Torvalds, linux-doc, LKML On Wed, Feb 19, 2020 at 03:45:58PM +0100, Petr Mladek wrote: > On Wed 2020-02-19 14:56:32, Rasmus Villemoes wrote: ... > Sigh, I should have been more strict[*]. The function should have been > called err_ptr() and located right below null_pointer(). But taking above into consideration it should be rather error_pointer(). No? > [*] I am still trying to find a right balance between preventing > nitpicking, bikeshedding, enforcing my style, and creating a mess. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] vsprintf: sanely handle NULL passed to %pe 2020-02-19 14:45 ` Petr Mladek 2020-02-19 15:38 ` Andy Shevchenko @ 2020-02-19 15:40 ` Rasmus Villemoes 2020-02-19 17:23 ` Ilya Dryomov 2020-02-20 12:57 ` Petr Mladek 1 sibling, 2 replies; 18+ messages in thread From: Rasmus Villemoes @ 2020-02-19 15:40 UTC (permalink / raw) To: Petr Mladek Cc: Ilya Dryomov, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Jonathan Corbet, Kees Cook, Tobin C . Harding, Linus Torvalds, linux-doc, LKML On 19/02/2020 15.45, Petr Mladek wrote: > On Wed 2020-02-19 14:56:32, Rasmus Villemoes wrote: >> On 19/02/2020 14.48, Petr Mladek wrote: >>> On Wed 2020-02-19 12:53:22, Rasmus Villemoes wrote: >>>> --- a/lib/vsprintf.c >>>> +++ b/lib/vsprintf.c >>> The test should go into null_pointer() instead of errptr(). >> >> Eh, no, the behaviour of %pe is tested by errptr(). I'll keep it that >> way. But I should add a #else section that tests how %pe behaves without >> CONFIG_SYMBOLIC_ERRNAME - though that's orthogonal to this patch. > > OK, we should agree on some structure first. > > We already have two top level functions that test how a particular > pointer is printed using different pointer modifiers: > > null_pointer(); -> NULL with %p, %pX, %pE > invalid_pointer(); -> random pointer with %p, %pX, %pE > > Following this logic, errptr() should test how a pointer from IS_ERR() range > is printed using different pointer formats. Oh please. I wrote test_printf.c originally and structured it with one helper for each %p<whatever>. How are your additions null_pointer and invalid_pointer good examples for what the existing style is? 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 649) test_pointer(void) 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 650) { 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 651) plain(); 3e5903eb9cff7 (Petr Mladek 2019-04-17 13:53:48 +0200 652) null_pointer(); 3e5903eb9cff7 (Petr Mladek 2019-04-17 13:53:48 +0200 653) invalid_pointer(); 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 654) symbol_ptr(); 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 655) kernel_ptr(); 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 656) struct_resource(); 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 657) addr(); 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 658) escaped_str(); 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 659) hex_string(); 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 660) mac(); 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 661) ip(); 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 662) uuid(); 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 663) dentry(); 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 664) struct_va_format(); 4d42c44727a06 (Andy Shevchenko 2018-12-04 23:23:11 +0200 665) struct_rtc_time(); 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 666) struct_clk(); 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 667) bitmap(); 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 668) netdev_features(); edf14cdbf9a0e (Vlastimil Babka 2016-03-15 14:55:56 -0700 669) flags(); 57f5677e535ba (Rasmus Villemoes 2019-10-15 21:07:05 +0200 670) errptr(); f1ce39df508de (Sakari Ailus 2019-10-03 15:32:19 +0300 671) fwnode_pointer(); > I am open to crate another logic but it must be consistent. So yeah, I'm going to continue testing the behaviour of %pe in errptr, TYVM. > If you want to check %pe with NULL in errptr(), you have to > split the other two functions per-modifier. IMHO, it is not > worth it. Agreed, let's leave null_pointer and invalid_pointer alone. >>>> BTW., your original patch for %p lacks corresponding update of >>>> test_vsprintf.c. Please add appropriate test cases. >>> >>> diff --git a/lib/test_printf.c b/lib/test_printf.c >>> index 2d9f520d2f27..1726a678bccd 100644 >>> --- a/lib/test_printf.c >>> +++ b/lib/test_printf.c >>> @@ -333,7 +333,7 @@ test_hashed(const char *fmt, const void *p) >>> static void __init >>> null_pointer(void) >>> { >>> - test_hashed("%p", NULL); >>> + test(ZEROS "00000000", "%p", NULL); >> >> No, it most certainly also needs to check a few "%p", ERR_PTR(-4) cases >> (where one of course has to use explicit integers and not E* constants). > > Yes, it would be great to add checks for %p, %px for IS_ERR() range. > But it is different story. The above change is for the original patch > and it was about NULL pointer handling. Wrong. The original patch (i.e. Ilya's) had subject "vsprintf: don't obfuscate NULL and error pointers" and did + if (IS_ERR_OR_NULL(ptr)) so the tests that should be part of that patch very much need to cover both NULL and ERR_PTRs passed to plain %p. Rasmus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] vsprintf: sanely handle NULL passed to %pe 2020-02-19 15:40 ` Rasmus Villemoes @ 2020-02-19 17:23 ` Ilya Dryomov 2020-02-20 12:57 ` Petr Mladek 1 sibling, 0 replies; 18+ messages in thread From: Ilya Dryomov @ 2020-02-19 17:23 UTC (permalink / raw) To: Rasmus Villemoes Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Jonathan Corbet, Kees Cook, Tobin C . Harding, Linus Torvalds, Linux Documentation List, LKML On Wed, Feb 19, 2020 at 4:40 PM Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > > On 19/02/2020 15.45, Petr Mladek wrote: > > On Wed 2020-02-19 14:56:32, Rasmus Villemoes wrote: > >> On 19/02/2020 14.48, Petr Mladek wrote: > >>> On Wed 2020-02-19 12:53:22, Rasmus Villemoes wrote: > >>>> --- a/lib/vsprintf.c > >>>> +++ b/lib/vsprintf.c > >>> The test should go into null_pointer() instead of errptr(). > >> > >> Eh, no, the behaviour of %pe is tested by errptr(). I'll keep it that > >> way. But I should add a #else section that tests how %pe behaves without > >> CONFIG_SYMBOLIC_ERRNAME - though that's orthogonal to this patch. > > > > OK, we should agree on some structure first. > > > > We already have two top level functions that test how a particular > > pointer is printed using different pointer modifiers: > > > > null_pointer(); -> NULL with %p, %pX, %pE > > invalid_pointer(); -> random pointer with %p, %pX, %pE > > > > Following this logic, errptr() should test how a pointer from IS_ERR() range > > is printed using different pointer formats. > > Oh please. I wrote test_printf.c originally and structured it with one > helper for each %p<whatever>. How are your additions null_pointer and > invalid_pointer good examples for what the existing style is? > > 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 649) > test_pointer(void) > 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 650) { > 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 651) plain(); > 3e5903eb9cff7 (Petr Mladek 2019-04-17 13:53:48 +0200 652) > null_pointer(); > 3e5903eb9cff7 (Petr Mladek 2019-04-17 13:53:48 +0200 653) > invalid_pointer(); > 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 654) > symbol_ptr(); > 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 655) > kernel_ptr(); > 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 656) > struct_resource(); > 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 657) addr(); > 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 658) > escaped_str(); > 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 659) > hex_string(); > 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 660) mac(); > 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 661) ip(); > 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 662) uuid(); > 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 663) dentry(); > 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 664) > struct_va_format(); > 4d42c44727a06 (Andy Shevchenko 2018-12-04 23:23:11 +0200 665) > struct_rtc_time(); > 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 666) > struct_clk(); > 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 667) bitmap(); > 707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 668) > netdev_features(); > edf14cdbf9a0e (Vlastimil Babka 2016-03-15 14:55:56 -0700 669) flags(); > 57f5677e535ba (Rasmus Villemoes 2019-10-15 21:07:05 +0200 670) errptr(); > f1ce39df508de (Sakari Ailus 2019-10-03 15:32:19 +0300 671) > fwnode_pointer(); > > > > I am open to crate another logic but it must be consistent. > > So yeah, I'm going to continue testing the behaviour of %pe in errptr, TYVM. > > > If you want to check %pe with NULL in errptr(), you have to > > split the other two functions per-modifier. IMHO, it is not > > worth it. > > Agreed, let's leave null_pointer and invalid_pointer alone. > > >>>> BTW., your original patch for %p lacks corresponding update of > >>>> test_vsprintf.c. Please add appropriate test cases. > >>> > >>> diff --git a/lib/test_printf.c b/lib/test_printf.c > >>> index 2d9f520d2f27..1726a678bccd 100644 > >>> --- a/lib/test_printf.c > >>> +++ b/lib/test_printf.c > >>> @@ -333,7 +333,7 @@ test_hashed(const char *fmt, const void *p) > >>> static void __init > >>> null_pointer(void) > >>> { > >>> - test_hashed("%p", NULL); > >>> + test(ZEROS "00000000", "%p", NULL); > >> > >> No, it most certainly also needs to check a few "%p", ERR_PTR(-4) cases > >> (where one of course has to use explicit integers and not E* constants). > > > > Yes, it would be great to add checks for %p, %px for IS_ERR() range. > > But it is different story. The above change is for the original patch > > and it was about NULL pointer handling. > > Wrong. The original patch (i.e. Ilya's) had subject "vsprintf: don't > obfuscate NULL and error pointers" and did > > + if (IS_ERR_OR_NULL(ptr)) > > so the tests that should be part of that patch very much need to cover > both NULL and ERR_PTRs passed to plain %p. I sent v2 of my patch with the update to test_printf.c. I see your point about one function for each %p variant, but since it's already been disrupted with null_pointer() and invalid_pointer() and also because test_hashed() has a comment which implies that it must be called after plain(), I piled on by adding error_pointer(). Thanks, Ilya ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] vsprintf: sanely handle NULL passed to %pe 2020-02-19 15:40 ` Rasmus Villemoes 2020-02-19 17:23 ` Ilya Dryomov @ 2020-02-20 12:57 ` Petr Mladek 2020-02-20 15:02 ` Ilya Dryomov 1 sibling, 1 reply; 18+ messages in thread From: Petr Mladek @ 2020-02-20 12:57 UTC (permalink / raw) To: Rasmus Villemoes Cc: Ilya Dryomov, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Jonathan Corbet, Kees Cook, Tobin C . Harding, Linus Torvalds, linux-doc, LKML On Wed 2020-02-19 16:40:08, Rasmus Villemoes wrote: > On 19/02/2020 15.45, Petr Mladek wrote: > > On Wed 2020-02-19 14:56:32, Rasmus Villemoes wrote: > >> On 19/02/2020 14.48, Petr Mladek wrote: > >>> On Wed 2020-02-19 12:53:22, Rasmus Villemoes wrote: > >>>> --- a/lib/vsprintf.c > >>>> +++ b/lib/vsprintf.c > >>> The test should go into null_pointer() instead of errptr(). > >> > >> Eh, no, the behaviour of %pe is tested by errptr(). I'll keep it that > >> way. But I should add a #else section that tests how %pe behaves without > >> CONFIG_SYMBOLIC_ERRNAME - though that's orthogonal to this patch. > > > > OK, we should agree on some structure first. > > > > We already have two top level functions that test how a particular > > pointer is printed using different pointer modifiers: > > > > null_pointer(); -> NULL with %p, %pX, %pE > > invalid_pointer(); -> random pointer with %p, %pX, %pE > > > > Following this logic, errptr() should test how a pointer from IS_ERR() range > > is printed using different pointer formats. > > Oh please. I wrote test_printf.c originally and structured it with one > helper for each %p<whatever>. How are your additions null_pointer and > invalid_pointer good examples for what the existing style is? I see, I was the one who broke the style. Please, find below a patch that tries to fix it. If you agree with the approach then I could split it into smaller steps. Also it would make sense to add checks for NULL and ERR pointer into each existing %p modifier check. It will make sure that check_pointer() is called in all handlers. > So yeah, I'm going to continue testing the behaviour of %pe in errptr, TYVM. OK. > >>>> BTW., your original patch for %p lacks corresponding update of > >>>> test_vsprintf.c. Please add appropriate test cases. > >>> > >>> diff --git a/lib/test_printf.c b/lib/test_printf.c > >>> index 2d9f520d2f27..1726a678bccd 100644 > >>> --- a/lib/test_printf.c > >>> +++ b/lib/test_printf.c > >>> @@ -333,7 +333,7 @@ test_hashed(const char *fmt, const void *p) > >>> static void __init > >>> null_pointer(void) > >>> { > >>> - test_hashed("%p", NULL); > >>> + test(ZEROS "00000000", "%p", NULL); > >> > >> No, it most certainly also needs to check a few "%p", ERR_PTR(-4) cases > >> (where one of course has to use explicit integers and not E* constants). > > > > Yes, it would be great to add checks for %p, %px for IS_ERR() range. > > But it is different story. The above change is for the original patch > > and it was about NULL pointer handling. > > Wrong. The original patch (i.e. Ilya's) had subject "vsprintf: don't > obfuscate NULL and error pointers" and did > > + if (IS_ERR_OR_NULL(ptr)) > > so the tests that should be part of that patch very much need to cover > both NULL and ERR_PTRs passed to plain %p. Grr, I see. I was too fast yesterday. OK, I suggest to fix the structure of the tests first. All these patches are for 5.7 anyway. Here is the proposed clean up: From 855909f2a1945d3a5bf490ddf4f2cca775ef967b Mon Sep 17 00:00:00 2001 From: Petr Mladek <pmladek@suse.com> Date: Thu, 20 Feb 2020 12:53:43 +0100 Subject: [PATCH] lib/test_printf: Clean up basic pointer testing The pointer testing has been originally split by the %p modifiers, for example, the function dentry() tested %pd and %pD handling. There were recently added tests that do not really fit into the existing structure, namely: + hashed pointers tested by a maze of functions + null and invalid pointer handling with various modifiers The hash pointer test is really special because the hash depends on a random key that is generated during boot. Though, it is still possible to check some aspects: + output string length + hash differs from the original pointer value + top half bites are zeroed on 64-bit systems Let's put all these checks into test_hashed() function that has the same behavior as the test() functions for well-defined output. It increments the number of tests and eventual failures. It prints warnings/errors when some aspects of the output are not as expected. Most of these checks were there even before. The only addition is the check whether hash differs from the original pointer value. There is a small chance of a false error. It might be reduced by checking more pointers but let's keep it simple for now. The existing null_pointer() and invalid_pointer() checks are newly split per-format modifier. And there is also fixed difference between invalid pointer in the IS_ERR() range and invalid pointer that looks like a valid one. The invalid pointer Oxdeaddead00000000 should work on most architectures. But I am not able to check it everywhere. So there is a small chance of a false error. It might get fixed when anyone reports a problem. Signed-off-by: Petr Mladek <pmladek@suse.com> --- lib/test_printf.c | 162 ++++++++++++++++++++---------------------------------- 1 file changed, 59 insertions(+), 103 deletions(-) diff --git a/lib/test_printf.c b/lib/test_printf.c index 2d9f520d2f27..4e89b508def6 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -206,146 +206,101 @@ test_string(void) } #define PLAIN_BUF_SIZE 64 /* leave some space so we don't oops */ +#define PTR_ERROR ERR_PTR(-EFAULT) +#define PTR_VAL_ERROR "fffffff2" #if BITS_PER_LONG == 64 #define PTR_WIDTH 16 #define PTR ((void *)0xffff0123456789abUL) #define PTR_STR "ffff0123456789ab" +#define PTR_INVALID ((void *)0xdeaddead000000ab) +#define PTR_VAL_INVALID "deaddead000000ab" #define PTR_VAL_NO_CRNG "(____ptrval____)" +#define ONES "ffffffff" /* hex 32 one bits */ #define ZEROS "00000000" /* hex 32 zero bits */ -static int __init -plain_format(void) -{ - char buf[PLAIN_BUF_SIZE]; - int nchars; - - nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR); - - if (nchars != PTR_WIDTH) - return -1; - - if (strncmp(buf, PTR_VAL_NO_CRNG, PTR_WIDTH) == 0) { - pr_warn("crng possibly not yet initialized. plain 'p' buffer contains \"%s\"", - PTR_VAL_NO_CRNG); - return 0; - } - - if (strncmp(buf, ZEROS, strlen(ZEROS)) != 0) - return -1; - - return 0; -} - #else #define PTR_WIDTH 8 #define PTR ((void *)0x456789ab) #define PTR_STR "456789ab" +#define PTR_INVALID ((void *)0x000000ab) +#define PTR_VAL_INVALID "000000ab" #define PTR_VAL_NO_CRNG "(ptrval)" +#define ONES "" #define ZEROS "" -static int __init -plain_format(void) -{ - /* Format is implicitly tested for 32 bit machines by plain_hash() */ - return 0; -} - #endif /* BITS_PER_LONG == 64 */ -static int __init -plain_hash_to_buffer(const void *p, char *buf, size_t len) +static void __init +test_hashed(const char *fmt, const void *p) { + char pointer[PLAIN_BUF_SIZE]; + char hash[PLAIN_BUF_SIZE]; int nchars; - nchars = snprintf(buf, len, "%p", p); - - if (nchars != PTR_WIDTH) - return -1; + total_tests++; - if (strncmp(buf, PTR_VAL_NO_CRNG, PTR_WIDTH) == 0) { - pr_warn("crng possibly not yet initialized. plain 'p' buffer contains \"%s\"", - PTR_VAL_NO_CRNG); - return 0; + nchars = snprintf(pointer, sizeof(pointer), "%px", p); + if (nchars != PTR_WIDTH) { + pr_err("error in test suite: vsprintf(\"%%px\", p) returned number of characters %d, expected %d\n", + nchars, PTR_WIDTH); + goto err; } - return 0; -} - -static int __init -plain_hash(void) -{ - char buf[PLAIN_BUF_SIZE]; - int ret; - - ret = plain_hash_to_buffer(PTR, buf, PLAIN_BUF_SIZE); - if (ret) - return ret; - - if (strncmp(buf, PTR_STR, PTR_WIDTH) == 0) - return -1; - - return 0; -} - -/* - * We can't use test() to test %p because we don't know what output to expect - * after an address is hashed. - */ -static void __init -plain(void) -{ - int err; - - err = plain_hash(); - if (err) { - pr_warn("plain 'p' does not appear to be hashed\n"); - failed_tests++; - return; + nchars = snprintf(hash, sizeof(hash), fmt, p); + if (nchars != PTR_WIDTH) { + pr_warn("vsprintf(\"%s\", p) returned number of characters %d, expected %d\n", + fmt, nchars, PTR_WIDTH); + goto err; } - err = plain_format(); - if (err) { - pr_warn("hashing plain 'p' has unexpected format\n"); - failed_tests++; + if (strncmp(hash, PTR_VAL_NO_CRNG, PTR_WIDTH) == 0) { + pr_warn_once("crng possibly not yet initialized. vsprinf(\"%s\", p) printed \"%s\"", + fmt, hash); + total_tests--; + return; } -} - -static void __init -test_hashed(const char *fmt, const void *p) -{ - char buf[PLAIN_BUF_SIZE]; - int ret; /* - * No need to increase failed test counter since this is assumed - * to be called after plain(). + * There is a small chance of a false negative on 32-bit systems + * when the hash is the same as the pointer value. */ - ret = plain_hash_to_buffer(p, buf, PLAIN_BUF_SIZE); - if (ret) - return; + if (strncmp(hash, pointer, PTR_WIDTH) == 0) { + pr_warn("vsprintf(\"%s\", p) returned %s, expected hashed pointer\n", + fmt, hash); + goto err; + } + +#if BITS_PER_LONG == 64 + if (strncmp(hash, ZEROS, PTR_WIDTH / 2) != 0) { + pr_warn("vsprintf(\"%s\", p) returned %s, expected %s in the top half bits\n", + fmt, hash, ZEROS); + goto err; + } +#endif + return; - test(buf, fmt, p); +err: + failed_tests++; } static void __init -null_pointer(void) +plain_pointer(void) { test_hashed("%p", NULL); - test(ZEROS "00000000", "%px", NULL); - test("(null)", "%pE", NULL); + test_hashed("%p", PTR_ERROR); + test_hashed("%p", PTR_INVALID); } -#define PTR_INVALID ((void *)0x000000ab) static void __init -invalid_pointer(void) +real_pointer(void) { - test_hashed("%p", PTR_INVALID); - test(ZEROS "000000ab", "%px", PTR_INVALID); - test("(efault)", "%pE", PTR_INVALID); + test(ZEROS "00000000", "%px", NULL); + test(ONES PTR_VAL_ERROR, "%px", PTR_ERROR); + test(PTR_VAL_INVALID, "%px", PTR_INVALID); } static void __init @@ -372,6 +327,8 @@ addr(void) static void __init escaped_str(void) { + test("(null)", "%pE", NULL); + test("(efault)", "%pE", PTR_ERROR); } static void __init @@ -458,9 +415,9 @@ dentry(void) test("foo", "%pd2", &test_dentry[0]); test("(null)", "%pd", NULL); - test("(efault)", "%pd", PTR_INVALID); + test("(efault)", "%pd", PTR_ERROR); test("(null)", "%pD", NULL); - test("(efault)", "%pD", PTR_INVALID); + test("(efault)", "%pD", PTR_ERROR); test("romeo", "%pd", &test_dentry[3]); test("alfa/romeo", "%pd2", &test_dentry[3]); @@ -647,9 +604,8 @@ errptr(void) static void __init test_pointer(void) { - plain(); - null_pointer(); - invalid_pointer(); + plain_pointer(); + real_pointer(); symbol_ptr(); kernel_ptr(); struct_resource(); -- 2.16.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] vsprintf: sanely handle NULL passed to %pe 2020-02-20 12:57 ` Petr Mladek @ 2020-02-20 15:02 ` Ilya Dryomov 2020-02-21 13:05 ` Petr Mladek 0 siblings, 1 reply; 18+ messages in thread From: Ilya Dryomov @ 2020-02-20 15:02 UTC (permalink / raw) To: Petr Mladek Cc: Rasmus Villemoes, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Jonathan Corbet, Kees Cook, Tobin C . Harding, Linus Torvalds, Linux Documentation List, LKML On Thu, Feb 20, 2020 at 1:57 PM Petr Mladek <pmladek@suse.com> wrote: > > On Wed 2020-02-19 16:40:08, Rasmus Villemoes wrote: > > On 19/02/2020 15.45, Petr Mladek wrote: > > > On Wed 2020-02-19 14:56:32, Rasmus Villemoes wrote: > > >> On 19/02/2020 14.48, Petr Mladek wrote: > > >>> On Wed 2020-02-19 12:53:22, Rasmus Villemoes wrote: > > >>>> --- a/lib/vsprintf.c > > >>>> +++ b/lib/vsprintf.c > > >>> The test should go into null_pointer() instead of errptr(). > > >> > > >> Eh, no, the behaviour of %pe is tested by errptr(). I'll keep it that > > >> way. But I should add a #else section that tests how %pe behaves without > > >> CONFIG_SYMBOLIC_ERRNAME - though that's orthogonal to this patch. > > > > > > OK, we should agree on some structure first. > > > > > > We already have two top level functions that test how a particular > > > pointer is printed using different pointer modifiers: > > > > > > null_pointer(); -> NULL with %p, %pX, %pE > > > invalid_pointer(); -> random pointer with %p, %pX, %pE > > > > > > Following this logic, errptr() should test how a pointer from IS_ERR() range > > > is printed using different pointer formats. > > > > Oh please. I wrote test_printf.c originally and structured it with one > > helper for each %p<whatever>. How are your additions null_pointer and > > invalid_pointer good examples for what the existing style is? > > I see, I was the one who broke the style. Please, find below a patch > that tries to fix it. If you agree with the approach then I could > split it into smaller steps. > > Also it would make sense to add checks for NULL and ERR pointer > into each existing %p modifier check. It will make sure that > check_pointer() is called in all handlers. > > > > So yeah, I'm going to continue testing the behaviour of %pe in errptr, TYVM. > > OK. > > > >>>> BTW., your original patch for %p lacks corresponding update of > > >>>> test_vsprintf.c. Please add appropriate test cases. > > >>> > > >>> diff --git a/lib/test_printf.c b/lib/test_printf.c > > >>> index 2d9f520d2f27..1726a678bccd 100644 > > >>> --- a/lib/test_printf.c > > >>> +++ b/lib/test_printf.c > > >>> @@ -333,7 +333,7 @@ test_hashed(const char *fmt, const void *p) > > >>> static void __init > > >>> null_pointer(void) > > >>> { > > >>> - test_hashed("%p", NULL); > > >>> + test(ZEROS "00000000", "%p", NULL); > > >> > > >> No, it most certainly also needs to check a few "%p", ERR_PTR(-4) cases > > >> (where one of course has to use explicit integers and not E* constants). > > > > > > Yes, it would be great to add checks for %p, %px for IS_ERR() range. > > > But it is different story. The above change is for the original patch > > > and it was about NULL pointer handling. > > > > Wrong. The original patch (i.e. Ilya's) had subject "vsprintf: don't > > obfuscate NULL and error pointers" and did > > > > + if (IS_ERR_OR_NULL(ptr)) > > > > so the tests that should be part of that patch very much need to cover > > both NULL and ERR_PTRs passed to plain %p. > > Grr, I see. I was too fast yesterday. OK, I suggest to fix the > structure of the tests first. All these patches are for 5.7 > anyway. My patch fixes a regression introduced by 3e5903eb9cff ("vsprintf: Prevent crash when dereferencing invalid pointers" in 5.2, which made debugging based on existing pr_debugs (used extensively in some subsystems) very annoying. I would like to see it in 5.6, so that it is backported to 5.4 and 5.5. > > > Here is the proposed clean up: > > From 855909f2a1945d3a5bf490ddf4f2cca775ef967b Mon Sep 17 00:00:00 2001 > From: Petr Mladek <pmladek@suse.com> > Date: Thu, 20 Feb 2020 12:53:43 +0100 > Subject: [PATCH] lib/test_printf: Clean up basic pointer testing > > The pointer testing has been originally split by the %p modifiers, > for example, the function dentry() tested %pd and %pD handling. > > There were recently added tests that do not really fit into > the existing structure, namely: > > + hashed pointers tested by a maze of functions > + null and invalid pointer handling with various modifiers > > The hash pointer test is really special because the hash depends > on a random key that is generated during boot. Though, it is > still possible to check some aspects: > > + output string length > + hash differs from the original pointer value > + top half bites are zeroed on 64-bit systems > > Let's put all these checks into test_hashed() function that has > the same behavior as the test() functions for well-defined output. > It increments the number of tests and eventual failures. It prints > warnings/errors when some aspects of the output are not as expected. > > Most of these checks were there even before. The only addition is > the check whether hash differs from the original pointer value. > There is a small chance of a false error. It might be reduced > by checking more pointers but let's keep it simple for now. > > The existing null_pointer() and invalid_pointer() checks are > newly split per-format modifier. And there is also fixed > difference between invalid pointer in the IS_ERR() range > and invalid pointer that looks like a valid one. > > The invalid pointer Oxdeaddead00000000 should work on most > architectures. But I am not able to check it everywhere. > So there is a small chance of a false error. It might get > fixed when anyone reports a problem. > > Signed-off-by: Petr Mladek <pmladek@suse.com> > --- > lib/test_printf.c | 162 ++++++++++++++++++++---------------------------------- > 1 file changed, 59 insertions(+), 103 deletions(-) > > diff --git a/lib/test_printf.c b/lib/test_printf.c > index 2d9f520d2f27..4e89b508def6 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -206,146 +206,101 @@ test_string(void) > } > > #define PLAIN_BUF_SIZE 64 /* leave some space so we don't oops */ > +#define PTR_ERROR ERR_PTR(-EFAULT) > +#define PTR_VAL_ERROR "fffffff2" > > #if BITS_PER_LONG == 64 > > #define PTR_WIDTH 16 > #define PTR ((void *)0xffff0123456789abUL) > #define PTR_STR "ffff0123456789ab" > +#define PTR_INVALID ((void *)0xdeaddead000000ab) > +#define PTR_VAL_INVALID "deaddead000000ab" > #define PTR_VAL_NO_CRNG "(____ptrval____)" > +#define ONES "ffffffff" /* hex 32 one bits */ > #define ZEROS "00000000" /* hex 32 zero bits */ > > -static int __init > -plain_format(void) > -{ > - char buf[PLAIN_BUF_SIZE]; > - int nchars; > - > - nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR); > - > - if (nchars != PTR_WIDTH) > - return -1; > - > - if (strncmp(buf, PTR_VAL_NO_CRNG, PTR_WIDTH) == 0) { > - pr_warn("crng possibly not yet initialized. plain 'p' buffer contains \"%s\"", > - PTR_VAL_NO_CRNG); > - return 0; > - } > - > - if (strncmp(buf, ZEROS, strlen(ZEROS)) != 0) > - return -1; > - > - return 0; > -} > - > #else > > #define PTR_WIDTH 8 > #define PTR ((void *)0x456789ab) > #define PTR_STR "456789ab" > +#define PTR_INVALID ((void *)0x000000ab) > +#define PTR_VAL_INVALID "000000ab" > #define PTR_VAL_NO_CRNG "(ptrval)" > +#define ONES "" > #define ZEROS "" > > -static int __init > -plain_format(void) > -{ > - /* Format is implicitly tested for 32 bit machines by plain_hash() */ > - return 0; > -} > - > #endif /* BITS_PER_LONG == 64 */ > > -static int __init > -plain_hash_to_buffer(const void *p, char *buf, size_t len) > +static void __init > +test_hashed(const char *fmt, const void *p) > { > + char pointer[PLAIN_BUF_SIZE]; > + char hash[PLAIN_BUF_SIZE]; > int nchars; > > - nchars = snprintf(buf, len, "%p", p); > - > - if (nchars != PTR_WIDTH) > - return -1; > + total_tests++; > > - if (strncmp(buf, PTR_VAL_NO_CRNG, PTR_WIDTH) == 0) { > - pr_warn("crng possibly not yet initialized. plain 'p' buffer contains \"%s\"", > - PTR_VAL_NO_CRNG); > - return 0; > + nchars = snprintf(pointer, sizeof(pointer), "%px", p); > + if (nchars != PTR_WIDTH) { > + pr_err("error in test suite: vsprintf(\"%%px\", p) returned number of characters %d, expected %d\n", > + nchars, PTR_WIDTH); > + goto err; > } > > - return 0; > -} > - > -static int __init > -plain_hash(void) > -{ > - char buf[PLAIN_BUF_SIZE]; > - int ret; > - > - ret = plain_hash_to_buffer(PTR, buf, PLAIN_BUF_SIZE); > - if (ret) > - return ret; > - > - if (strncmp(buf, PTR_STR, PTR_WIDTH) == 0) > - return -1; > - > - return 0; > -} > - > -/* > - * We can't use test() to test %p because we don't know what output to expect > - * after an address is hashed. > - */ > -static void __init > -plain(void) > -{ > - int err; > - > - err = plain_hash(); > - if (err) { > - pr_warn("plain 'p' does not appear to be hashed\n"); > - failed_tests++; > - return; > + nchars = snprintf(hash, sizeof(hash), fmt, p); > + if (nchars != PTR_WIDTH) { > + pr_warn("vsprintf(\"%s\", p) returned number of characters %d, expected %d\n", > + fmt, nchars, PTR_WIDTH); > + goto err; > } > > - err = plain_format(); > - if (err) { > - pr_warn("hashing plain 'p' has unexpected format\n"); > - failed_tests++; > + if (strncmp(hash, PTR_VAL_NO_CRNG, PTR_WIDTH) == 0) { > + pr_warn_once("crng possibly not yet initialized. vsprinf(\"%s\", p) printed \"%s\"", > + fmt, hash); > + total_tests--; > + return; > } > -} > - > -static void __init > -test_hashed(const char *fmt, const void *p) > -{ > - char buf[PLAIN_BUF_SIZE]; > - int ret; > > /* > - * No need to increase failed test counter since this is assumed > - * to be called after plain(). > + * There is a small chance of a false negative on 32-bit systems > + * when the hash is the same as the pointer value. > */ > - ret = plain_hash_to_buffer(p, buf, PLAIN_BUF_SIZE); > - if (ret) > - return; > + if (strncmp(hash, pointer, PTR_WIDTH) == 0) { > + pr_warn("vsprintf(\"%s\", p) returned %s, expected hashed pointer\n", > + fmt, hash); > + goto err; > + } > + > +#if BITS_PER_LONG == 64 > + if (strncmp(hash, ZEROS, PTR_WIDTH / 2) != 0) { > + pr_warn("vsprintf(\"%s\", p) returned %s, expected %s in the top half bits\n", > + fmt, hash, ZEROS); > + goto err; > + } > +#endif > + return; > > - test(buf, fmt, p); > +err: > + failed_tests++; > } > > static void __init > -null_pointer(void) > +plain_pointer(void) > { > test_hashed("%p", NULL); > - test(ZEROS "00000000", "%px", NULL); > - test("(null)", "%pE", NULL); > + test_hashed("%p", PTR_ERROR); > + test_hashed("%p", PTR_INVALID); > } > > -#define PTR_INVALID ((void *)0x000000ab) > > static void __init > -invalid_pointer(void) > +real_pointer(void) > { > - test_hashed("%p", PTR_INVALID); > - test(ZEROS "000000ab", "%px", PTR_INVALID); > - test("(efault)", "%pE", PTR_INVALID); > + test(ZEROS "00000000", "%px", NULL); > + test(ONES PTR_VAL_ERROR, "%px", PTR_ERROR); > + test(PTR_VAL_INVALID, "%px", PTR_INVALID); > } > > static void __init > @@ -372,6 +327,8 @@ addr(void) > static void __init > escaped_str(void) > { > + test("(null)", "%pE", NULL); > + test("(efault)", "%pE", PTR_ERROR); > } > > static void __init > @@ -458,9 +415,9 @@ dentry(void) > test("foo", "%pd2", &test_dentry[0]); > > test("(null)", "%pd", NULL); > - test("(efault)", "%pd", PTR_INVALID); > + test("(efault)", "%pd", PTR_ERROR); > test("(null)", "%pD", NULL); > - test("(efault)", "%pD", PTR_INVALID); > + test("(efault)", "%pD", PTR_ERROR); > > test("romeo", "%pd", &test_dentry[3]); > test("alfa/romeo", "%pd2", &test_dentry[3]); > @@ -647,9 +604,8 @@ errptr(void) > static void __init > test_pointer(void) > { > - plain(); > - null_pointer(); > - invalid_pointer(); > + plain_pointer(); > + real_pointer(); > symbol_ptr(); > kernel_ptr(); > struct_resource(); Please note that I sent v2 of my patch ("[PATCH v2] vsprintf: don't obfuscate NULL and error pointers"), fixing null_pointer() and adding error_pointer() test cases, which conflicts with this restructure. Thanks, Ilya ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] vsprintf: sanely handle NULL passed to %pe 2020-02-20 15:02 ` Ilya Dryomov @ 2020-02-21 13:05 ` Petr Mladek 2020-02-21 23:52 ` Rasmus Villemoes 0 siblings, 1 reply; 18+ messages in thread From: Petr Mladek @ 2020-02-21 13:05 UTC (permalink / raw) To: Ilya Dryomov, Rasmus Villemoes Cc: Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Jonathan Corbet, Kees Cook, Tobin C . Harding, Linus Torvalds, Linux Documentation List, LKML On Thu 2020-02-20 16:02:48, Ilya Dryomov wrote: > On Thu, Feb 20, 2020 at 1:57 PM Petr Mladek <pmladek@suse.com> wrote: > > > > On Wed 2020-02-19 16:40:08, Rasmus Villemoes wrote: > > > On 19/02/2020 15.45, Petr Mladek wrote: > > > > On Wed 2020-02-19 14:56:32, Rasmus Villemoes wrote: > > > >> On 19/02/2020 14.48, Petr Mladek wrote: > > > >>> On Wed 2020-02-19 12:53:22, Rasmus Villemoes wrote: > > > >>>> --- a/lib/vsprintf.c > > > >>>> +++ b/lib/vsprintf.c > > > >>> The test should go into null_pointer() instead of errptr(). > > > >> > > > >> Eh, no, the behaviour of %pe is tested by errptr(). I'll keep it that > > > >> way. But I should add a #else section that tests how %pe behaves without > > > >> CONFIG_SYMBOLIC_ERRNAME - though that's orthogonal to this patch. > > > > > > > > OK, we should agree on some structure first. > > > > > > > > We already have two top level functions that test how a particular > > > > pointer is printed using different pointer modifiers: > > > > > > > > null_pointer(); -> NULL with %p, %pX, %pE > > > > invalid_pointer(); -> random pointer with %p, %pX, %pE > > > > > > > > Following this logic, errptr() should test how a pointer from IS_ERR() range > > > > is printed using different pointer formats. > > > > > > Oh please. I wrote test_printf.c originally and structured it with one > > > helper for each %p<whatever>. How are your additions null_pointer and > > > invalid_pointer good examples for what the existing style is? > > > > I see, I was the one who broke the style. Please, find below a patch > > that tries to fix it. If you agree with the approach then I could > > split it into smaller steps. > > > > Also it would make sense to add checks for NULL and ERR pointer > > into each existing %p modifier check. It will make sure that > > check_pointer() is called in all handlers. > > > > > > > So yeah, I'm going to continue testing the behaviour of %pe in errptr, TYVM. > > > > OK. > > > > > >>>> BTW., your original patch for %p lacks corresponding update of > > > >>>> test_vsprintf.c. Please add appropriate test cases. > > > >>> > > > >>> diff --git a/lib/test_printf.c b/lib/test_printf.c > > > >>> index 2d9f520d2f27..1726a678bccd 100644 > > > >>> --- a/lib/test_printf.c > > > >>> +++ b/lib/test_printf.c > > > >>> @@ -333,7 +333,7 @@ test_hashed(const char *fmt, const void *p) > > > >>> static void __init > > > >>> null_pointer(void) > > > >>> { > > > >>> - test_hashed("%p", NULL); > > > >>> + test(ZEROS "00000000", "%p", NULL); > > > >> > > > >> No, it most certainly also needs to check a few "%p", ERR_PTR(-4) cases > > > >> (where one of course has to use explicit integers and not E* constants). > > > > > > > > Yes, it would be great to add checks for %p, %px for IS_ERR() range. > > > > But it is different story. The above change is for the original patch > > > > and it was about NULL pointer handling. > > > > > > Wrong. The original patch (i.e. Ilya's) had subject "vsprintf: don't > > > obfuscate NULL and error pointers" and did > > > > > > + if (IS_ERR_OR_NULL(ptr)) > > > > > > so the tests that should be part of that patch very much need to cover > > > both NULL and ERR_PTRs passed to plain %p. > > > > Grr, I see. I was too fast yesterday. OK, I suggest to fix the > > structure of the tests first. All these patches are for 5.7 > > anyway. > > My patch fixes a regression introduced by 3e5903eb9cff ("vsprintf: > Prevent crash when dereferencing invalid pointers" in 5.2, which > made debugging based on existing pr_debugs (used extensively in some > subsystems) very annoying. > > I would like to see it in 5.6, so that it is backported to 5.4 and 5.5. OK, it would make sense to make the patch minimalist to make it easier for backporting. > Please note that I sent v2 of my patch ("[PATCH v2] vsprintf: don't > obfuscate NULL and error pointers"), fixing null_pointer() and adding > error_pointer() test cases, which conflicts with this restructure. IMHO, v2 creates even more mess in print tests that would need to be fixed later. If we agree to have a minimalist patch for backport then I suggest to take v1. We could clean up and update tests later. Rasmus, others, is anyone against this approach (v1 first, tests later)? Best Regards, Petr ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] vsprintf: sanely handle NULL passed to %pe 2020-02-21 13:05 ` Petr Mladek @ 2020-02-21 23:52 ` Rasmus Villemoes 2020-02-22 8:14 ` Andy Shevchenko 2020-02-24 9:55 ` Petr Mladek 0 siblings, 2 replies; 18+ messages in thread From: Rasmus Villemoes @ 2020-02-21 23:52 UTC (permalink / raw) To: Petr Mladek, Ilya Dryomov Cc: Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Jonathan Corbet, Kees Cook, Tobin C . Harding, Linus Torvalds, Linux Documentation List, LKML On 21/02/2020 14.05, Petr Mladek wrote: > On Thu 2020-02-20 16:02:48, Ilya Dryomov wrote: >> I would like to see it in 5.6, so that it is backported to 5.4 and 5.5. > > OK, it would make sense to make the patch minimalist to make it > easier for backporting. > > >> Please note that I sent v2 of my patch ("[PATCH v2] vsprintf: don't >> obfuscate NULL and error pointers"), fixing null_pointer() and adding >> error_pointer() test cases, which conflicts with this restructure. > > IMHO, v2 creates even more mess in print tests that would need > to be fixed later. > > If we agree to have a minimalist patch for backport > then I suggest to take v1. We could clean up and update > tests later. > > Rasmus, others, is anyone against this approach (v1 first, > tests later)? Sorry to be that guy, but yes, I'm against changing the behavior of vsnprintf() without at least some test(s) added to the test suite - the lack of machine-checked documentation in the form of tests is what led to that regression in the first place. But I agree that there's no point adding another helper function and muddying the test suite even more (especially as the name error_pointer is too close to the name errptr() I chose a few months back for the %pe). So how about - remove the now stale test_hashed("%p", NULL); from null_pointer() - add tests of "%p", NULL and "%p", ERR_PTR(-123) to plain() and we save testing the "%px" case for when we figure out a good name for a helper for that (explicit_pointer? pointer_as_hex?) ? Rasmus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] vsprintf: sanely handle NULL passed to %pe 2020-02-21 23:52 ` Rasmus Villemoes @ 2020-02-22 8:14 ` Andy Shevchenko 2020-02-24 9:55 ` Petr Mladek 1 sibling, 0 replies; 18+ messages in thread From: Andy Shevchenko @ 2020-02-22 8:14 UTC (permalink / raw) To: Rasmus Villemoes Cc: Petr Mladek, Ilya Dryomov, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Jonathan Corbet, Kees Cook, Tobin C . Harding, Linus Torvalds, Linux Documentation List, LKML On Sat, Feb 22, 2020 at 1:53 AM Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > On 21/02/2020 14.05, Petr Mladek wrote: ... > and we save testing the "%px" case for when we figure out a good name > for a helper for that (explicit_pointer? pointer_as_hex?) > > ? real_pointer() ? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] vsprintf: sanely handle NULL passed to %pe 2020-02-21 23:52 ` Rasmus Villemoes 2020-02-22 8:14 ` Andy Shevchenko @ 2020-02-24 9:55 ` Petr Mladek 1 sibling, 0 replies; 18+ messages in thread From: Petr Mladek @ 2020-02-24 9:55 UTC (permalink / raw) To: Rasmus Villemoes Cc: Ilya Dryomov, Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko, Jonathan Corbet, Kees Cook, Tobin C . Harding, Linus Torvalds, Linux Documentation List, LKML On Sat 2020-02-22 00:52:27, Rasmus Villemoes wrote: > On 21/02/2020 14.05, Petr Mladek wrote: > > On Thu 2020-02-20 16:02:48, Ilya Dryomov wrote: > > >> I would like to see it in 5.6, so that it is backported to 5.4 and 5.5. > > > Sorry to be that guy, but yes, I'm against changing the behavior of > vsnprintf() without at least some test(s) added to the test suite - the > lack of machine-checked documentation in the form of tests is what led > to that regression in the first place. I would not call this regression. It was intentional. The change in 5.2 unified the behavior for the other %p? modifiers. I simply did not care about plain %p because it was already crippled by the hashing. I am fine with the proposed change. But the more I think about it the less I want to rush it in for 5.6. The proposed patch changes the behavior again: Value Output v5.1 Output v5.2 Proposal NULL (null) 00000000<.hash.> 0000000000000000 fffffffffffffffe 00000000<.hash.> 00000000<.hash.> fffffffffffffffe ffffffff12345678 00000000<.hash.> 00000000<.hash.> 00000000<.hash.> I do not want to change this in rc phase. I would really like to wait for 5.7. > But I agree that there's no point adding another helper function and > muddying the test suite even more (especially as the name error_pointer > is too close to the name errptr() I chose a few months back for the %pe). > > So how about > > - remove the now stale test_hashed("%p", NULL); from null_pointer() > - add tests of "%p", NULL and "%p", ERR_PTR(-123) to plain() > > and we save testing the "%px" case for when we figure out a good name > for a helper for that (explicit_pointer? pointer_as_hex?) In this, case I would prefer to fix the tests properly first. There have been only few commits in lib/test_printf.c since 5.2. And they should not conflict with the changes proposed at https://lkml.kernel.org/r/20200220125707.hbcox3xgevpezq4l@pathway.suse.cz So it should be easy to backport as well. If you want, I could sent the cleanup patch properly for review. Best Regards, Petr ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-02-24 9:55 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CAHk-=wjEd-gZ1g52kgi_g8gq-QCF2E01TkQd5Hmj4W5aThLw3A@mail.gmail.com> 2020-02-19 8:21 ` [PATCH] vsprintf: sanely handle NULL passed to %pe Rasmus Villemoes 2020-02-19 9:35 ` Andy Shevchenko 2020-02-19 11:20 ` Ilya Dryomov 2020-02-19 11:25 ` Andy Shevchenko 2020-02-19 11:29 ` Ilya Dryomov 2020-02-19 11:53 ` Rasmus Villemoes 2020-02-19 13:48 ` Petr Mladek 2020-02-19 13:56 ` Rasmus Villemoes 2020-02-19 14:45 ` Petr Mladek 2020-02-19 15:38 ` Andy Shevchenko 2020-02-19 15:40 ` Rasmus Villemoes 2020-02-19 17:23 ` Ilya Dryomov 2020-02-20 12:57 ` Petr Mladek 2020-02-20 15:02 ` Ilya Dryomov 2020-02-21 13:05 ` Petr Mladek 2020-02-21 23:52 ` Rasmus Villemoes 2020-02-22 8:14 ` Andy Shevchenko 2020-02-24 9:55 ` Petr Mladek
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).