[PATCH v2] printf: add support for printing symbolic error codes

[PATCH v2] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

It has been suggested several times to extend vsnprintf() to be able
to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
another attempt. Rather than adding another %p extension, simply teach
plain %p to convert ERR_PTRs. While the primary use case is

  if (IS_ERR(foo)) {
    pr_err("Sorry, can't do that: %p\n", foo);
    return PTR_ERR(foo);
  }

it is also more helpful to get a symbolic error code (or, worst case,
a decimal number) in case an ERR_PTR is accidentally passed to some
%p<something>, rather than the (efault) that check_pointer() would
result in.

With my embedded hat on, I've made it possible to remove this.

I've tested that the #ifdeffery in errcode.c is sufficient to make
this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
0day bot will tell me which ones I've missed.

The symbols to include have been found by massaging the output of

  find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'

In the cases where some common aliasing exists
(e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
to the bottom so that one takes precedence.

Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
v2:
- add #include <linux/stddef.h> to errcode.h (0day)
- keep 'x' handling in switch() (Andy)
- add paragraph to Documentation/core-api/printk-formats.rst
- add ack from Uwe

 Documentation/core-api/printk-formats.rst |   8 +
 include/linux/errcode.h                   |  16 ++
 lib/Kconfig.debug                         |   8 +
 lib/Makefile                              |   1 +
 lib/errcode.c                             | 215 ++++++++++++++++++++++
 lib/test_printf.c                         |  14 ++
 lib/vsprintf.c                            |  26 +++
 7 files changed, 288 insertions(+)
 create mode 100644 include/linux/errcode.h
 create mode 100644 lib/errcode.c

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index c6224d039bcb..7d3bf3cb207b 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -66,6 +66,14 @@ might be printed instead of the unreachable information::
 	(efault) data on invalid address
 	(einval) invalid data on a valid address
 
+Error pointers, i.e. pointers for which IS_ERR() is true, are handled
+as follows: If CONFIG_SYMBOLIC_ERRCODE is set, an appropriate symbolic
+constant is printed. For example, '"%p", PTR_ERR(-ENOSPC)' results in
+"ENOSPC", while '"%p", PTR_ERR(-EWOULDBLOCK)' results in "EAGAIN"
+(since EAGAIN == EWOULDBLOCK, and the former is the most common). If
+CONFIG_SYMBOLIC_ERRCODE is not set, ERR_PTRs are printed as their
+decimal representation ("-28" and "-11" for the two examples).
+
 Plain Pointers
 --------------
 
diff --git a/include/linux/errcode.h b/include/linux/errcode.h
new file mode 100644
index 000000000000..c6a4c1b04f9c
--- /dev/null
+++ b/include/linux/errcode.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_ERRCODE_H
+#define _LINUX_ERRCODE_H
+
+#include <linux/stddef.h>
+
+#ifdef CONFIG_SYMBOLIC_ERRCODE
+const char *errcode(int err);
+#else
+static inline const char *errcode(int err)
+{
+	return NULL;
+}
+#endif
+
+#endif /* _LINUX_ERRCODE_H */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5960e2980a8a..dc1b20872774 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -164,6 +164,14 @@ config DYNAMIC_DEBUG
 	  See Documentation/admin-guide/dynamic-debug-howto.rst for additional
 	  information.
 
+config SYMBOLIC_ERRCODE
+	bool "Support symbolic error codes in printf"
+	help
+	  If you say Y here, the kernel's printf implementation will
+	  be able to print symbolic errors such as ENOSPC instead of
+	  the number 28. It makes the kernel image slightly larger
+	  (about 3KB), but can make the kernel logs easier to read.
+
 endmenu # "printk and dmesg options"
 
 menu "Compile-time checks and compiler options"
diff --git a/lib/Makefile b/lib/Makefile
index 29c02a924973..664ecf10ee2a 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -185,6 +185,7 @@ lib-$(CONFIG_GENERIC_BUG) += bug.o
 obj-$(CONFIG_HAVE_ARCH_TRACEHOOK) += syscall.o
 
 obj-$(CONFIG_DYNAMIC_DEBUG) += dynamic_debug.o
+obj-$(CONFIG_SYMBOLIC_ERRCODE) += errcode.o
 
 obj-$(CONFIG_NLATTR) += nlattr.o
 
diff --git a/lib/errcode.c b/lib/errcode.c
new file mode 100644
index 000000000000..7dcf97d5307f
--- /dev/null
+++ b/lib/errcode.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/build_bug.h>
+#include <linux/errno.h>
+#include <linux/errcode.h>
+#include <linux/kernel.h>
+
+/*
+ * Ensure these tables to not accidentally become gigantic if some
+ * huge errno makes it in. On most architectures, the first table will
+ * only have about 140 entries, but mips and parisc have more sparsely
+ * 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.
+ */
+#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
+static const char *codes_0[] = {
+	E(E2BIG),
+	E(EACCES),
+	E(EADDRINUSE),
+	E(EADDRNOTAVAIL),
+	E(EADV),
+	E(EAFNOSUPPORT),
+	E(EALREADY),
+	E(EBADE),
+	E(EBADF),
+	E(EBADFD),
+	E(EBADMSG),
+	E(EBADR),
+	E(EBADRQC),
+	E(EBADSLT),
+	E(EBFONT),
+	E(EBUSY),
+#ifdef ECANCELLED
+	E(ECANCELLED),
+#endif
+	E(ECHILD),
+	E(ECHRNG),
+	E(ECOMM),
+	E(ECONNABORTED),
+	E(ECONNRESET),
+	E(EDEADLOCK),
+	E(EDESTADDRREQ),
+	E(EDOM),
+	E(EDOTDOT),
+#ifndef CONFIG_MIPS
+	E(EDQUOT),
+#endif
+	E(EEXIST),
+	E(EFAULT),
+	E(EFBIG),
+	E(EHOSTDOWN),
+	E(EHOSTUNREACH),
+	E(EHWPOISON),
+	E(EIDRM),
+	E(EILSEQ),
+#ifdef EINIT
+	E(EINIT),
+#endif
+	E(EINPROGRESS),
+	E(EINTR),
+	E(EINVAL),
+	E(EIO),
+	E(EISCONN),
+	E(EISDIR),
+	E(EISNAM),
+	E(EKEYEXPIRED),
+	E(EKEYREJECTED),
+	E(EKEYREVOKED),
+	E(EL2HLT),
+	E(EL2NSYNC),
+	E(EL3HLT),
+	E(EL3RST),
+	E(ELIBACC),
+	E(ELIBBAD),
+	E(ELIBEXEC),
+	E(ELIBMAX),
+	E(ELIBSCN),
+	E(ELNRNG),
+	E(ELOOP),
+	E(EMEDIUMTYPE),
+	E(EMFILE),
+	E(EMLINK),
+	E(EMSGSIZE),
+	E(EMULTIHOP),
+	E(ENAMETOOLONG),
+	E(ENAVAIL),
+	E(ENETDOWN),
+	E(ENETRESET),
+	E(ENETUNREACH),
+	E(ENFILE),
+	E(ENOANO),
+	E(ENOBUFS),
+	E(ENOCSI),
+	E(ENODATA),
+	E(ENODEV),
+	E(ENOENT),
+	E(ENOEXEC),
+	E(ENOKEY),
+	E(ENOLCK),
+	E(ENOLINK),
+	E(ENOMEDIUM),
+	E(ENOMEM),
+	E(ENOMSG),
+	E(ENONET),
+	E(ENOPKG),
+	E(ENOPROTOOPT),
+	E(ENOSPC),
+	E(ENOSR),
+	E(ENOSTR),
+#ifdef ENOSYM
+	E(ENOSYM),
+#endif
+	E(ENOSYS),
+	E(ENOTBLK),
+	E(ENOTCONN),
+	E(ENOTDIR),
+	E(ENOTEMPTY),
+	E(ENOTNAM),
+	E(ENOTRECOVERABLE),
+	E(ENOTSOCK),
+	E(ENOTTY),
+	E(ENOTUNIQ),
+	E(ENXIO),
+	E(EOPNOTSUPP),
+	E(EOVERFLOW),
+	E(EOWNERDEAD),
+	E(EPERM),
+	E(EPFNOSUPPORT),
+	E(EPIPE),
+#ifdef EPROCLIM
+	E(EPROCLIM),
+#endif
+	E(EPROTO),
+	E(EPROTONOSUPPORT),
+	E(EPROTOTYPE),
+	E(ERANGE),
+	E(EREMCHG),
+#ifdef EREMDEV
+	E(EREMDEV),
+#endif
+	E(EREMOTE),
+	E(EREMOTEIO),
+#ifdef EREMOTERELEASE
+	E(EREMOTERELEASE),
+#endif
+	E(ERESTART),
+	E(ERFKILL),
+	E(EROFS),
+#ifdef ERREMOTE
+	E(ERREMOTE),
+#endif
+	E(ESHUTDOWN),
+	E(ESOCKTNOSUPPORT),
+	E(ESPIPE),
+	E(ESRCH),
+	E(ESRMNT),
+	E(ESTALE),
+	E(ESTRPIPE),
+	E(ETIME),
+	E(ETIMEDOUT),
+	E(ETOOMANYREFS),
+	E(ETXTBSY),
+	E(EUCLEAN),
+	E(EUNATCH),
+	E(EUSERS),
+	E(EXDEV),
+	E(EXFULL),
+
+	E(ECANCELED),
+	E(EAGAIN), /* EWOULDBLOCK */
+	E(ECONNREFUSED), /* EREFUSED */
+	E(EDEADLK), /* EDEADLK */
+};
+#undef E
+
+#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err
+static const char *codes_512[] = {
+	E(ERESTARTSYS),
+	E(ERESTARTNOINTR),
+	E(ERESTARTNOHAND),
+	E(ENOIOCTLCMD),
+	E(ERESTART_RESTARTBLOCK),
+	E(EPROBE_DEFER),
+	E(EOPENSTALE),
+	E(ENOPARAM),
+
+	E(EBADHANDLE),
+	E(ENOTSYNC),
+	E(EBADCOOKIE),
+	E(ENOTSUPP),
+	E(ETOOSMALL),
+	E(ESERVERFAULT),
+	E(EBADTYPE),
+	E(EJUKEBOX),
+	E(EIOCBQUEUED),
+	E(ERECALLCONFLICT),
+};
+#undef E
+
+const char *errcode(int err)
+{
+	/* Might as well accept both -EIO and EIO. */
+	if (err < 0)
+		err = -err;
+	if (err <= 0) /* INT_MIN or 0 */
+		return NULL;
+	if (err < ARRAY_SIZE(codes_0))
+		return codes_0[err];
+	if (err >= 512 && err - 512 < ARRAY_SIZE(codes_512))
+		return codes_512[err - 512];
+	/* But why? */
+	if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */
+		return "EDQUOT";
+	return NULL;
+}
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 944eb50f3862..0401a2341245 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -212,6 +212,7 @@ test_string(void)
 #define PTR_STR "ffff0123456789ab"
 #define PTR_VAL_NO_CRNG "(____ptrval____)"
 #define ZEROS "00000000"	/* hex 32 zero bits */
+#define FFFFS "ffffffff"
 
 static int __init
 plain_format(void)
@@ -243,6 +244,7 @@ plain_format(void)
 #define PTR_STR "456789ab"
 #define PTR_VAL_NO_CRNG "(ptrval)"
 #define ZEROS ""
+#define FFFFS ""
 
 static int __init
 plain_format(void)
@@ -588,6 +590,17 @@ flags(void)
 	kfree(cmp_buffer);
 }
 
