[PATCH] vsprintf: sanely handle NULL passed to %pe

[PATCH] vsprintf: sanely handle NULL passed to %pe

[Rasmus Villemoes]

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

Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Andy Shevchenko]

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

Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Ilya Dryomov]

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

Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Andy Shevchenko]

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

Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Ilya Dryomov]

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

Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Rasmus Villemoes]

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

Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Petr Mladek]

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

Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Rasmus Villemoes]

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

Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Petr Mladek]

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

Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Andy Shevchenko]

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

Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Rasmus Villemoes]

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

Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Ilya Dryomov]

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

Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Petr Mladek]

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

Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Ilya Dryomov]

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

Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Petr Mladek]

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

Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Rasmus Villemoes]

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

Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Andy Shevchenko]

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

Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

[Petr Mladek]

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