I don't see what security concern is addressed by obfuscating NULL
and IS_ERR() error pointers, printed with %p/%pK. Given the number
of sites where %p is used (over 10000) and the fact that NULL pointers
aren't uncommon, it probably wouldn't take long for an attacker to
find the hash that corresponds to 0. Although harder, the same goes
for most common error values, such as -1, -2, -11, -14, etc.
The NULL part actually fixes a regression: NULL pointers weren't
obfuscated until commit 3e5903eb9cff ("vsprintf: Prevent crash when
dereferencing invalid pointers") which went into 5.2. I'm tacking
the IS_ERR() part on here because error pointers won't leak kernel
addresses and printing them as pointers shouldn't be any different
from e.g. %d with PTR_ERR_OR_ZERO(). Obfuscating them just makes
debugging based on existing pr_debug and friends excruciating.
Note that the "always print 0's for %pK when kptr_restrict == 2"
behaviour which goes way back is left as is.
Example output with the patch applied:
ptr error-ptr NULL
%p: 0000000001f8cc5b fffffffffffffff2 0000000000000000
%pK, kptr = 0: 0000000001f8cc5b fffffffffffffff2 0000000000000000
%px: ffff888048c04020 fffffffffffffff2 0000000000000000
%pK, kptr = 1: ffff888048c04020 fffffffffffffff2 0000000000000000
%pK, kptr = 2: 0000000000000000 0000000000000000 0000000000000000
Fixes: 3e5903eb9cff ("vsprintf: Prevent crash when dereferencing invalid pointers")
Signed-off-by: Ilya Dryomov <[email protected]>
---
lib/vsprintf.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7c488a1ce318..f0f0522cd5a7 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -794,6 +794,13 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr,
unsigned long hashval;
int ret;
+ /*
+ * Print the real pointer value for NULL and error pointers,
+ * as they are not actual addresses.
+ */
+ if (IS_ERR_OR_NULL(ptr))
+ return pointer_string(buf, end, ptr, spec);
+
/* When debugging early boot use non-cryptographically secure hash. */
if (unlikely(debug_boot_weak_hash)) {
hashval = hash_long((unsigned long)ptr, 32);
--
2.19.2
On Mon, Feb 17, 2020 at 11:28:03PM +0100, Ilya Dryomov wrote:
> I don't see what security concern is addressed by obfuscating NULL
> and IS_ERR() error pointers, printed with %p/%pK. Given the number
> of sites where %p is used (over 10000) and the fact that NULL pointers
> aren't uncommon, it probably wouldn't take long for an attacker to
> find the hash that corresponds to 0. Although harder, the same goes
> for most common error values, such as -1, -2, -11, -14, etc.
>
> The NULL part actually fixes a regression: NULL pointers weren't
> obfuscated until commit 3e5903eb9cff ("vsprintf: Prevent crash when
> dereferencing invalid pointers") which went into 5.2. I'm tacking
> the IS_ERR() part on here because error pointers won't leak kernel
> addresses and printing them as pointers shouldn't be any different
> from e.g. %d with PTR_ERR_OR_ZERO(). Obfuscating them just makes
> debugging based on existing pr_debug and friends excruciating.
>
> Note that the "always print 0's for %pK when kptr_restrict == 2"
> behaviour which goes way back is left as is.
>
> Example output with the patch applied:
>
> ptr error-ptr NULL
> %p: 0000000001f8cc5b fffffffffffffff2 0000000000000000
> %pK, kptr = 0: 0000000001f8cc5b fffffffffffffff2 0000000000000000
> %px: ffff888048c04020 fffffffffffffff2 0000000000000000
> %pK, kptr = 1: ffff888048c04020 fffffffffffffff2 0000000000000000
> %pK, kptr = 2: 0000000000000000 0000000000000000 0000000000000000
This seems reasonable. Though I wonder -- since the efault string is
exposed now -- should this instead print all the error-ptr strings
instead of the unsigned negative pointer value?
-Kees
>
> Fixes: 3e5903eb9cff ("vsprintf: Prevent crash when dereferencing invalid pointers")
> Signed-off-by: Ilya Dryomov <[email protected]>
> ---
> lib/vsprintf.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 7c488a1ce318..f0f0522cd5a7 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -794,6 +794,13 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr,
> unsigned long hashval;
> int ret;
>
> + /*
> + * Print the real pointer value for NULL and error pointers,
> + * as they are not actual addresses.
> + */
> + if (IS_ERR_OR_NULL(ptr))
> + return pointer_string(buf, end, ptr, spec);
> +
> /* When debugging early boot use non-cryptographically secure hash. */
> if (unlikely(debug_boot_weak_hash)) {
> hashval = hash_long((unsigned long)ptr, 32);
> --
> 2.19.2
>
--
Kees Cook
On Tue, Feb 18, 2020 at 12:47 AM Kees Cook <[email protected]> wrote:
>
> On Mon, Feb 17, 2020 at 11:28:03PM +0100, Ilya Dryomov wrote:
> > I don't see what security concern is addressed by obfuscating NULL
> > and IS_ERR() error pointers, printed with %p/%pK. Given the number
> > of sites where %p is used (over 10000) and the fact that NULL pointers
> > aren't uncommon, it probably wouldn't take long for an attacker to
> > find the hash that corresponds to 0. Although harder, the same goes
> > for most common error values, such as -1, -2, -11, -14, etc.
> >
> > The NULL part actually fixes a regression: NULL pointers weren't
> > obfuscated until commit 3e5903eb9cff ("vsprintf: Prevent crash when
> > dereferencing invalid pointers") which went into 5.2. I'm tacking
> > the IS_ERR() part on here because error pointers won't leak kernel
> > addresses and printing them as pointers shouldn't be any different
> > from e.g. %d with PTR_ERR_OR_ZERO(). Obfuscating them just makes
> > debugging based on existing pr_debug and friends excruciating.
> >
> > Note that the "always print 0's for %pK when kptr_restrict == 2"
> > behaviour which goes way back is left as is.
> >
> > Example output with the patch applied:
> >
> > ptr error-ptr NULL
> > %p: 0000000001f8cc5b fffffffffffffff2 0000000000000000
> > %pK, kptr = 0: 0000000001f8cc5b fffffffffffffff2 0000000000000000
> > %px: ffff888048c04020 fffffffffffffff2 0000000000000000
> > %pK, kptr = 1: ffff888048c04020 fffffffffffffff2 0000000000000000
> > %pK, kptr = 2: 0000000000000000 0000000000000000 0000000000000000
>
> This seems reasonable. Though I wonder -- since the efault string is
> exposed now -- should this instead print all the error-ptr strings
> instead of the unsigned negative pointer value?
I'm not sure what you mean by efault string. Are you referring to what
%pe is doing? If so, no -- I would keep %p and %pe separate.
Thanks,
Ilya
On Tue 2020-02-18 01:07:53, Ilya Dryomov wrote:
> On Tue, Feb 18, 2020 at 12:47 AM Kees Cook <[email protected]> wrote:
> >
> > On Mon, Feb 17, 2020 at 11:28:03PM +0100, Ilya Dryomov wrote:
> > > I don't see what security concern is addressed by obfuscating NULL
> > > and IS_ERR() error pointers, printed with %p/%pK. Given the number
> > > of sites where %p is used (over 10000) and the fact that NULL pointers
> > > aren't uncommon, it probably wouldn't take long for an attacker to
> > > find the hash that corresponds to 0. Although harder, the same goes
> > > for most common error values, such as -1, -2, -11, -14, etc.
> > >
> > > The NULL part actually fixes a regression: NULL pointers weren't
> > > obfuscated until commit 3e5903eb9cff ("vsprintf: Prevent crash when
> > > dereferencing invalid pointers") which went into 5.2. I'm tacking
> > > the IS_ERR() part on here because error pointers won't leak kernel
> > > addresses and printing them as pointers shouldn't be any different
> > > from e.g. %d with PTR_ERR_OR_ZERO(). Obfuscating them just makes
> > > debugging based on existing pr_debug and friends excruciating.
> > >
> > > Note that the "always print 0's for %pK when kptr_restrict == 2"
> > > behaviour which goes way back is left as is.
> > >
> > > Example output with the patch applied:
> > >
> > > ptr error-ptr NULL
> > > %p: 0000000001f8cc5b fffffffffffffff2 0000000000000000
> > > %pK, kptr = 0: 0000000001f8cc5b fffffffffffffff2 0000000000000000
> > > %px: ffff888048c04020 fffffffffffffff2 0000000000000000
> > > %pK, kptr = 1: ffff888048c04020 fffffffffffffff2 0000000000000000
> > > %pK, kptr = 2: 0000000000000000 0000000000000000 0000000000000000
> >
> > This seems reasonable. Though I wonder -- since the efault string is
> > exposed now -- should this instead print all the error-ptr strings
> > instead of the unsigned negative pointer value?
It would make sense to distinguish it from a hashed value that might
be in the NULL or ERR range as well.
The chance is small. But it might safe people from spending time
on false paths.
That said, I am fine to accept the patch as is. It makes sense
and it does not need to be perfect. After all, one motivation
behind the hashed %p was to make it useless and motivate people
to remove it. And I am sure that someone will send a patch adding
error-ptr sooner or later anyway ;-)
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On Tue, Feb 18, 2020 at 11:33 AM Petr Mladek <[email protected]> wrote:
>
> On Tue 2020-02-18 01:07:53, Ilya Dryomov wrote:
> > On Tue, Feb 18, 2020 at 12:47 AM Kees Cook <[email protected]> wrote:
> > >
> > > On Mon, Feb 17, 2020 at 11:28:03PM +0100, Ilya Dryomov wrote:
> > > > I don't see what security concern is addressed by obfuscating NULL
> > > > and IS_ERR() error pointers, printed with %p/%pK. Given the number
> > > > of sites where %p is used (over 10000) and the fact that NULL pointers
> > > > aren't uncommon, it probably wouldn't take long for an attacker to
> > > > find the hash that corresponds to 0. Although harder, the same goes
> > > > for most common error values, such as -1, -2, -11, -14, etc.
> > > >
> > > > The NULL part actually fixes a regression: NULL pointers weren't
> > > > obfuscated until commit 3e5903eb9cff ("vsprintf: Prevent crash when
> > > > dereferencing invalid pointers") which went into 5.2. I'm tacking
> > > > the IS_ERR() part on here because error pointers won't leak kernel
> > > > addresses and printing them as pointers shouldn't be any different
> > > > from e.g. %d with PTR_ERR_OR_ZERO(). Obfuscating them just makes
> > > > debugging based on existing pr_debug and friends excruciating.
> > > >
> > > > Note that the "always print 0's for %pK when kptr_restrict == 2"
> > > > behaviour which goes way back is left as is.
> > > >
> > > > Example output with the patch applied:
> > > >
> > > > ptr error-ptr NULL
> > > > %p: 0000000001f8cc5b fffffffffffffff2 0000000000000000
> > > > %pK, kptr = 0: 0000000001f8cc5b fffffffffffffff2 0000000000000000
> > > > %px: ffff888048c04020 fffffffffffffff2 0000000000000000
> > > > %pK, kptr = 1: ffff888048c04020 fffffffffffffff2 0000000000000000
> > > > %pK, kptr = 2: 0000000000000000 0000000000000000 0000000000000000
> > >
> > > This seems reasonable. Though I wonder -- since the efault string is
> > > exposed now -- should this instead print all the error-ptr strings
> > > instead of the unsigned negative pointer value?
>
> It would make sense to distinguish it from a hashed value that might
> be in the NULL or ERR range as well.
>
> The chance is small. But it might safe people from spending time
> on false paths.
Yeah, when the obfuscation patch went in, NULL was "(null)", but
that got dropped later in an argument that "(null)" should only
be printed on dereference attempts and also in an effort to make it
consistent with %pK. I don't agree with that because "(null)" dated
back to 2009 and %pK has always been a special case, but I decided
to go with the minimal fix to avoid bike shedding.
For error pointers, at least on 64-bit they are easy to distinguish
because the upper 32 bits of the hash are cleared.
Thanks,
Ilya
On Tue, 18 Feb 2020 11:33:46 +0100
Petr Mladek <[email protected]> wrote:
> Reviewed-by: Petr Mladek <[email protected]>
For what it's worth, I like the patch as well.
Acked-by: Steven Rostedt (VMware) <[email protected]>
-- Steve
On Mon, Feb 17, 2020 at 2:27 PM Ilya Dryomov <[email protected]> wrote:
>
> I don't see what security concern is addressed by obfuscating NULL
> and IS_ERR() error pointers, printed with %p/%pK.
Ack, looks good to me.
Linus
On Mon, Feb 17, 2020 at 4:07 PM Ilya Dryomov <[email protected]> wrote:
>
> I'm not sure what you mean by efault string. Are you referring to what
> %pe is doing? If so, no -- I would keep %p and %pe separate.
Right.
But bringing up %pe makes me realize that we do odd things for NULL
for that. We print errors in a nice legible form, but we show NULL as
a zero value, I think.
So maybe %pe should show NULL as "(null)"? Or even as just "0" to go
with the error names that just look like the integer error syntax (eg
"-EINVAL")
Linus
On Tue, Feb 18, 2020 at 11:31 AM Adam Borowski <[email protected]> wrote:
>
> Either "0" or "NULL" (or "∅" if you allow cp437-subset Unicode ) wouldn't
> cause such confusion.
An all-uppercase "NULL" probably matches the error code printout
syntax better too, and is more clearly a pointer.
And with %pe you can't assume columnar output anyway (unless you
explicitly ask for some width), so the length of the output cannot
matter.
So yeah, I agree. To extend on Ilya's example:
ptr error-ptr NULL
%p: 0000000001f8cc5b fffffffffffffff2 0000000000000000
%pK, kptr = 0: 0000000001f8cc5b fffffffffffffff2 0000000000000000
%px: ffff888048c04020 fffffffffffffff2 0000000000000000
%pK, kptr = 1: ffff888048c04020 fffffffffffffff2 0000000000000000
%pK, kptr = 2: 0000000000000000 0000000000000000 0000000000000000
%p: 0000000001f8cc5b -EFAULT NULL
would seem to be a sane output format. Hmm?
Linus
On Tue, Feb 18, 2020 at 10:49:30AM -0800, Linus Torvalds wrote:
> On Mon, Feb 17, 2020 at 4:07 PM Ilya Dryomov <[email protected]> wrote:
> >
> > I'm not sure what you mean by efault string. Are you referring to what
> > %pe is doing? If so, no -- I would keep %p and %pe separate.
>
> Right.
>
> But bringing up %pe makes me realize that we do odd things for NULL
> for that. We print errors in a nice legible form, but we show NULL as
> a zero value, I think.
>
> So maybe %pe should show NULL as "(null)"? Or even as just "0" to go
> with the error names that just look like the integer error syntax (eg
> "-EINVAL")
"(null)" stands for a dereference of a null pointer rather than for printing
the pointer itself. This is a convention copied from glibc's printf("%s").
Either "0" or "NULL" (or "∅" if you allow cp437-subset Unicode ☺ ) wouldn't
cause such confusion.
Meow!
--
⢀⣴⠾⠻⢶⣦⠀ A MAP07 (Dead Simple) raspberry tincture recipe: 0.5l 95% alcohol,
⣾⠁⢠⠒⠀⣿⡁ 1kg raspberries, 0.4kg sugar; put into a big jar for 1 month.
⢿⡄⠘⠷⠚⠋⠀ Filter out and throw away the fruits (can dump them into a cake,
⠈⠳⣄⠀⠀⠀⠀ etc), let the drink age at least 3-6 months.
On Tue, Feb 18, 2020 at 8:39 PM Linus Torvalds
<[email protected]> wrote:
>
> On Tue, Feb 18, 2020 at 11:31 AM Adam Borowski <[email protected]> wrote:
> >
> > Either "0" or "NULL" (or "∅" if you allow cp437-subset Unicode ) wouldn't
> > cause such confusion.
>
> An all-uppercase "NULL" probably matches the error code printout
> syntax better too, and is more clearly a pointer.
>
> And with %pe you can't assume columnar output anyway (unless you
> explicitly ask for some width), so the length of the output cannot
> matter.
>
> So yeah, I agree. To extend on Ilya's example:
>
> ptr error-ptr NULL
> %p: 0000000001f8cc5b fffffffffffffff2 0000000000000000
> %pK, kptr = 0: 0000000001f8cc5b fffffffffffffff2 0000000000000000
> %px: ffff888048c04020 fffffffffffffff2 0000000000000000
> %pK, kptr = 1: ffff888048c04020 fffffffffffffff2 0000000000000000
> %pK, kptr = 2: 0000000000000000 0000000000000000 0000000000000000
> %p: 0000000001f8cc5b -EFAULT NULL
^^^
I assume you meant %pe here.
>
> would seem to be a sane output format. Hmm?
Looks sensible to me. Without this patch NULL is obfuscated for
both %p and %pe though. Do you want this patch amended or prefer
a follow-up for %pe "0000000000000000" -> "NULL" so that it can be
discussed separately?
Thanks,
Ilya
On Tue, Feb 18, 2020 at 12:18 PM Ilya Dryomov <[email protected]> wrote:
>
> > %p: 0000000001f8cc5b -EFAULT NULL
>
> ^^^
> I assume you meant %pe here.
Heh, yes.
> Looks sensible to me. Without this patch NULL is obfuscated for
> both %p and %pe though. Do you want this patch amended or prefer
> a follow-up for %pe "0000000000000000" -> "NULL" so that it can be
> discussed separately?
Yeah, as an independent follow-up. I think your first patch is fine,
and I think this %pe NULL behavior thing is a completely separate
issue that just came up when %pe was mentioned.
Linus
On (20/02/18 11:33), Petr Mladek wrote:
> It would make sense to distinguish it from a hashed value that might
> be in the NULL or ERR range as well.
[..]
> Reviewed-by: Petr Mladek <[email protected]>
FWIW,
Reviewed-by: Sergey Senozhatsky <[email protected]>
-ss
On 18/02/2020 21.19, Ilya Dryomov wrote:
> Looks sensible to me. Without this patch NULL is obfuscated for
> both %p and %pe though. Do you want this patch amended or prefer
> a follow-up for %pe "0000000000000000" -> "NULL" so that it can be
> discussed separately?
Please cc me on all vsprintf.c patches (scripts/get_maintainer.pl should
list me as reviewer), and especially ones that touch the recently added %pe.
Thanks,
Rasmus
Extend %pe to pretty-print NULL in addition to ERR_PTRs,
i.e. everything IS_ERR_OR_NULL().
Suggested-by: Linus Torvalds <[email protected]>
Signed-off-by: Rasmus Villemoes <[email protected]>
---
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
On Wed, Feb 19, 2020 at 10:24 AM Rasmus Villemoes
<[email protected]> wrote:
>
> Extend %pe to pretty-print NULL in addition to ERR_PTRs,
> i.e. everything IS_ERR_OR_NULL().
>
Reviewed-by: Andy Shevchenko <[email protected]>
One nit below, though.
> Suggested-by: Linus Torvalds <[email protected]>
> Signed-off-by: Rasmus Villemoes <[email protected]>
> ---
> 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
On Wed, Feb 19, 2020 at 9:21 AM Rasmus Villemoes
<[email protected]> wrote:
>
> Extend %pe to pretty-print NULL in addition to ERR_PTRs,
> i.e. everything IS_ERR_OR_NULL().
>
> Suggested-by: Linus Torvalds <[email protected]>
> Signed-off-by: Rasmus Villemoes <[email protected]>
> ---
> 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
On Wed, Feb 19, 2020 at 1:21 PM Ilya Dryomov <[email protected]> wrote:
> On Wed, Feb 19, 2020 at 9:21 AM Rasmus Villemoes
> <[email protected]> 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
On Wed, Feb 19, 2020 at 12:25 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Feb 19, 2020 at 1:21 PM Ilya Dryomov <[email protected]> wrote:
> > On Wed, Feb 19, 2020 at 9:21 AM Rasmus Villemoes
> > <[email protected]> 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
On 19/02/2020 12.20, Ilya Dryomov wrote:
> On Wed, Feb 19, 2020 at 9:21 AM Rasmus Villemoes
> <[email protected]> wrote:
>>
>> Extend %pe to pretty-print NULL in addition to ERR_PTRs,
>> i.e. everything IS_ERR_OR_NULL().
>>
>> Suggested-by: Linus Torvalds <[email protected]>
>> Signed-off-by: Rasmus Villemoes <[email protected]>
>> ---
>> 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
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
> > <[email protected]> wrote:
> >>
> >> Extend %pe to pretty-print NULL in addition to ERR_PTRs,
> >> i.e. everything IS_ERR_OR_NULL().
> >>
> >> Suggested-by: Linus Torvalds <[email protected]>
> >> Signed-off-by: Rasmus Villemoes <[email protected]>
> >> ---
> >> 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
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
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
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
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
On Wed, Feb 19, 2020 at 4:40 PM Rasmus Villemoes
<[email protected]> 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
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 <[email protected]>
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 <[email protected]>
---
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
On Thu, Feb 20, 2020 at 1:57 PM Petr Mladek <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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
On Thu 2020-02-20 16:02:48, Ilya Dryomov wrote:
> On Thu, Feb 20, 2020 at 1:57 PM Petr Mladek <[email protected]> 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
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
On Sat, Feb 22, 2020 at 1:53 AM Rasmus Villemoes
<[email protected]> 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
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/[email protected]
So it should be easy to backport as well.
If you want, I could sent the cleanup patch properly for review.
Best Regards,
Petr