+static void __init
+errptr(void)
+{
+	test("-1234", "%p", ERR_PTR(-1234));
+	test(FFFFS "ffffffff " FFFFS "ffffff00", "%px %px", ERR_PTR(-1), ERR_PTR(-256));
+#ifdef CONFIG_SYMBOLIC_ERRCODE
+	test("EIO EINVAL ENOSPC", "%p %p %p", ERR_PTR(-EIO), ERR_PTR(-EINVAL), ERR_PTR(-ENOSPC));
+	test("EAGAIN EAGAIN", "%p %p", ERR_PTR(-EAGAIN), ERR_PTR(-EWOULDBLOCK));
+#endif
+}
+
 static void __init
 test_pointer(void)
 {
@@ -610,6 +623,7 @@ test_pointer(void)
 	bitmap();
 	netdev_features();
 	flags();
+	errptr();
 }
 
 static void __init selftest(void)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b0967cf17137..bfa5c3025965 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -21,6 +21,7 @@
 #include <linux/build_bug.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/errcode.h>
 #include <linux/module.h>	/* for KSYM_SYMBOL_LEN */
 #include <linux/types.h>
 #include <linux/string.h>
@@ -2111,6 +2112,31 @@ static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
+	/*
+	 * If it's an ERR_PTR, try to print its symbolic
+	 * representation, except for %px, where the user explicitly
+	 * wanted the pointer formatted as a hex value.
+	 */
+	if (IS_ERR(ptr) && *fmt != 'x') {
+		long err = PTR_ERR(ptr);
+		const char *sym = errcode(-err);
+		if (sym)
+			return string_nocheck(buf, end, sym, spec);
+		/*
+		 * Funky, somebody passed ERR_PTR(-1234) or some other
+		 * non-existing Efoo - or more likely
+		 * CONFIG_SYMBOLIC_ERRCODE=n. None of the
+		 * %p<something> extensions can make any sense of an
+		 * ERR_PTR(), and if this was just a plain %p, the
+		 * user is still better off getting the decimal
+		 * representation rather than the hash value that
+		 * ptr_to_id() would generate.
+		 */
+		spec.flags |= SIGN;
+		spec.base = 10;
+		return number(buf, end, err, spec);
+	}
+
 	switch (*fmt) {
 	case 'F':
 	case 'f':
-- 
2.20.1

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Andy Shevchenko]

On Mon, Sep 9, 2019 at 11:39 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> It has been suggested several times to extend vsnprintf() to be able
> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
> another attempt. Rather than adding another %p extension, simply teach
> plain %p to convert ERR_PTRs. While the primary use case is
>
>   if (IS_ERR(foo)) {
>     pr_err("Sorry, can't do that: %p\n", foo);
>     return PTR_ERR(foo);
>   }
>
> it is also more helpful to get a symbolic error code (or, worst case,
> a decimal number) in case an ERR_PTR is accidentally passed to some
> %p<something>, rather than the (efault) that check_pointer() would
> result in.
>
> With my embedded hat on, I've made it possible to remove this.
>
> I've tested that the #ifdeffery in errcode.c is sufficient to make
> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
> 0day bot will tell me which ones I've missed.
>
> The symbols to include have been found by massaging the output of
>
>   find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'
>
> In the cases where some common aliasing exists
> (e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
> I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
> to the bottom so that one takes precedence.

> +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
> +#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err

From long term prospective 300 and 550 hard coded here may be forgotten.

> +const char *errcode(int err)
We got long, why not to use long type for it?

> +{
> +       /* Might as well accept both -EIO and EIO. */
> +       if (err < 0)
> +               err = -err;

> +       if (err <= 0) /* INT_MIN or 0 */
> +               return NULL;
> +       if (err < ARRAY_SIZE(codes_0))

> +               return codes_0[err];

It won't work if one of the #ifdef:s in the array fails.
Would it?

> +       if (err >= 512 && err - 512 < ARRAY_SIZE(codes_512))
> +               return codes_512[err - 512];
> +       /* But why? */
> +       if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */
> +               return "EDQUOT";
> +       return NULL;
> +}

> +               long err = PTR_ERR(ptr);
> +               const char *sym = errcode(-err);

Do we need additional sign change if we already have such check inside
errcode()?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Joe Perches]

On Tue, 2019-09-10 at 18:26 +0300, Andy Shevchenko wrote:
> On Mon, Sep 9, 2019 at 11:39 PM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
> > It has been suggested several times to extend vsnprintf() to be able
> > to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
> > another attempt. Rather than adding another %p extension, simply teach
> > plain %p to convert ERR_PTRs. While the primary use case is
> > 
> >   if (IS_ERR(foo)) {
> >     pr_err("Sorry, can't do that: %p\n", foo);
> >     return PTR_ERR(foo);
> >   }
> > 
> > it is also more helpful to get a symbolic error code (or, worst case,
> > a decimal number) in case an ERR_PTR is accidentally passed to some
> > %p<something>, rather than the (efault) that check_pointer() would
> > result in.
> > 
> > With my embedded hat on, I've made it possible to remove this.
> > 
> > I've tested that the #ifdeffery in errcode.c is sufficient to make
> > this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
> > 0day bot will tell me which ones I've missed.
> > 
> > The symbols to include have been found by massaging the output of
> > 
> >   find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'
> > 
> > In the cases where some common aliasing exists
> > (e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
> > I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
> > to the bottom so that one takes precedence.
> > +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
> > +#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err
> 
> From long term prospective 300 and 550 hard coded here may be forgotten.
> 
> > +const char *errcode(int err)
> We got long, why not to use long type for it?
> 
> > +{
> > +       /* Might as well accept both -EIO and EIO. */
> > +       if (err < 0)
> > +               err = -err;
> > +       if (err <= 0) /* INT_MIN or 0 */
> > +               return NULL;
> > +       if (err < ARRAY_SIZE(codes_0))
> > +               return codes_0[err];
> 
> It won't work if one of the #ifdef:s in the array fails.
> Would it?
> 
> > +       if (err >= 512 && err - 512 < ARRAY_SIZE(codes_512))
> > +               return codes_512[err - 512];
> > +       /* But why? */
> > +       if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */
> > +               return "EDQUOT";
> > +       return NULL;
> > +}
> > +               long err = PTR_ERR(ptr);
> > +               const char *sym = errcode(-err);
> 
> Do we need additional sign change if we already have such check inside
> errcode()?

How is EBUSY differentiated from ZERO_SIZE_PTR ?

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

On 11/09/2019 02.15, Joe Perches wrote:
> On Tue, 2019-09-10 at 18:26 +0300, Andy Shevchenko wrote:
>> On Mon, Sep 9, 2019 at 11:39 PM Rasmus Villemoes
>> <linux@rasmusvillemoes.dk> wrote:

>>> +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
>>> +#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err
>>
>> From long term prospective 300 and 550 hard coded here may be forgotten.

No? The point of the BUILD_BUG_ON_ZEROs is that if you add a new
Esomething, you'll get an instant build error if Esomething doesn't fit
nicely in the array you put it in. Then one can go back and figure out
whether the limit should be raised, a new codes_foo should be created,
or if it's early enough so it's not ABI yet, simply change Esomething to
a saner value.

A much bigger problem is that it's possible to add something to some
errno.h without updating this table, but there's no good solution for
that, I'm afraid. However, new Esomething are very rarely added, and
printf() will still handle it gracefully until somebody notices.

>>> +const char *errcode(int err)
>> We got long, why not to use long type for it?

Because errno values by definition have type int - and the linux syscall
ABI very clearly limits values to [1,4095]. I can change the type used
in vsnprintf.c if you prefer.

>>> +{
>>> +       /* Might as well accept both -EIO and EIO. */
>>> +       if (err < 0)
>>> +               err = -err;
>>> +       if (err <= 0) /* INT_MIN or 0 */
>>> +               return NULL;
>>> +       if (err < ARRAY_SIZE(codes_0))
>>> +               return codes_0[err];
>>
>> It won't work if one of the #ifdef:s in the array fails.
>> Would it?

I don't understand what you mean. How can an ifdef fail(?), and what
exactly won't work?

>>> +}
>>> +               long err = PTR_ERR(ptr);
>>> +               const char *sym = errcode(-err);
>>
>> Do we need additional sign change if we already have such check inside
>> errcode()?

Nah, but I went back and forth on this and ended up falling between two
stools. I think I'll drop the handling of negative arguments to
errcode(), the INT_MIN case makes that slightly ugly anyway.

> How is EBUSY differentiated from ZERO_SIZE_PTR ?

Huh? ZERO_SIZE_PTR aka (void*)(16L) is not IS_ERR(), so we won't get
here in that case.

Rasmus

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2060 bytes --]

On 9/11/19 8:43 AM, Rasmus Villemoes wrote:
> On 11/09/2019 02.15, Joe Perches wrote:
>> On Tue, 2019-09-10 at 18:26 +0300, Andy Shevchenko wrote:
>>> On Mon, Sep 9, 2019 at 11:39 PM Rasmus Villemoes
>>> <linux@rasmusvillemoes.dk> wrote:
> 
>>>> +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
>>>> +#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err
>>>
>>> From long term prospective 300 and 550 hard coded here may be forgotten.
> 
> No? The point of the BUILD_BUG_ON_ZEROs is that if you add a new
> Esomething, you'll get an instant build error if Esomething doesn't fit
> nicely in the array you put it in. Then one can go back and figure out
> whether the limit should be raised, a new codes_foo should be created,
> or if it's early enough so it's not ABI yet, simply change Esomething to
> a saner value.
> 
> A much bigger problem is that it's possible to add something to some
> errno.h without updating this table, but there's no good solution for
> that, I'm afraid. However, new Esomething are very rarely added, and
> printf() will still handle it gracefully until somebody notices.
> 
>>>> +const char *errcode(int err)
>>> We got long, why not to use long type for it?
> 
> Because errno values by definition have type int - and the linux syscall
> ABI very clearly limits values to [1,4095]. I can change the type used
> in vsnprintf.c if you prefer.
> 
>>>> +{
>>>> +       /* Might as well accept both -EIO and EIO. */
>>>> +       if (err < 0)
>>>> +               err = -err;
>>>> +       if (err <= 0) /* INT_MIN or 0 */
>>>> +               return NULL;
>>>> +       if (err < ARRAY_SIZE(codes_0))
>>>> +               return codes_0[err];
>>>
>>> It won't work if one of the #ifdef:s in the array fails.
>>> Would it?
> 
> I don't understand what you mean. How can an ifdef fail(?), and what
> exactly won't work?

I think Joe means: What happens if codes_0[57] is "" because there is no
ESOMETHING with value 57.

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

On 11/09/2019 11.37, Uwe Kleine-König wrote:
> On 9/11/19 8:43 AM, Rasmus Villemoes wrote:
>> On 11/09/2019 02.15, Joe Perches wrote:
>>> On Tue, 2019-09-10 at 18:26 +0300, Andy Shevchenko wrote:
>>>> On Mon, Sep 9, 2019 at 11:39 PM Rasmus Villemoes
>>>> <linux@rasmusvillemoes.dk> wrote:
>>>>> +{
>>>>> +       /* Might as well accept both -EIO and EIO. */
>>>>> +       if (err < 0)
>>>>> +               err = -err;
>>>>> +       if (err <= 0) /* INT_MIN or 0 */
>>>>> +               return NULL;
>>>>> +       if (err < ARRAY_SIZE(codes_0))
>>>>> +               return codes_0[err];
>>>>
>>>> It won't work if one of the #ifdef:s in the array fails.
>>>> Would it?
>>
>> I don't understand what you mean. How can an ifdef fail(?), and what
>> exactly won't work?
> 
> I think Joe means: What happens if codes_0[57] is "" because there is no
> ESOMETHING with value 57.

[That was Andy, not Joe, I was lazy and replied to both in one email
since Joe quoted all of Andy's questions].

So first, for good measure, codes_0[57] may be NULL but not "". Second,
if we're passed 57 but no Exxx on this architecture has the value 57,
then yes, we return NULL, just as if we're passed -18 or 0 or 1234. But
57 (or -57, or ERR_PTR(-57)) would realistically never find its way into
an err variable (be it pointer or integer) in the first place when no
ESOMETHING has the value 57.

Except for the case where somebody in the future adds #define ESOMETHING
57 to their asm/errno.h and starts using ESOMETHING, without updating
the table in errcode.c. But if that's the case, letting the caller
handle the "sorry, I can't translate 57 to some string" is still the
right thing to do.

Rasmus

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Pavel Machek]

On Mon 2019-09-09 22:38:25, Rasmus Villemoes wrote:
> It has been suggested several times to extend vsnprintf() to be able
> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
> another attempt. Rather than adding another %p extension, simply teach
> plain %p to convert ERR_PTRs. While the primary use case is

For the record, I hate manually decoding errors, so I like this patch.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2000 bytes --]

Hello Rasmus,

On 9/9/19 10:38 PM, Rasmus Villemoes wrote:
> It has been suggested several times to extend vsnprintf() to be able
> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
> another attempt. Rather than adding another %p extension, simply teach
> plain %p to convert ERR_PTRs. While the primary use case is
> 
>   if (IS_ERR(foo)) {
>     pr_err("Sorry, can't do that: %p\n", foo);
>     return PTR_ERR(foo);
>   }
> 
> it is also more helpful to get a symbolic error code (or, worst case,
> a decimal number) in case an ERR_PTR is accidentally passed to some
> %p<something>, rather than the (efault) that check_pointer() would
> result in.
> 
> With my embedded hat on, I've made it possible to remove this.
> 
> I've tested that the #ifdeffery in errcode.c is sufficient to make
> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
> 0day bot will tell me which ones I've missed.
> 
> The symbols to include have been found by massaging the output of
> 
>   find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'
> 
> In the cases where some common aliasing exists
> (e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
> I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
> to the bottom so that one takes precedence.
> 
> Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Even with my ack already given I still think having %pE (or %pe) for
ints holding an error code is sensible. So I wonder if it would be a
good idea to split this patch into one that introduces errcode() and
then the patch that teaches vsprintf about emitting its return value for
error valued pointers. Then I could rebase my initial patch for %pe on
top of your first one.

Other than that I wonder how we can go forward from here. So I think it
is time for v3 which picks up the few suggestions.

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

On 16/09/2019 14.23, Uwe Kleine-König wrote:
> Hello Rasmus,
> 
> On 9/9/19 10:38 PM, Rasmus Villemoes wrote:
>> It has been suggested several times to extend vsnprintf() to be able
>> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
>> another attempt. Rather than adding another %p extension, simply teach
>> plain %p to convert ERR_PTRs. While the primary use case is
>>
>>   if (IS_ERR(foo)) {
>>     pr_err("Sorry, can't do that: %p\n", foo);
>>     return PTR_ERR(foo);
>>   }
>>
>> it is also more helpful to get a symbolic error code (or, worst case,
>> a decimal number) in case an ERR_PTR is accidentally passed to some
>> %p<something>, rather than the (efault) that check_pointer() would
>> result in.
>>
>> With my embedded hat on, I've made it possible to remove this.
>>
>> I've tested that the #ifdeffery in errcode.c is sufficient to make
>> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
>> 0day bot will tell me which ones I've missed.
>>
>> The symbols to include have been found by massaging the output of
>>
>>   find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'
>>
>> In the cases where some common aliasing exists
>> (e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
>> I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
>> to the bottom so that one takes precedence.
>>
>> Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> 
> Even with my ack already given I still think having %pE (or %pe) for
> ints holding an error code is sensible. 

I don't understand why you'd want an explicit %p<something> to do what
%p does by itself - in fact, with the current vsnprintf implementation,
"%pe", ERR_PTR(-EFOO) would already do what you want (since after %p is
processed, all alphanumeric are skipped whether they were interpreted or
not). So we could "reserve" %pe perhaps in order to make the call sites
a little more readable, but no code change in vsnprintf.c would be
necessary.

Or did you mean %pe with the argument being an (int*), so one would do

  if (err < 0)
    pr_err("bad: %pe\n", &err);

Maybe I'd buy that one, though I don't think it's much worse to do

  if (err < 0)
    pr_err("bad: %p\n", ERR_PTR(err));

Also, the former has less type safety/type genericity than the latter;
if err happens to be a long (or s8 or s16) the former won't work while
the latter will.

Or perhaps you meant introduce a %d<something> extension? I still think
that's a bad idea, and I've in the meantime found another reason
(covering %d in particular): Netdevices can be given a name containing
exactly one occurrence of %d (or no % at all), and then the actual name
will be determined based on that pattern. These patterns are settable
from userspace. And everything of course breaks horribly if somebody set
a name to "bla%deth" and that got turned into "blaEPERMth".

> So I wonder if it would be a
> good idea to split this patch into one that introduces errcode() and
> then the patch that teaches vsprintf about emitting its return value for
> error valued pointers. Then I could rebase my initial patch for %pe on
> top of your first one.

Well, I think my patch as-is is simple enough, there's not much point
separating the few lines in vsnprintf() from the introduction of
errcode() (which, realistically, will never have other callers).

> Other than that I wonder how we can go forward from here. So I think it
> is time for v3 which picks up the few suggestions.

Yes, I have actually prepared a v3, was just waiting for additional
comments on my responses to the v2 review comments.

Rasmus

Re: [PATCH v2] printf: add support for printing symbolic error codes

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 3786 bytes --]

On 9/16/19 3:23 PM, Rasmus Villemoes wrote:
> On 16/09/2019 14.23, Uwe Kleine-König wrote:
>> Hello Rasmus,
>>
>> On 9/9/19 10:38 PM, Rasmus Villemoes wrote:
>>> It has been suggested several times to extend vsnprintf() to be able
>>> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
>>> another attempt. Rather than adding another %p extension, simply teach
>>> plain %p to convert ERR_PTRs. While the primary use case is
>>>
>>>   if (IS_ERR(foo)) {
>>>     pr_err("Sorry, can't do that: %p\n", foo);
>>>     return PTR_ERR(foo);
>>>   }
>>>
>>> it is also more helpful to get a symbolic error code (or, worst case,
>>> a decimal number) in case an ERR_PTR is accidentally passed to some
>>> %p<something>, rather than the (efault) that check_pointer() would
>>> result in.
>>>
>>> With my embedded hat on, I've made it possible to remove this.
>>>
>>> I've tested that the #ifdeffery in errcode.c is sufficient to make
>>> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
>>> 0day bot will tell me which ones I've missed.
>>>
>>> The symbols to include have been found by massaging the output of
>>>
>>>   find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'
>>>
>>> In the cases where some common aliasing exists
>>> (e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
>>> I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
>>> to the bottom so that one takes precedence.
>>>
>>> Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>
>>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>>
>> Even with my ack already given I still think having %pE (or %pe) for
>> ints holding an error code is sensible. 
> 
> I don't understand why you'd want an explicit %p<something> to do what
> %p does by itself - in fact, with the current vsnprintf implementation,
> "%pe", ERR_PTR(-EFOO) would already do what you want (since after %p is
> processed, all alphanumeric are skipped whether they were interpreted or
> not). So we could "reserve" %pe perhaps in order to make the call sites
> a little more readable, but no code change in vsnprintf.c would be
> necessary.

Sorry, I meant I still consider %de (or %dE) sensible which I suggested
at the start of this thread.

> Or perhaps you meant introduce a %d<something> extension? I still think
> that's a bad idea, and I've in the meantime found another reason
> (covering %d in particular): Netdevices can be given a name containing
> exactly one occurrence of %d (or no % at all), and then the actual name
> will be determined based on that pattern. These patterns are settable
> from userspace. And everything of course breaks horribly if somebody set
> a name to "bla%deth" and that got turned into "blaEPERMth".

Sure, this should not happen. I don't see imminent danger though.
(ethernet IDs are usually positive, right?)

I think having a possibility to print error codes in an int is
beneficial, as otherwise I'd have to convert to a pointer first when
printing the code which is IMHO unnecessary burden.

>> So I wonder if it would be a
>> good idea to split this patch into one that introduces errcode() and
>> then the patch that teaches vsprintf about emitting its return value for
>> error valued pointers. Then I could rebase my initial patch for %pe on
>> top of your first one.
> 
> Well, I think my patch as-is is simple enough, there's not much point
> separating the few lines in vsnprintf() from the introduction of
> errcode() (which, realistically, will never have other callers).

Fine if your series goes in soon. If not I'd like to use errcode()
without having to discuss the changes to how pointers are printed.

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

It has been suggested several times to extend vsnprintf() to be able
to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
another attempt. Rather than adding another %p extension, simply teach
plain %p to convert ERR_PTRs. While the primary use case is

  if (IS_ERR(foo)) {
    pr_err("Sorry, can't do that: %p\n", foo);
    return PTR_ERR(foo);
  }

it is also more helpful to get a symbolic error code (or, worst case,
a decimal number) in case an ERR_PTR is accidentally passed to some
%p<something>, rather than the (efault) that check_pointer() would
result in.

With my embedded hat on, I've made it possible to remove this.

I've tested that the #ifdeffery in errcode.c is sufficient to make
this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
0day bot will tell me which ones I've missed.

The symbols to include have been found by massaging the output of

  find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'

In the cases where some common aliasing exists
(e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
to the bottom so that one takes precedence.

Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---

Andrew: please consider picking this up, even if we're already in the
merge window. Quite a few people have said they'd like to see
something like this, it's a debug improvement in its own right (the
"ERR_PTR accidentally passed to printf" case), the printf tests pass,
and it's much easier to start adding (and testing) users around the
tree once this is in master.

v3:
- only accept positive errno values in errcode()
- change type of err variable in pointer() from long to int
v2:
- add #include <linux/stddef.h> to errcode.h (0day)
- keep 'x' handling in switch() (Andy)
- add paragraph to Documentation/core-api/printk-formats.rst
- add ack from Uwe

 Documentation/core-api/printk-formats.rst |   8 +
 include/linux/errcode.h                   |  16 ++
 lib/Kconfig.debug                         |   8 +
 lib/Makefile                              |   1 +
 lib/errcode.c                             | 212 ++++++++++++++++++++++
 lib/test_printf.c                         |  14 ++
 lib/vsprintf.c                            |  26 +++
 7 files changed, 285 insertions(+)
 create mode 100644 include/linux/errcode.h
 create mode 100644 lib/errcode.c

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index c6224d039bcb..7d3bf3cb207b 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -66,6 +66,14 @@ might be printed instead of the unreachable information::
 	(efault) data on invalid address
 	(einval) invalid data on a valid address
 
+Error pointers, i.e. pointers for which IS_ERR() is true, are handled
+as follows: If CONFIG_SYMBOLIC_ERRCODE is set, an appropriate symbolic
+constant is printed. For example, '"%p", PTR_ERR(-ENOSPC)' results in
+"ENOSPC", while '"%p", PTR_ERR(-EWOULDBLOCK)' results in "EAGAIN"
+(since EAGAIN == EWOULDBLOCK, and the former is the most common). If
+CONFIG_SYMBOLIC_ERRCODE is not set, ERR_PTRs are printed as their
+decimal representation ("-28" and "-11" for the two examples).
+
 Plain Pointers
 --------------
 
diff --git a/include/linux/errcode.h b/include/linux/errcode.h
new file mode 100644
index 000000000000..c6a4c1b04f9c
--- /dev/null
+++ b/include/linux/errcode.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_ERRCODE_H
+#define _LINUX_ERRCODE_H
+
+#include <linux/stddef.h>
+
+#ifdef CONFIG_SYMBOLIC_ERRCODE
+const char *errcode(int err);
+#else
+static inline const char *errcode(int err)
+{
+	return NULL;
+}
+#endif
+
+#endif /* _LINUX_ERRCODE_H */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5960e2980a8a..dc1b20872774 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -164,6 +164,14 @@ config DYNAMIC_DEBUG
 	  See Documentation/admin-guide/dynamic-debug-howto.rst for additional
 	  information.
 
+config SYMBOLIC_ERRCODE
+	bool "Support symbolic error codes in printf"
+	help
+	  If you say Y here, the kernel's printf implementation will
+	  be able to print symbolic errors such as ENOSPC instead of
+	  the number 28. It makes the kernel image slightly larger
+	  (about 3KB), but can make the kernel logs easier to read.
+
 endmenu # "printk and dmesg options"
 
 menu "Compile-time checks and compiler options"
diff --git a/lib/Makefile b/lib/Makefile
index c5892807e06f..9f14edc7ef63 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -181,6 +181,7 @@ lib-$(CONFIG_GENERIC_BUG) += bug.o
 obj-$(CONFIG_HAVE_ARCH_TRACEHOOK) += syscall.o
 
 obj-$(CONFIG_DYNAMIC_DEBUG) += dynamic_debug.o
+obj-$(CONFIG_SYMBOLIC_ERRCODE) += errcode.o
 
 obj-$(CONFIG_NLATTR) += nlattr.o
 
diff --git a/lib/errcode.c b/lib/errcode.c
new file mode 100644
index 000000000000..3e519b13245e
--- /dev/null
+++ b/lib/errcode.c
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/build_bug.h>
+#include <linux/errno.h>
+#include <linux/errcode.h>
+#include <linux/kernel.h>
+
+/*
+ * Ensure these tables to not accidentally become gigantic if some
+ * huge errno makes it in. On most architectures, the first table will
+ * only have about 140 entries, but mips and parisc have more sparsely
+ * 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.
+ */
+#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
+static const char *codes_0[] = {
+	E(E2BIG),
+	E(EACCES),
+	E(EADDRINUSE),
+	E(EADDRNOTAVAIL),
+	E(EADV),
+	E(EAFNOSUPPORT),
+	E(EALREADY),
+	E(EBADE),
+	E(EBADF),
+	E(EBADFD),
+	E(EBADMSG),
+	E(EBADR),
+	E(EBADRQC),
+	E(EBADSLT),
+	E(EBFONT),
+	E(EBUSY),
+#ifdef ECANCELLED
+	E(ECANCELLED),
+#endif
+	E(ECHILD),
+	E(ECHRNG),
+	E(ECOMM),
+	E(ECONNABORTED),
+	E(ECONNRESET),
+	E(EDEADLOCK),
+	E(EDESTADDRREQ),
+	E(EDOM),
+	E(EDOTDOT),
+#ifndef CONFIG_MIPS
+	E(EDQUOT),
+#endif
+	E(EEXIST),
+	E(EFAULT),
+	E(EFBIG),
+	E(EHOSTDOWN),
+	E(EHOSTUNREACH),
+	E(EHWPOISON),
+	E(EIDRM),
+	E(EILSEQ),
+#ifdef EINIT
+	E(EINIT),
+#endif
+	E(EINPROGRESS),
+	E(EINTR),
+	E(EINVAL),
+	E(EIO),
+	E(EISCONN),
+	E(EISDIR),
+	E(EISNAM),
+	E(EKEYEXPIRED),
+	E(EKEYREJECTED),
+	E(EKEYREVOKED),
+	E(EL2HLT),
+	E(EL2NSYNC),
+	E(EL3HLT),
+	E(EL3RST),
+	E(ELIBACC),
+	E(ELIBBAD),
+	E(ELIBEXEC),
+	E(ELIBMAX),
+	E(ELIBSCN),
+	E(ELNRNG),
+	E(ELOOP),
+	E(EMEDIUMTYPE),
+	E(EMFILE),
+	E(EMLINK),
+	E(EMSGSIZE),
+	E(EMULTIHOP),
+	E(ENAMETOOLONG),
+	E(ENAVAIL),
+	E(ENETDOWN),
+	E(ENETRESET),
+	E(ENETUNREACH),
+	E(ENFILE),
+	E(ENOANO),
+	E(ENOBUFS),
+	E(ENOCSI),
+	E(ENODATA),
+	E(ENODEV),
+	E(ENOENT),
+	E(ENOEXEC),
+	E(ENOKEY),
+	E(ENOLCK),
+	E(ENOLINK),
+	E(ENOMEDIUM),
+	E(ENOMEM),
+	E(ENOMSG),
+	E(ENONET),
+	E(ENOPKG),
+	E(ENOPROTOOPT),
+	E(ENOSPC),
+	E(ENOSR),
+	E(ENOSTR),
+#ifdef ENOSYM
+	E(ENOSYM),
+#endif
+	E(ENOSYS),
+	E(ENOTBLK),
+	E(ENOTCONN),
+	E(ENOTDIR),
+	E(ENOTEMPTY),
+	E(ENOTNAM),
+	E(ENOTRECOVERABLE),
+	E(ENOTSOCK),
+	E(ENOTTY),
+	E(ENOTUNIQ),
+	E(ENXIO),
+	E(EOPNOTSUPP),
+	E(EOVERFLOW),
+	E(EOWNERDEAD),
+	E(EPERM),
+	E(EPFNOSUPPORT),
+	E(EPIPE),
+#ifdef EPROCLIM
+	E(EPROCLIM),
+#endif
+	E(EPROTO),
+	E(EPROTONOSUPPORT),
+	E(EPROTOTYPE),
+	E(ERANGE),
+	E(EREMCHG),
+#ifdef EREMDEV
+	E(EREMDEV),
+#endif
+	E(EREMOTE),
+	E(EREMOTEIO),
+#ifdef EREMOTERELEASE
+	E(EREMOTERELEASE),
+#endif
+	E(ERESTART),
+	E(ERFKILL),
+	E(EROFS),
+#ifdef ERREMOTE
+	E(ERREMOTE),
+#endif
+	E(ESHUTDOWN),
+	E(ESOCKTNOSUPPORT),
+	E(ESPIPE),
+	E(ESRCH),
+	E(ESRMNT),
+	E(ESTALE),
+	E(ESTRPIPE),
+	E(ETIME),
+	E(ETIMEDOUT),
+	E(ETOOMANYREFS),
+	E(ETXTBSY),
+	E(EUCLEAN),
+	E(EUNATCH),
+	E(EUSERS),
+	E(EXDEV),
+	E(EXFULL),
+
+	E(ECANCELED),
+	E(EAGAIN), /* EWOULDBLOCK */
+	E(ECONNREFUSED), /* EREFUSED */
+	E(EDEADLK), /* EDEADLK */
+};
+#undef E
+
+#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err
+static const char *codes_512[] = {
+	E(ERESTARTSYS),
+	E(ERESTARTNOINTR),
+	E(ERESTARTNOHAND),
+	E(ENOIOCTLCMD),
+	E(ERESTART_RESTARTBLOCK),
+	E(EPROBE_DEFER),
+	E(EOPENSTALE),
+	E(ENOPARAM),
+
+	E(EBADHANDLE),
+	E(ENOTSYNC),
+	E(EBADCOOKIE),
+	E(ENOTSUPP),
+	E(ETOOSMALL),
+	E(ESERVERFAULT),
+	E(EBADTYPE),
+	E(EJUKEBOX),
+	E(EIOCBQUEUED),
+	E(ERECALLCONFLICT),
+};
+#undef E
+
+const char *errcode(int err)
+{
+	if (err <= 0)
+		return NULL;
+	if (err < ARRAY_SIZE(codes_0))
+		return codes_0[err];
+	if (err >= 512 && err - 512 < ARRAY_SIZE(codes_512))
+		return codes_512[err - 512];
+	/* But why? */
+	if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */
+		return "EDQUOT";
+	return NULL;
+}
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 944eb50f3862..0401a2341245 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -212,6 +212,7 @@ test_string(void)
 #define PTR_STR "ffff0123456789ab"
 #define PTR_VAL_NO_CRNG "(____ptrval____)"
 #define ZEROS "00000000"	/* hex 32 zero bits */
+#define FFFFS "ffffffff"
 
 static int __init
 plain_format(void)
@@ -243,6 +244,7 @@ plain_format(void)
 #define PTR_STR "456789ab"
 #define PTR_VAL_NO_CRNG "(ptrval)"
 #define ZEROS ""
+#define FFFFS ""
 
 static int __init
 plain_format(void)
@@ -588,6 +590,17 @@ flags(void)
 	kfree(cmp_buffer);
 }
 
+static void __init
+errptr(void)
+{
+	test("-1234", "%p", ERR_PTR(-1234));
+	test(FFFFS "ffffffff " FFFFS "ffffff00", "%px %px", ERR_PTR(-1), ERR_PTR(-256));
+#ifdef CONFIG_SYMBOLIC_ERRCODE
+	test("EIO EINVAL ENOSPC", "%p %p %p", ERR_PTR(-EIO), ERR_PTR(-EINVAL), ERR_PTR(-ENOSPC));
+	test("EAGAIN EAGAIN", "%p %p", ERR_PTR(-EAGAIN), ERR_PTR(-EWOULDBLOCK));
+#endif
+}
+
 static void __init
 test_pointer(void)
 {
@@ -610,6 +623,7 @@ test_pointer(void)
 	bitmap();
 	netdev_features();
 	flags();
+	errptr();
 }
 
 static void __init selftest(void)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b0967cf17137..299fce317eb3 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -21,6 +21,7 @@
 #include <linux/build_bug.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/errcode.h>
 #include <linux/module.h>	/* for KSYM_SYMBOL_LEN */
 #include <linux/types.h>
 #include <linux/string.h>
@@ -2111,6 +2112,31 @@ static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
+	/*
+	 * If it's an ERR_PTR, try to print its symbolic
+	 * representation, except for %px, where the user explicitly
+	 * wanted the pointer formatted as a hex value.
+	 */
+	if (IS_ERR(ptr) && *fmt != 'x') {
+		int err = PTR_ERR(ptr);
+		const char *sym = errcode(-err);
+		if (sym)
+			return string_nocheck(buf, end, sym, spec);
+		/*
+		 * Funky, somebody passed ERR_PTR(-1234) or some other
+		 * non-existing Efoo - or more likely
+		 * CONFIG_SYMBOLIC_ERRCODE=n. None of the
+		 * %p<something> extensions can make any sense of an
+		 * ERR_PTR(), and if this was just a plain %p, the
+		 * user is still better off getting the decimal
+		 * representation rather than the hash value that
+		 * ptr_to_id() would generate.
+		 */
+		spec.flags |= SIGN;
+		spec.base = 10;
+		return number(buf, end, err, spec);
+	}
+
 	switch (*fmt) {
 	case 'F':
 	case 'f':
-- 
2.20.1

Re: [PATCH v3] printf: add support for printing symbolic error codes

[Petr Mladek]

First, I am sorry that I replay so late. I was traveling the previous
two weeks and was not able to follow the discussion about this patch.

I am fine with adding this feature. But I would like to do it
a cleaner way, see below.


On Tue 2019-09-17 08:59:59, Rasmus Villemoes wrote:
> With my embedded hat on, I've made it possible to remove this.
> 
> I've tested that the #ifdeffery in errcode.c is sufficient to make
> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
> 0day bot will tell me which ones I've missed.

Please, remove the above two paragraphs in the final patch. They make
sense for review but not for git history.


> Andrew: please consider picking this up, even if we're already in the
> merge window. Quite a few people have said they'd like to see
> something like this, it's a debug improvement in its own right (the
> "ERR_PTR accidentally passed to printf" case), the printf tests pass,
> and it's much easier to start adding (and testing) users around the
> tree once this is in master.

This change would deserve to spend some time in linux-next. Anyway,
it is already too late for 5.4.


> diff --git a/include/linux/errcode.h b/include/linux/errcode.h
> new file mode 100644
> index 000000000000..c6a4c1b04f9c
> --- /dev/null
> +++ b/include/linux/errcode.h

The word "code" is quite ambiguous. I am not sure if it is the name or
the number. I think that it is actually both (the relation between
the two.

Both "man 3 errno" and
https://www.gnu.org/software/libc/manual/html_node/Checking-for-Errors.html#Checking-for-Errors
talks about numbers and symbolic names.

Please use errname or so instead of errcode everywhere.


> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 5960e2980a8a..dc1b20872774 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -164,6 +164,14 @@ config DYNAMIC_DEBUG
>  	  See Documentation/admin-guide/dynamic-debug-howto.rst for additional
>  	  information.
>  
> +config SYMBOLIC_ERRCODE

What is the exact reason to make this configurable, please?

Nobody was really against this feature. The only question
was if it was worth the code complexity and maintenance.
If we are going to have the code then we should use it.

Then there was a concerns that it might be too big for embedded
people. But did it come from people working on embedded kernel
or was it just theoretical?

I would personally enable it when CONFIG_PRINTK is enabled.
We could always introduce a new config option later when
anyone really wants to disable it.

> --- /dev/null
> +++ b/lib/errcode.c
> @@ -0,0 +1,212 @@
> +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
> +static const char *codes_0[] = {
> +	E(E2BIG),

I really like the way how the array is initialized.


> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 944eb50f3862..0401a2341245 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> +static void __init
> +errptr(void)
> +{
> +	test("-1234", "%p", ERR_PTR(-1234));
> +	test(FFFFS "ffffffff " FFFFS "ffffff00", "%px %px", ERR_PTR(-1), ERR_PTR(-256));
> +#ifdef CONFIG_SYMBOLIC_ERRCODE
> +	test("EIO EINVAL ENOSPC", "%p %p %p", ERR_PTR(-EIO), ERR_PTR(-EINVAL), ERR_PTR(-ENOSPC));
> +	test("EAGAIN EAGAIN", "%p %p", ERR_PTR(-EAGAIN), ERR_PTR(-EWOULDBLOCK));

I like that you check more values. But please split it to check
only one value per line to make it better readable.


> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index b0967cf17137..299fce317eb3 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2111,6 +2112,31 @@ static noinline_for_stack
>  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  	      struct printf_spec spec)
>  {
> +	/*
> +	 * If it's an ERR_PTR, try to print its symbolic
> +	 * representation, except for %px, where the user explicitly
> +	 * wanted the pointer formatted as a hex value.
> +	 */
> +	if (IS_ERR(ptr) && *fmt != 'x') {

We had similar code before the commit 3e5903eb9cff70730171 ("vsprintf:
Prevent crash when dereferencing invalid pointers"). Note that the
original code kept the original value also for *fmt == 'K'.

Anyway, the above commit tried to unify the handling of special
values:

   + use the same strings for special values
   + check for special values only when pointer is dereferenced

This patch makes it inconsistent again. I mean that the code will:

   + check for (null) and (efault) only when the pointer is
     dereferenced

   + check for err codes in more situations but not in all
     and not in %s

   + use another style to print the error (uppercase without
      brackets)


I would like to keep it consistent. My proposal is:

1. Print the plain error code name only when
   a new %pe modifier is used. This will be useful
   in the error messages, e.g.

	void *p = ERR_PTR(-ENOMEM);
	if (IS_ERR(foo)) {
		pr_err("Sorry, can't do that: %pe\n", foo);
	return PTR_ERR(foo);

   would produce

	Sorry, can't do that: -ENOMEM


2. Use error code names also in check_pointer_msg() instead
   of (efault). But put it into brackets to distinguish it
   from the expected value, for example:

      /* valid pointer */
      phys_addr_t addr = 0xab;
      printk("value: %pa\n", &addr);
      /* known error code */
      printk("value: %pa\n", ERR_PTR(-ENOMEM));
      /* unknown error code */
      printk("value: %pa\n", ERR_PTR(-1234));

   would produce:

     value: 0xab
     value: (-ENOMEM)
     value: (-1234)


3. Unify the style for the null pointer:

    + use (NULL) instead of (null)

4. Do not use error code names for internal vsprintf error
   to avoid confusion. For example:

    + replace the one (einval) with (%piS-err) or so

How does that sound, please?

Best Regards,
Petr

Re: [PATCH v3] printf: add support for printing symbolic error codes

[Rasmus Villemoes]

On 25/09/2019 16.36, Petr Mladek wrote:
> First, I am sorry that I replay so late. I was traveling the previous
> two weeks and was not able to follow the discussion about this patch.
> 
> I am fine with adding this feature. But I would like to do it
> a cleaner way, see below.
> 
> 
> On Tue 2019-09-17 08:59:59, Rasmus Villemoes wrote:
>> With my embedded hat on, I've made it possible to remove this.
>>
>> I've tested that the #ifdeffery in errcode.c is sufficient to make
>> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
>> 0day bot will tell me which ones I've missed.
> 
> Please, remove the above two paragraphs in the final patch. They make
> sense for review but not for git history.

Agree for the latter, but not the former - I do want to explain why it's
possible to configure out; see also below.

> This change would deserve to spend some time in linux-next. Anyway,
> it is already too late for 5.4.

Yes, it's of course way too late now. Perhaps I should ask you to take
it via the printk tree? Anyway, let's see what we can agree to.

> The word "code" is quite ambiguous. I am not sure if it is the name or
> the number. I think that it is actually both (the relation between
> the two.
> 
> Both "man 3 errno" and
> https://www.gnu.org/software/libc/manual/html_node/Checking-for-Errors.html#Checking-for-Errors
> talks about numbers and symbolic names.
> 
> Please use errname or so instead of errcode everywhere.

OK. I wasn't too happy about errcode anyway - but I wanted to avoid
"str" being in there to avoid anyone thinking it was a strerror(). So
"CONFIG_SYMBOLIC_ERRNAME" and errname() seems fine with the above
justification.

>> +config SYMBOLIC_ERRCODE
> 
> What is the exact reason to make this configurable, please?
> 
> Nobody was really against this feature. The only question
> was if it was worth the code complexity and maintenance.
> If we are going to have the code then we should use it.
> 
> Then there was a concerns that it might be too big for embedded
> people. But did it come from people working on embedded kernel
> or was it just theoretical?

I am one such person, and while 3K may not be a lot, death by a thousand
paper cuts...

> I would personally enable it when CONFIG_PRINTK is enabled.

Agree. So let's make it 'default y if PRINTK'? These are only/mostly
useful when destined for dmesg, I can't imagine any of the sysfs/procfs
uses of vsprintf() to want this. So if somebody has gone to the rather
extremely length of disabling printk (which even I rarely do), they
really want the absolute minimal kernel, and would not benefit from this
anyway. While for the common case, it gets enabled for anyone that just
updates their defconfig and accepts new default values.

> We could always introduce a new config option later when
> anyone really wants to disable it.

No, because by the time the kernel has grown too large for some target,
it's almost impossible to start figuring out which small pieces can be
chopped away with suitable config options, and even harder to get
upstream to accept such configurability ("why, that would only gain you
3K...").

>> --- /dev/null
>> +++ b/lib/errcode.c
>> @@ -0,0 +1,212 @@
>> +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
>> +static const char *codes_0[] = {
>> +	E(E2BIG),
> 
> I really like the way how the array is initialized.

Thanks.

> 
>> diff --git a/lib/test_printf.c b/lib/test_printf.c
>> index 944eb50f3862..0401a2341245 100644
>> --- a/lib/test_printf.c
>> +++ b/lib/test_printf.c
>> +static void __init
>> +errptr(void)
>> +{
>> +	test("-1234", "%p", ERR_PTR(-1234));
>> +	test(FFFFS "ffffffff " FFFFS "ffffff00", "%px %px", ERR_PTR(-1), ERR_PTR(-256));
>> +#ifdef CONFIG_SYMBOLIC_ERRCODE
>> +	test("EIO EINVAL ENOSPC", "%p %p %p", ERR_PTR(-EIO), ERR_PTR(-EINVAL), ERR_PTR(-ENOSPC));
>> +	test("EAGAIN EAGAIN", "%p %p", ERR_PTR(-EAGAIN), ERR_PTR(-EWOULDBLOCK));
> 
> I like that you check more values. But please split it to check
> only one value per line to make it better readable.

Hm, ok, but I actually do it this way on purpose - I want to ensure that
the test where one passes a random not-zero-but-too-small buffer size
sometimes hits in the middle of the %p output, sometimes just before and
sometimes just after. The very reason I added test_printf was because
there were some %p extensions that violated the basic rule of
snprintf()'s return value and/or wrote beyond the provided buffer.

Not a big deal, so if you insist I'll break it up.

> 
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index b0967cf17137..299fce317eb3 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -2111,6 +2112,31 @@ static noinline_for_stack
>>  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>>  	      struct printf_spec spec)
>>  {
>> +	/*
>> +	 * If it's an ERR_PTR, try to print its symbolic
>> +	 * representation, except for %px, where the user explicitly
>> +	 * wanted the pointer formatted as a hex value.
>> +	 */
>> +	if (IS_ERR(ptr) && *fmt != 'x') {
> 
> We had similar code before the commit 3e5903eb9cff70730171 ("vsprintf:
> Prevent crash when dereferencing invalid pointers"). Note that the
> original code kept the original value also for *fmt == 'K'.
> 
> Anyway, the above commit tried to unify the handling of special
> values:
> 
>    + use the same strings for special values
>    + check for special values only when pointer is dereferenced
> 
> This patch makes it inconsistent again. I mean that the code will:
> 
>    + check for (null) and (efault) only when the pointer is
>      dereferenced
> 
>    + check for err codes in more situations but not in all
>      and not in %s
>
>    + use another style to print the error (uppercase without
>       brackets)
> 
> 
> I would like to keep it consistent. My proposal is:
> 
> 1. Print the plain error code name only when
>    a new %pe modifier is used. This will be useful
>    in the error messages, e.g.
> 
> 	void *p = ERR_PTR(-ENOMEM);
> 	if (IS_ERR(foo)) {
> 		pr_err("Sorry, can't do that: %pe\n", foo);
> 	return PTR_ERR(foo);
> 
>    would produce
> 
> 	Sorry, can't do that: -ENOMEM

Well, we can certainly do that. However, I didn't want that for two
reasons: (1) I want plain %p to be more useful when passed an ERR_PTR.
Printing the value, possibly symbolically, doesn't leak anything about
kernel addresses, so the hashing is pointless and just makes the
printk() less useful - and non-repeatable across reboots, making
debugging needlessly harder. (2) With a dedicated extension, we have to
define what happens if a non-ERR_PTR gets passed as %pe argument. [and
(3), we're running out of %p<foo> namespace].

So, if you have some good answer to (2) I can do that - but if the
answer is "fall through to handling it as just a normal %p", well, then
we haven't really won much. And I don't see what else we could do -
print "(!ERR_PTR)"?

> 2. Use error code names also in check_pointer_msg() instead
>    of (efault). But put it into brackets to distinguish it
>    from the expected value, for example:
> 
>       /* valid pointer */
>       phys_addr_t addr = 0xab;
>       printk("value: %pa\n", &addr);
>       /* known error code */
>       printk("value: %pa\n", ERR_PTR(-ENOMEM));
>       /* unknown error code */
>       printk("value: %pa\n", ERR_PTR(-1234));
> 
>    would produce:
> 
>      value: 0xab
>      value: (-ENOMEM)
>      value: (-1234)

Yes, I think this is a good idea. But only for ERR_PTRs, for other
obviously-not-a-kernel-address pointer values (i.e. the < PAGE_SIZE
case) we still need some other string.

So how about I try to add something like this so that
would-be-dereferenced ERR_PTRs get printed symbolically in brackets,
while I move the check for IS_ERR() to after the switch() (i.e. before
handing over to do the hashing)? Then all ERR_PTRs get printed
symbolically - except for %px and possibly %pK which are explicitly
"print this value".

> 3. Unify the style for the null pointer:
> 
>     + use (NULL) instead of (null)

Yes, that's better. But somewhat out of scope for this patch.

> 4. Do not use error code names for internal vsprintf error
>    to avoid confusion. For example:
> 
>     + replace the one (einval) with (%piS-err) or so
> 
> How does that sound, please?

Oh, yes, I never was a fan of the (einval) (efault) strings.

Rasmus

Re: [PATCH v3] printf: add support for printing symbolic error codes

[Petr Mladek]

On Sun 2019-09-29 22:09:28, Rasmus Villemoes wrote:
> > On Tue 2019-09-17 08:59:59, Rasmus Villemoes wrote:
> >> With my embedded hat on, I've made it possible to remove this.
> >>
> >> I've tested that the #ifdeffery in errcode.c is sufficient to make
> >> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
> >> 0day bot will tell me which ones I've missed.
> > 
> > Please, remove the above two paragraphs in the final patch. They make
> > sense for review but not for git history.
>
> Agree for the latter, but not the former - I do want to explain why it's
> possible to configure out; see also below.

I see. It was too cryptic for me. I did not get the proper meaning
and context ;-)

> > This change would deserve to spend some time in linux-next. Anyway,
> > it is already too late for 5.4.
> 
> Yes, it's of course way too late now. Perhaps I should ask you to take
> it via the printk tree? Anyway, let's see what we can agree to.

Yup, I could take it via printk.git.

> >> +config SYMBOLIC_ERRCODE
> > 
> > What is the exact reason to make this configurable, please?
> 
> I am one such person, and while 3K may not be a lot, death by a thousand
> paper cuts...
> 
> > I would personally enable it when CONFIG_PRINTK is enabled.
> 
> Agree. So let's make it 'default y if PRINTK'?

Yeah, it makes perfect sense to me.

> > We could always introduce a new config option later when
> > anyone really wants to disable it.
> 
> No, because by the time the kernel has grown too large for some target,
> it's almost impossible to start figuring out which small pieces can be
> chopped away with suitable config options

OK, if you are the embedded guy and you would appreciate the config
then let's have it. Just please add it by a separate patch,
ideally with some numbers.

> and even harder to get
> upstream to accept such configurability ("why, that would only gain you
> 3K...").

I remeber LWN articles about shrinking the kernel for embeded systems
and that every kB was important.


> >> --- /dev/null
> >> +++ b/lib/errcode.c
> >> @@ -0,0 +1,212 @@
> >> +#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = #err
> >> +static const char *codes_0[] = {
> >> +	E(E2BIG),
> > 
> > I really like the way how the array is initialized.
> 
> Thanks.
> 
> > 
> >> diff --git a/lib/test_printf.c b/lib/test_printf.c
> >> index 944eb50f3862..0401a2341245 100644
> >> --- a/lib/test_printf.c
> >> +++ b/lib/test_printf.c
> >> +static void __init
> >> +errptr(void)
> >> +{
> >> +	test("-1234", "%p", ERR_PTR(-1234));
> >> +	test(FFFFS "ffffffff " FFFFS "ffffff00", "%px %px", ERR_PTR(-1), ERR_PTR(-256));
> >> +#ifdef CONFIG_SYMBOLIC_ERRCODE
> >> +	test("EIO EINVAL ENOSPC", "%p %p %p", ERR_PTR(-EIO), ERR_PTR(-EINVAL), ERR_PTR(-ENOSPC));
> >> +	test("EAGAIN EAGAIN", "%p %p", ERR_PTR(-EAGAIN), ERR_PTR(-EWOULDBLOCK));
> > 
> > I like that you check more values. But please split it to check
> > only one value per line to make it better readable.
> 
> Hm, ok, but I actually do it this way on purpose - I want to ensure that
> the test where one passes a random not-zero-but-too-small buffer size
> sometimes hits in the middle of the %p output, sometimes just before and
> sometimes just after.

Is this really tested? do_test() function uses buffer for 256 characters.
There are some consistency checks. But any of your test string
is not truncated by the size of the buffer. Do I miss anything, please?

> The very reason I added test_printf was because
> there were some %p extensions that violated the basic rule of
> snprintf()'s return value and/or wrote beyond the provided buffer.

This sounds like a serious bug. Are your aware of any still
existing %p extension that causes this?

> Not a big deal, so if you insist I'll break it up.

Yes, it is not a big deal. But I would still prefer to
understand what is tested. And it is better to have
more tests focused on different aspects than a single
magic one.


> > 
> >> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> >> index b0967cf17137..299fce317eb3 100644
> >> --- a/lib/vsprintf.c
> >> +++ b/lib/vsprintf.c
> >> @@ -2111,6 +2112,31 @@ static noinline_for_stack
> >>  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> >>  	      struct printf_spec spec)
> >>  {
> >> +	/*
> >> +	 * If it's an ERR_PTR, try to print its symbolic
> >> +	 * representation, except for %px, where the user explicitly
> >> +	 * wanted the pointer formatted as a hex value.
> >> +	 */
> >> +	if (IS_ERR(ptr) && *fmt != 'x') {
> > 
> > We had similar code before the commit 3e5903eb9cff70730171 ("vsprintf:
> > Prevent crash when dereferencing invalid pointers"). Note that the
> > original code kept the original value also for *fmt == 'K'.
> > 
> > Anyway, the above commit tried to unify the handling of special
> > values:
> > 
> >    + use the same strings for special values
> >    + check for special values only when pointer is dereferenced
> > 
> > This patch makes it inconsistent again. I mean that the code will:
> > 
> >    + check for (null) and (efault) only when the pointer is
> >      dereferenced
> > 
> >    + check for err codes in more situations but not in all
> >      and not in %s
> >
> >    + use another style to print the error (uppercase without
> >       brackets)
> > 
> > 
> > I would like to keep it consistent. My proposal is:
> > 
> > 1. Print the plain error code name only when
> >    a new %pe modifier is used. This will be useful
> >    in the error messages, e.g.
> > 
> > 	void *p = ERR_PTR(-ENOMEM);
> > 	if (IS_ERR(foo)) {
> > 		pr_err("Sorry, can't do that: %pe\n", foo);
> > 	return PTR_ERR(foo);
> > 
> >    would produce
> > 
> > 	Sorry, can't do that: -ENOMEM
> 
> Well, we can certainly do that. However, I didn't want that for two
> reasons: (1) I want plain %p to be more useful when passed an ERR_PTR.
> Printing the value, possibly symbolically, doesn't leak anything about
> kernel addresses, so the hashing is pointless and just makes the
> printk() less useful - and non-repeatable across reboots, making
> debugging needlessly harder. (2) With a dedicated extension, we have to
> define what happens if a non-ERR_PTR gets passed as %pe argument. [and
> (3), we're running out of %p<foo> namespace].
>
> So, if you have some good answer to (2) I can do that - but if the
> answer is "fall through to handling it as just a normal %p", well, then
> we haven't really won much. And I don't see what else we could do -
> print "(!ERR_PTR)"?

This made me to remember the long discussion about filtering kernel
pointers, see
https://lkml.kernel.org/r/1506816410-10230-1-git-send-email-me@tobin.cc

It was basically about that %p might leak information. %pK failed
because it did not force people to remove the dangerous %p calls.
It ended with hashing %p to actually force people to convert %p
into something more useful and safe.

IMHO, the most extreme was a wish to get rid of %p and %pa
completely, see
https://lkml.kernel.org/r/CA+55aFwac2BzgZs-X1_VhekkuGfuLqNui2+2DbvLiDyptS-rXQ@mail.gmail.com

I do not think that exposing ERR_PTR values might be dangerous.
Well, it would be great to confirm this by some security guys.
Anyway, I do not think that it is a good idea to make %p
more useful again.

I would print the hashed pointer value when the value passed
to %pe is out of range.

Best Regards,
Petr

Re: [PATCH v3] printf: add support for printing symbolic error codes

[Andrew Morton]

On Tue, 17 Sep 2019 08:59:59 +0200 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> It has been suggested several times to extend vsnprintf() to be able
> to convert the numeric value of ENOSPC to print "ENOSPC". This is yet
> another attempt. Rather than adding another %p extension, simply teach
> plain %p to convert ERR_PTRs. While the primary use case is
> 
>   if (IS_ERR(foo)) {
>     pr_err("Sorry, can't do that: %p\n", foo);
>     return PTR_ERR(foo);
>   }
> 
> it is also more helpful to get a symbolic error code (or, worst case,
> a decimal number) in case an ERR_PTR is accidentally passed to some
> %p<something>, rather than the (efault) that check_pointer() would
> result in.
> 
> With my embedded hat on, I've made it possible to remove this.
> 
> I've tested that the #ifdeffery in errcode.c is sufficient to make
> this compile on arm, arm64, mips, powerpc, s390, x86 - I'm sure the
> 0day bot will tell me which ones I've missed.
> 
> The symbols to include have been found by massaging the output of
> 
>   find arch include -iname 'errno*.h' | xargs grep -E 'define\s*E'
> 
> In the cases where some common aliasing exists
> (e.g. EAGAIN=EWOULDBLOCK on all platforms, EDEADLOCK=EDEADLK on most),
> I've moved the more popular one (in terms of 'git grep -w Efoo | wc)
> to the bottom so that one takes precedence.

Looks reasonable to me.

Is there any existing kernel code which presently uses this?  Can we
get some conversions done to demonstrate and hopefully test the
feature?