* [PATCH] printf: add support for printing symbolic error codes @ 2019-08-30 21:46 Rasmus Villemoes 2019-08-30 21:53 ` Joe Perches ` (4 more replies) 0 siblings, 5 replies; 42+ messages in thread From: Rasmus Villemoes @ 2019-08-30 21:46 UTC (permalink / raw) To: Sergey Senozhatsky, Uwe Kleine-König, Petr Mladek Cc: Andrew Morton, Jani Nikula, Joe Perches, Juergen Gross, linux-kernel, 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. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- include/linux/errcode.h | 14 +++ lib/Kconfig.debug | 8 ++ lib/Makefile | 1 + lib/errcode.c | 215 ++++++++++++++++++++++++++++++++++++++++ lib/test_printf.c | 14 +++ lib/vsprintf.c | 28 +++++- 6 files changed, 278 insertions(+), 2 deletions(-) create mode 100644 include/linux/errcode.h create mode 100644 lib/errcode.c diff --git a/include/linux/errcode.h b/include/linux/errcode.h new file mode 100644 index 000000000000..9dc4cfaa37ab --- /dev/null +++ b/include/linux/errcode.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_ERRCODE_H +#define _LINUX_ERRCODE_H + +#ifdef CONFIG_SYMBOLIC_ERRCODE +const char *errcode(int code); +#else +static inline const char *errcode(int code) +{ + 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..4b67fc23f3f2 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) { + /* %px means the user explicitly wanted the pointer formatted as a hex value. */ + if (*fmt == 'x') + return pointer_string(buf, end, ptr, spec); + + /* If it's an ERR_PTR, try to print its symbolic representation. */ + if (IS_ERR(ptr)) { + 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': @@ -2178,8 +2204,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, return flags_string(buf, end, ptr, spec, fmt); case 'O': return kobject_string(buf, end, ptr, spec, fmt); - case 'x': - return pointer_string(buf, end, ptr, spec); } /* default is to _not_ leak addresses, hash before printing */ -- 2.20.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH] printf: add support for printing symbolic error codes 2019-08-30 21:46 [PATCH] printf: add support for printing symbolic error codes Rasmus Villemoes @ 2019-08-30 21:53 ` Joe Perches 2019-08-30 22:03 ` Rasmus Villemoes 2019-08-31 9:38 ` kbuild test robot ` (3 subsequent siblings) 4 siblings, 1 reply; 42+ messages in thread From: Joe Perches @ 2019-08-30 21:53 UTC (permalink / raw) To: Rasmus Villemoes, Sergey Senozhatsky, Uwe Kleine-König, Petr Mladek Cc: Andrew Morton, Jani Nikula, Juergen Gross, linux-kernel On Fri, 2019-08-30 at 23:46 +0200, 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 Thanks, this all this seems reasonable except for > diff --git a/lib/vsprintf.c b/lib/vsprintf.c [] > @@ -2178,8 +2204,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, > return flags_string(buf, end, ptr, spec, fmt); > case 'O': > return kobject_string(buf, end, ptr, spec, fmt); > - case 'x': > - return pointer_string(buf, end, ptr, spec); > } > > /* default is to _not_ leak addresses, hash before printing */ why remove this? ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] printf: add support for printing symbolic error codes 2019-08-30 21:53 ` Joe Perches @ 2019-08-30 22:03 ` Rasmus Villemoes 2019-08-30 22:21 ` Joe Perches 0 siblings, 1 reply; 42+ messages in thread From: Rasmus Villemoes @ 2019-08-30 22:03 UTC (permalink / raw) To: Joe Perches, Rasmus Villemoes, Sergey Senozhatsky, Uwe Kleine-König, Petr Mladek Cc: Andrew Morton, Jani Nikula, Juergen Gross, linux-kernel On 30/08/2019 23.53, Joe Perches wrote: > >> diff --git a/lib/vsprintf.c b/lib/vsprintf.c > [] >> @@ -2178,8 +2204,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, >> return flags_string(buf, end, ptr, spec, fmt); >> case 'O': >> return kobject_string(buf, end, ptr, spec, fmt); >> - case 'x': >> - return pointer_string(buf, end, ptr, spec); >> } >> >> /* default is to _not_ leak addresses, hash before printing */ > > why remove this? > The handling of %px is moved above the test for ptr being an ERR_PTR, so that %px, ptr continues to be (roughly) equivalent to %08lx, (long)ptr. Rasmus ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] printf: add support for printing symbolic error codes 2019-08-30 22:03 ` Rasmus Villemoes @ 2019-08-30 22:21 ` Joe Perches 2019-08-30 22:50 ` Rasmus Villemoes 0 siblings, 1 reply; 42+ messages in thread From: Joe Perches @ 2019-08-30 22:21 UTC (permalink / raw) To: Rasmus Villemoes, Sergey Senozhatsky, Uwe Kleine-König, Petr Mladek Cc: Andrew Morton, Jani Nikula, Juergen Gross, linux-kernel On Sat, 2019-08-31 at 00:03 +0200, Rasmus Villemoes wrote: > On 30/08/2019 23.53, Joe Perches wrote: > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > > [] > > > @@ -2178,8 +2204,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, > > > return flags_string(buf, end, ptr, spec, fmt); > > > case 'O': > > > return kobject_string(buf, end, ptr, spec, fmt); > > > - case 'x': > > > - return pointer_string(buf, end, ptr, spec); > > > } > > > > > > /* default is to _not_ leak addresses, hash before printing */ > > > > why remove this? > > > > The handling of %px is moved above the test for ptr being an ERR_PTR, so > that %px, ptr continues to be (roughly) equivalent to %08lx, (long)ptr. Ah. Pity the flow of the switch/case is disrupted. That now deserves a separate comment. But why not just extend check_pointer_msg? ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] printf: add support for printing symbolic error codes 2019-08-30 22:21 ` Joe Perches @ 2019-08-30 22:50 ` Rasmus Villemoes 2019-09-02 9:07 ` David Laight 0 siblings, 1 reply; 42+ messages in thread From: Rasmus Villemoes @ 2019-08-30 22:50 UTC (permalink / raw) To: Joe Perches, Sergey Senozhatsky, Uwe Kleine-König, Petr Mladek Cc: Andrew Morton, Jani Nikula, Juergen Gross, linux-kernel On 31/08/2019 00.21, Joe Perches wrote: > On Sat, 2019-08-31 at 00:03 +0200, Rasmus Villemoes wrote: >> On 30/08/2019 23.53, Joe Perches wrote: >>>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c >>> [] >>>> @@ -2178,8 +2204,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, >>>> return flags_string(buf, end, ptr, spec, fmt); >>>> case 'O': >>>> return kobject_string(buf, end, ptr, spec, fmt); >>>> - case 'x': >>>> - return pointer_string(buf, end, ptr, spec); >>>> } >>>> >>>> /* default is to _not_ leak addresses, hash before printing */ >>> >>> why remove this? >>> >> >> The handling of %px is moved above the test for ptr being an ERR_PTR, so >> that %px, ptr continues to be (roughly) equivalent to %08lx, (long)ptr. > > Ah. > Pity the flow of the switch/case is disrupted. Agree, but I don't think it's that bad. > That now deserves a separate comment. You mean a comment like /* %px means the user explicitly wanted the pointer formatted as a hex value. */. Or do you want (the|an additional) comment somewhere inside the switch()? > But why not just extend check_pointer_msg? Partly because that would rely on all %p<foo> actually eventually passing ptr through to that (notably plain %p does not), partly because the way check_pointer_msg works means that it has to return a string for its caller to print - which is ok when the errcode is found, but breaks if it needs to format a decimal. It can't even snprintf() to a stack buffer and return that, because, well, you can't do that, and it would be a silly recursive snprintf anyway. OK, so perhaps you meant check_pointer() where it might be doable, but again, with plain %p and possibly others we don't get to that. Rasmus ^ permalink raw reply [flat|nested] 42+ messages in thread
* RE: [PATCH] printf: add support for printing symbolic error codes 2019-08-30 22:50 ` Rasmus Villemoes @ 2019-09-02 9:07 ` David Laight 0 siblings, 0 replies; 42+ messages in thread From: David Laight @ 2019-09-02 9:07 UTC (permalink / raw) To: 'Rasmus Villemoes', Joe Perches, Sergey Senozhatsky, Uwe Kleine-König, Petr Mladek Cc: Andrew Morton, Jani Nikula, Juergen Gross, linux-kernel From: Rasmus Villemoes > Sent: 30 August 2019 23:51 ... > > But why not just extend check_pointer_msg? > > Partly because that would rely on all %p<foo> actually eventually > passing ptr through to that (notably plain %p does not), partly because > the way check_pointer_msg works means that it has to return a string for > its caller to print - which is ok when the errcode is found, but breaks > if it needs to format a decimal. It can't even snprintf() to a stack > buffer and return that, because, well, you can't do that, and it would > be a silly recursive snprintf anyway. Perhaps you could use NULL or "" to mean 'just print the value'. Then you might manage to use the test for NULL to print the errno strings. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] printf: add support for printing symbolic error codes 2019-08-30 21:46 [PATCH] printf: add support for printing symbolic error codes Rasmus Villemoes 2019-08-30 21:53 ` Joe Perches @ 2019-08-31 9:38 ` kbuild test robot 2019-09-02 15:29 ` Uwe Kleine-König ` (2 subsequent siblings) 4 siblings, 0 replies; 42+ messages in thread From: kbuild test robot @ 2019-08-31 9:38 UTC (permalink / raw) To: Rasmus Villemoes Cc: kbuild-all, Sergey Senozhatsky, Uwe =?unknown-8bit?Q?Kleine-K=C3=B6nig?=, Petr Mladek, Andrew Morton, Jani Nikula, Joe Perches, Juergen Gross, linux-kernel, Rasmus Villemoes [-- Attachment #1: Type: text/plain, Size: 1457 bytes --] Hi Rasmus, Thank you for the patch! Yet something to improve: [auto build test ERROR on linus/master] [cannot apply to v5.3-rc6 next-20190830] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Rasmus-Villemoes/printf-add-support-for-printing-symbolic-error-codes/20190831-162013 config: x86_64-randconfig-b002-201934 (attached as .config) compiler: gcc-7 (Debian 7.4.0-11) 7.4.0 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from <command-line>:0:0: include/linux/errcode.h: In function 'errcode': >> include/linux/errcode.h:10:9: error: 'NULL' undeclared (first use in this function) return NULL; ^~~~ include/linux/errcode.h:10:9: note: each undeclared identifier is reported only once for each function it appears in vim +/NULL +10 include/linux/errcode.h 4 5 #ifdef CONFIG_SYMBOLIC_ERRCODE 6 const char *errcode(int code); 7 #else 8 static inline const char *errcode(int code) 9 { > 10 return NULL; 11 } 12 #endif 13 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 37666 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] printf: add support for printing symbolic error codes 2019-08-30 21:46 [PATCH] printf: add support for printing symbolic error codes Rasmus Villemoes 2019-08-30 21:53 ` Joe Perches 2019-08-31 9:38 ` kbuild test robot @ 2019-09-02 15:29 ` Uwe Kleine-König 2019-09-04 9:13 ` Rasmus Villemoes 2019-09-04 16:19 ` Andy Shevchenko 2019-09-09 20:38 ` [PATCH v2] " Rasmus Villemoes 4 siblings, 1 reply; 42+ messages in thread From: Uwe Kleine-König @ 2019-09-02 15:29 UTC (permalink / raw) To: Rasmus Villemoes, Sergey Senozhatsky, Petr Mladek Cc: Andrew Morton, Jani Nikula, Joe Perches, Juergen Gross, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 1757 bytes --] Hello Rasmus, On 8/30/19 11:46 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. Your text suggests that only cases that formerly emitted "(efault)" are affected. I didn't check this but if this is the case, I like your patch. > With my embedded hat on, I've made it possible to remove this. I wonder if the config item should only be configurable if EXPERT is enabled. > 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. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> Apart from the above minor issues: Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org> Best regards Uwe [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] printf: add support for printing symbolic error codes 2019-09-02 15:29 ` Uwe Kleine-König @ 2019-09-04 9:13 ` Rasmus Villemoes 2019-09-04 9:21 ` Uwe Kleine-König 0 siblings, 1 reply; 42+ messages in thread From: Rasmus Villemoes @ 2019-09-04 9:13 UTC (permalink / raw) To: Uwe Kleine-König, Sergey Senozhatsky, Petr Mladek Cc: Andrew Morton, Jani Nikula, Joe Perches, Juergen Gross, linux-kernel On 02/09/2019 17.29, Uwe Kleine-König wrote: > Hello Rasmus, > > On 8/30/19 11:46 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. > > Your text suggests that only cases that formerly emitted "(efault)" are > affected. I didn't check this but if this is the case, I like your patch. Well, sort of. Depends on whether this is plain %p or %p<something>. In the former case, the pointer would have been treated as any other "valid" kernel pointer, hence passed through the ptr_to_id() and printed as the result of, roughly, siphash((long)ptr) - i.e. some hex number that has nothing directly to do with the -EIO that was passed in. Moreover, while the printed value would be the same for a given error code during a given boot, on the next boot, the hashing would use a different seed, so would result in another "random" hex value being printed - which one can easily imagine makes debugging harder. With my patch, these would always result in "EIO" (or its decimal representation) being printed. In the latter case, yes, all the %p extensions that would somehow dereference ptr would then be caught in the check_pointer() and instead of printing (efault), we'd (again) get the symbolic error code. In both cases, I see printing the symbolic errno as a clear improvement - completely independent of whether we somehow want to make it "officially allowed" to deliberately pass ERR_PTRs (and I see that I forgot to update Documentation). So while that is really the main point of the patch, IMO the patch can already be justified by the above. [A few of the %p extensions do not dereference ptr (e.g. the %pS aka print the symbol name) - I think they just print ptr as a hex value if no symbol is found (or !CONFIG_KALLSYMS). I can't imagine how an ERR_PTR would be passed to %pS, though, and again, getting the symbolic error (or the decimal -22) should be better than getting -22 printed as a hex string.] >> With my embedded hat on, I've made it possible to remove this. > > I wonder if the config item should only be configurable if EXPERT is > enabled. Maybe. Or one could make it default y and then only make it possible to deselect if CONFIG_EXPERT - only really space-constrained embedded devices would want this disabled, I think. But I prefer keeping it simple, i.e. keeping it as-is for now. > Apart from the above minor issues: > > Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org> Thanks. The buildbot apparently tried to compile the errcode.h header by itself and complained that NULL was not defined, so I'll respin to make it happy, and add a note to Documentation/. Can I include that ack even if I don't change the Kconfig logic? Thanks, Rasmus ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] printf: add support for printing symbolic error codes 2019-09-04 9:13 ` Rasmus Villemoes @ 2019-09-04 9:21 ` Uwe Kleine-König 0 siblings, 0 replies; 42+ messages in thread From: Uwe Kleine-König @ 2019-09-04 9:21 UTC (permalink / raw) To: Rasmus Villemoes, Sergey Senozhatsky, Petr Mladek Cc: Andrew Morton, Jani Nikula, Joe Perches, Juergen Gross, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 474 bytes --] On 9/4/19 11:13 AM, Rasmus Villemoes wrote: > On 02/09/2019 17.29, Uwe Kleine-König wrote: >> Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org> > > Thanks. The buildbot apparently tried to compile the errcode.h header by > itself and complained that NULL was not defined, so I'll respin to make > it happy, and add a note to Documentation/. Can I include that ack even > if I don't change the Kconfig logic? Yeah, feel free to keep it. Best regards Uwe [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] printf: add support for printing symbolic error codes 2019-08-30 21:46 [PATCH] printf: add support for printing symbolic error codes Rasmus Villemoes ` (2 preceding siblings ...) 2019-09-02 15:29 ` Uwe Kleine-König @ 2019-09-04 16:19 ` Andy Shevchenko 2019-09-04 16:28 ` Uwe Kleine-König 2019-09-09 20:38 ` [PATCH v2] " Rasmus Villemoes 4 siblings, 1 reply; 42+ messages in thread From: Andy Shevchenko @ 2019-09-04 16:19 UTC (permalink / raw) To: Rasmus Villemoes Cc: Sergey Senozhatsky, Uwe Kleine-König, Petr Mladek, Andrew Morton, Jani Nikula, Joe Perches, Juergen Gross, Linux Kernel Mailing List On Sat, Aug 31, 2019 at 12:48 AM 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. > +/* > + * 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 Hmm... Perhaps better to define the upper boundary with something like #define __E_POSIX_UPPER_BOUNDARY 300 // name sucks, I know > +#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err Similar to 550? > +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"; Another possibility is to initialize the errors at run time with radix tree. > + return NULL; > +} > @@ -2111,6 +2112,31 @@ static noinline_for_stack > char *pointer(const char *fmt, char *buf, char *end, void *ptr, > struct printf_spec spec) > { > + /* %px means the user explicitly wanted the pointer formatted as a hex value. */ > + if (*fmt == 'x') > + return pointer_string(buf, end, ptr, spec); But instead of breaking switch case apart can we use... > + > + /* If it's an ERR_PTR, try to print its symbolic representation. */ > + if (IS_ERR(ptr)) { ... if (IS_ERR() && *fmt != 'x') { here? > + 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); > + } -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] printf: add support for printing symbolic error codes 2019-09-04 16:19 ` Andy Shevchenko @ 2019-09-04 16:28 ` Uwe Kleine-König 2019-09-05 11:40 ` Rasmus Villemoes 0 siblings, 1 reply; 42+ messages in thread From: Uwe Kleine-König @ 2019-09-04 16:28 UTC (permalink / raw) To: Andy Shevchenko, Rasmus Villemoes Cc: Sergey Senozhatsky, Petr Mladek, Andrew Morton, Jani Nikula, Joe Perches, Juergen Gross, Linux Kernel Mailing List [-- Attachment #1.1: Type: text/plain, Size: 4116 bytes --] On 9/4/19 6:19 PM, Andy Shevchenko wrote: > On Sat, Aug 31, 2019 at 12:48 AM 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. > >> +/* >> + * 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 > > Hmm... Perhaps better to define the upper boundary with something like > > #define __E_POSIX_UPPER_BOUNDARY 300 // name sucks, I know > >> +#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err > > Similar to 550? I'd not add "POSIX" in the name. Given that the arrays are called codes_0 and codes_512 I don't think using plain numbers hurts much and choosing a good name is hard, so I suggest to keep the explicit numbers. >> +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"; > > Another possibility is to initialize the errors at run time with radix tree. The idea was to save space. But when using a radix tree this has overhead compared to the lists here, and you still need a map for error-code -> error-name to initialize the radix tree. Also a lookup is slower than with the idea implemented here. So it's bigger, slower and more complicated ... I don't think we should do that. > >> + return NULL; >> +} > >> @@ -2111,6 +2112,31 @@ static noinline_for_stack >> char *pointer(const char *fmt, char *buf, char *end, void *ptr, >> struct printf_spec spec) >> { >> + /* %px means the user explicitly wanted the pointer formatted as a hex value. */ >> + if (*fmt == 'x') >> + return pointer_string(buf, end, ptr, spec); > > But instead of breaking switch case apart can we use... > >> + >> + /* If it's an ERR_PTR, try to print its symbolic representation. */ >> + if (IS_ERR(ptr)) { > > ... if (IS_ERR() && *fmt != 'x') { > here? I don't feel strong here, works either way for me. Best regards Uwe [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] printf: add support for printing symbolic error codes 2019-09-04 16:28 ` Uwe Kleine-König @ 2019-09-05 11:40 ` Rasmus Villemoes 0 siblings, 0 replies; 42+ messages in thread From: Rasmus Villemoes @ 2019-09-05 11:40 UTC (permalink / raw) To: Uwe Kleine-König, Andy Shevchenko Cc: Sergey Senozhatsky, Petr Mladek, Andrew Morton, Jani Nikula, Joe Perches, Juergen Gross, Linux Kernel Mailing List On 04/09/2019 18.28, Uwe Kleine-König wrote: > On 9/4/19 6:19 PM, Andy Shevchenko wrote: >> On Sat, Aug 31, 2019 at 12:48 AM Rasmus Villemoes >> <linux@rasmusvillemoes.dk> wrote: >>> >> >>> +/* >>> + * 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 >> >> Hmm... Perhaps better to define the upper boundary with something like >> >> #define __E_POSIX_UPPER_BOUNDARY 300 // name sucks, I know >> >>> +#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = #err >> >> Similar to 550? > > I'd not add "POSIX" in the name. Given that the arrays are called > codes_0 and codes_512 I don't think using plain numbers hurts much and > choosing a good name is hard, so I suggest to keep the explicit numbers. I agree, adding random macro names for these essentially arbitrary (and one-time use) numbers doesn't make sense. Remember that the sizing of the arrays is done automatically by gcc. I suppose an alternative is to drop the BUILD_BUG_ON_ZEROs from the E() defines and then just have some static_assert(ARRAY_SIZE(codes_0) < 300) - but the advantage of the above is that one gets to know _which_ E* has a huge value (that is how I caught EDQUOT on MIPS). >>> +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"; >> >> Another possibility is to initialize the errors at run time with radix tree. > > The idea was to save space. But when using a radix tree this has > overhead compared to the lists here, and you still need a map for > error-code -> error-name to initialize the radix tree. Also a lookup is > slower than with the idea implemented here. So it's bigger, slower and > more complicated ... I don't think we should do that. Yes, a radix tree is unlikely to end up saving space at all. Moreover, any initialization at run-time means there's some window where we don't have them, and printk() should work as early as possible (and I really don't want to add any kind of synchronization "are we initialized yet", just see what that did to the pointer hashing). So I'll stick with the arrays. >>> @@ -2111,6 +2112,31 @@ static noinline_for_stack >>> char *pointer(const char *fmt, char *buf, char *end, void *ptr, >>> struct printf_spec spec) >>> { >>> + /* %px means the user explicitly wanted the pointer formatted as a hex value. */ >>> + if (*fmt == 'x') >>> + return pointer_string(buf, end, ptr, spec); >> >> But instead of breaking switch case apart can we use... >> >>> + >>> + /* If it's an ERR_PTR, try to print its symbolic representation. */ >>> + if (IS_ERR(ptr)) { >> >> ... if (IS_ERR() && *fmt != 'x') { >> here? This makes sense, I think I'll do it that way. Thanks. Rasmus ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2] printf: add support for printing symbolic error codes 2019-08-30 21:46 [PATCH] printf: add support for printing symbolic error codes Rasmus Villemoes ` (3 preceding siblings ...) 2019-09-04 16:19 ` Andy Shevchenko @ 2019-09-09 20:38 ` Rasmus Villemoes 2019-09-10 15:26 ` Andy Shevchenko ` (3 more replies) 4 siblings, 4 replies; 42+ messages in thread From: Rasmus Villemoes @ 2019-09-09 20:38 UTC (permalink / raw) To: Andrew Morton, Jonathan Corbet Cc: Rasmus Villemoes, Andy Shevchenko, Joe Perches, Petr Mladek, Sergey Senozhatsky, Jani Nikula, Linux Kernel Mailing List, Uwe Kleine-König, linux-doc 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v2] printf: add support for printing symbolic error codes 2019-09-09 20:38 ` [PATCH v2] " Rasmus Villemoes @ 2019-09-10 15:26 ` Andy Shevchenko 2019-09-11 0:15 ` Joe Perches 2019-09-15 9:43 ` Pavel Machek ` (2 subsequent siblings) 3 siblings, 1 reply; 42+ messages in thread From: Andy Shevchenko @ 2019-09-10 15:26 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Jonathan Corbet, Joe Perches, Petr Mladek, Sergey Senozhatsky, Jani Nikula, Linux Kernel Mailing List, Uwe Kleine-König, Linux Documentation List 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] printf: add support for printing symbolic error codes 2019-09-10 15:26 ` Andy Shevchenko @ 2019-09-11 0:15 ` Joe Perches 2019-09-11 6:43 ` Rasmus Villemoes 0 siblings, 1 reply; 42+ messages in thread From: Joe Perches @ 2019-09-11 0:15 UTC (permalink / raw) To: Andy Shevchenko, Rasmus Villemoes Cc: Andrew Morton, Jonathan Corbet, Petr Mladek, Sergey Senozhatsky, Jani Nikula, Linux Kernel Mailing List, Uwe Kleine-König, Linux Documentation List 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 ? ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] printf: add support for printing symbolic error codes 2019-09-11 0:15 ` Joe Perches @ 2019-09-11 6:43 ` Rasmus Villemoes 2019-09-11 9:37 ` Uwe Kleine-König 0 siblings, 1 reply; 42+ messages in thread From: Rasmus Villemoes @ 2019-09-11 6:43 UTC (permalink / raw) To: Joe Perches, Andy Shevchenko Cc: Andrew Morton, Jonathan Corbet, Petr Mladek, Sergey Senozhatsky, Jani Nikula, Linux Kernel Mailing List, Uwe Kleine-König, Linux Documentation List 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] printf: add support for printing symbolic error codes 2019-09-11 6:43 ` Rasmus Villemoes @ 2019-09-11 9:37 ` Uwe Kleine-König 2019-09-11 10:14 ` Rasmus Villemoes 0 siblings, 1 reply; 42+ messages in thread From: Uwe Kleine-König @ 2019-09-11 9:37 UTC (permalink / raw) To: Rasmus Villemoes, Joe Perches, Andy Shevchenko Cc: Andrew Morton, Jonathan Corbet, Petr Mladek, Sergey Senozhatsky, Jani Nikula, Linux Kernel Mailing List, Linux Documentation List [-- 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 --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] printf: add support for printing symbolic error codes 2019-09-11 9:37 ` Uwe Kleine-König @ 2019-09-11 10:14 ` Rasmus Villemoes 0 siblings, 0 replies; 42+ messages in thread From: Rasmus Villemoes @ 2019-09-11 10:14 UTC (permalink / raw) To: Uwe Kleine-König, Joe Perches, Andy Shevchenko Cc: Andrew Morton, Jonathan Corbet, Petr Mladek, Sergey Senozhatsky, Jani Nikula, Linux Kernel Mailing List, Linux Documentation List 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] printf: add support for printing symbolic error codes 2019-09-09 20:38 ` [PATCH v2] " Rasmus Villemoes 2019-09-10 15:26 ` Andy Shevchenko @ 2019-09-15 9:43 ` Pavel Machek 2019-09-16 12:23 ` Uwe Kleine-König 2019-09-17 6:59 ` [PATCH v3] " Rasmus Villemoes 3 siblings, 0 replies; 42+ messages in thread From: Pavel Machek @ 2019-09-15 9:43 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Jonathan Corbet, Andy Shevchenko, Joe Perches, Petr Mladek, Sergey Senozhatsky, Jani Nikula, Linux Kernel Mailing List, Uwe Kleine-K??nig, linux-doc 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] printf: add support for printing symbolic error codes 2019-09-09 20:38 ` [PATCH v2] " Rasmus Villemoes 2019-09-10 15:26 ` Andy Shevchenko 2019-09-15 9:43 ` Pavel Machek @ 2019-09-16 12:23 ` Uwe Kleine-König 2019-09-16 13:23 ` Rasmus Villemoes 2019-09-17 6:59 ` [PATCH v3] " Rasmus Villemoes 3 siblings, 1 reply; 42+ messages in thread From: Uwe Kleine-König @ 2019-09-16 12:23 UTC (permalink / raw) To: Rasmus Villemoes, Andrew Morton, Jonathan Corbet Cc: Andy Shevchenko, Joe Perches, Petr Mladek, Sergey Senozhatsky, Jani Nikula, Linux Kernel Mailing List, linux-doc [-- 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 --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] printf: add support for printing symbolic error codes 2019-09-16 12:23 ` Uwe Kleine-König @ 2019-09-16 13:23 ` Rasmus Villemoes 2019-09-16 13:36 ` Uwe Kleine-König 0 siblings, 1 reply; 42+ messages in thread From: Rasmus Villemoes @ 2019-09-16 13:23 UTC (permalink / raw) To: Uwe Kleine-König, Rasmus Villemoes, Andrew Morton, Jonathan Corbet Cc: Andy Shevchenko, Joe Perches, Petr Mladek, Sergey Senozhatsky, Jani Nikula, Linux Kernel Mailing List, linux-doc 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] printf: add support for printing symbolic error codes 2019-09-16 13:23 ` Rasmus Villemoes @ 2019-09-16 13:36 ` Uwe Kleine-König 0 siblings, 0 replies; 42+ messages in thread From: Uwe Kleine-König @ 2019-09-16 13:36 UTC (permalink / raw) To: Rasmus Villemoes, Andrew Morton, Jonathan Corbet Cc: Andy Shevchenko, Joe Perches, Petr Mladek, Sergey Senozhatsky, Jani Nikula, Linux Kernel Mailing List, linux-doc [-- 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 --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v3] printf: add support for printing symbolic error codes 2019-09-09 20:38 ` [PATCH v2] " Rasmus Villemoes ` (2 preceding siblings ...) 2019-09-16 12:23 ` Uwe Kleine-König @ 2019-09-17 6:59 ` Rasmus Villemoes 2019-09-25 14:36 ` Petr Mladek ` (2 more replies) 3 siblings, 3 replies; 42+ messages in thread From: Rasmus Villemoes @ 2019-09-17 6:59 UTC (permalink / raw) To: Andrew Morton, Jonathan Corbet Cc: Rasmus Villemoes, Andy Shevchenko, Joe Perches, Petr Mladek, Sergey Senozhatsky, Jani Nikula, Linux Kernel Mailing List, Uwe Kleine-König, linux-doc, Pavel Machek 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v3] printf: add support for printing symbolic error codes 2019-09-17 6:59 ` [PATCH v3] " Rasmus Villemoes @ 2019-09-25 14:36 ` Petr Mladek 2019-09-29 20:09 ` Rasmus Villemoes 2019-10-05 21:48 ` Andrew Morton 2019-10-11 13:36 ` [PATCH v4 0/1] printf: add support for printing symbolic error names Rasmus Villemoes 2 siblings, 1 reply; 42+ messages in thread From: Petr Mladek @ 2019-09-25 14:36 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Jonathan Corbet, Andy Shevchenko, Sergey Senozhatsky, Uwe Kleine-König, Jani Nikula, Joe Perches, Pavel Machek, linux-doc, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v3] printf: add support for printing symbolic error codes 2019-09-25 14:36 ` Petr Mladek @ 2019-09-29 20:09 ` Rasmus Villemoes 2019-10-02 8:34 ` Petr Mladek 0 siblings, 1 reply; 42+ messages in thread From: Rasmus Villemoes @ 2019-09-29 20:09 UTC (permalink / raw) To: Petr Mladek Cc: Andrew Morton, Jonathan Corbet, Andy Shevchenko, Sergey Senozhatsky, Uwe Kleine-König, Jani Nikula, Joe Perches, Pavel Machek, linux-doc, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v3] printf: add support for printing symbolic error codes 2019-09-29 20:09 ` Rasmus Villemoes @ 2019-10-02 8:34 ` Petr Mladek 0 siblings, 0 replies; 42+ messages in thread From: Petr Mladek @ 2019-10-02 8:34 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andy Shevchenko, Sergey Senozhatsky, Uwe Kleine-König, Andrew Morton, Jani Nikula, Jonathan Corbet, Joe Perches, Pavel Machek, linux-doc, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v3] printf: add support for printing symbolic error codes 2019-09-17 6:59 ` [PATCH v3] " Rasmus Villemoes 2019-09-25 14:36 ` Petr Mladek @ 2019-10-05 21:48 ` Andrew Morton 2019-10-11 13:36 ` [PATCH v4 0/1] printf: add support for printing symbolic error names Rasmus Villemoes 2 siblings, 0 replies; 42+ messages in thread From: Andrew Morton @ 2019-10-05 21:48 UTC (permalink / raw) To: Rasmus Villemoes Cc: Jonathan Corbet, Andy Shevchenko, Joe Perches, Petr Mladek, Sergey Senozhatsky, Jani Nikula, Linux Kernel Mailing List, Uwe Kleine-König, linux-doc, Pavel Machek 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? ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v4 0/1] printf: add support for printing symbolic error names 2019-09-17 6:59 ` [PATCH v3] " Rasmus Villemoes 2019-09-25 14:36 ` Petr Mladek 2019-10-05 21:48 ` Andrew Morton @ 2019-10-11 13:36 ` Rasmus Villemoes 2019-10-11 13:36 ` [PATCH v4 1/1] " Rasmus Villemoes ` (2 more replies) 2 siblings, 3 replies; 42+ messages in thread From: Rasmus Villemoes @ 2019-10-11 13:36 UTC (permalink / raw) To: Joe Perches, Petr Mladek, Uwe Kleine-König Cc: Andrew Morton, linux-kernel, Andy Shevchenko, Jonathan Corbet, Rasmus Villemoes This is a bit much for under the ---, so a separate cover letter for this single patch. v4: Dropped Uwe's ack since it's changed quite a bit. Change errcode->errname as suggested by Petr. Make it 'default y if PRINTK' so it's available in the common case, while those who have gone to great lengths to shave their kernel to the bare minimum are not affected. Also require the caller to use %pe instead of printing all ERR_PTRs symbolically. I can see some value in having the call site explicitly indicate that they're printing an ERR_PTR (i.e., having the %pe), but I also still believe it would make sense to print ordinary %p, ERR_PTR() symbolically instead of as a random hash value that's not stable across reboots. But in the interest of getting this in, I'll leave that for now. It's easy enough to do later by just changing the "case 'e'" to do a break (with an updated comment), then do an IS_ERR() check after the switch. Something I've glossed over in previous versions, and nobody has commented on, is that I produced "ENOSPC" while the 'fallback' would print "-28" (i.e., there's no minus in the symbolic case). I don't care much either way, but here I've tried to show how I'd do it if we want the minus also in the symbolic case. At first, I tried just using the standard idiom if (buf < end) *buf = '-'; buf++; followed by string(sym, ...). However, that doesn't work very well if one wants to honour field width - for that to work, the whole string including - must come from the errname() lookup and be handled by string(). The simplest seemed to be to just unconditionally prefix all strings with "-" when building the tables, and then change errname() back to supporting both positive and negative error numbers. As I said, I don't care much either way, so if somebody thinks this is too complicated and would prefer just printing "ENOSPC" (because really the minus doesn't offer much except that it's perhaps easier to recognize for a kernel developer) just speak up. I've also given some thought to Petr's suggestion of how to improve the handling of ERR_PTRs that are accidentally passed to %p<something-that-would-dereference-it>. But I'll do that as a separate series on top - for now I think this should go into -next if nobody complains loudly. Rasmus Villemoes (1): printf: add support for printing symbolic error names Documentation/core-api/printk-formats.rst | 12 ++ include/linux/errname.h | 16 ++ lib/Kconfig.debug | 9 + lib/Makefile | 1 + lib/errname.c | 222 ++++++++++++++++++++++ lib/test_printf.c | 24 +++ lib/vsprintf.c | 27 +++ 7 files changed, 311 insertions(+) create mode 100644 include/linux/errname.h create mode 100644 lib/errname.c -- 2.20.1 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v4 1/1] printf: add support for printing symbolic error names 2019-10-11 13:36 ` [PATCH v4 0/1] printf: add support for printing symbolic error names Rasmus Villemoes @ 2019-10-11 13:36 ` Rasmus Villemoes 2019-10-14 5:51 ` Uwe Kleine-König 2019-10-14 13:02 ` Petr Mladek 2019-10-15 19:07 ` [PATCH v5] " Rasmus Villemoes 2019-11-26 14:04 ` [PATCH v4 0/1] " Geert Uytterhoeven 2 siblings, 2 replies; 42+ messages in thread From: Rasmus Villemoes @ 2019-10-11 13:36 UTC (permalink / raw) To: Joe Perches, Petr Mladek, Uwe Kleine-König Cc: Andrew Morton, linux-kernel, Andy Shevchenko, Jonathan Corbet, 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 implements that as a %p extension: With %pe, one can do if (IS_ERR(foo)) { pr_err("Sorry, can't do that: %pe\n", foo); return PTR_ERR(foo); } instead of what is seen in quite a few places in the kernel: if (IS_ERR(foo)) { pr_err("Sorry, can't do that: %ld\n", PTR_ERR(foo)); return PTR_ERR(foo); } If the value passed to %pe is an ERR_PTR, but the library function errname() added here doesn't know about the value, the value is simply printed in decimal. If the value passed to %pe is not an ERR_PTR, we treat it as an ordinary %p and thus print the hashed value (passing non-ERR_PTR values to %pe indicates a bug in the caller, but we can't do much about that). With my embedded hat on, and because it's not very invasive to do, I've made it possible to remove this. The errname() function and associated lookup tables take up about 3K. For most, that's probably quite acceptable and a price worth paying for more readable dmesg (once this starts getting used), while for those that disable printk() it's of very little use - I don't see a procfs/sysfs/seq_printf() file reasonably making use of this - and they clearly want to squeeze vmlinux as much as possible. Hence the default y if PRINTK. 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. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- Documentation/core-api/printk-formats.rst | 12 ++ include/linux/errname.h | 16 ++ lib/Kconfig.debug | 9 + lib/Makefile | 1 + lib/errname.c | 222 ++++++++++++++++++++++ lib/test_printf.c | 24 +++ lib/vsprintf.c | 27 +++ 7 files changed, 311 insertions(+) create mode 100644 include/linux/errname.h create mode 100644 lib/errname.c diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index ecbebf4ca8e7..050f34f3a70f 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -79,6 +79,18 @@ has the added benefit of providing a unique identifier. On 64-bit machines the first 32 bits are zeroed. The kernel will print ``(ptrval)`` until it gathers enough entropy. If you *really* want the address see %px below. +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. + Symbols/Function Pointers ------------------------- diff --git a/include/linux/errname.h b/include/linux/errname.h new file mode 100644 index 000000000000..e8576ad90cb7 --- /dev/null +++ b/include/linux/errname.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_ERRNAME_H +#define _LINUX_ERRNAME_H + +#include <linux/stddef.h> + +#ifdef CONFIG_SYMBOLIC_ERRNAME +const char *errname(int err); +#else +static inline const char *errname(int err) +{ + return NULL; +} +#endif + +#endif /* _LINUX_ERRNAME_H */ diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 93d97f9b0157..99a6cf677140 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -164,6 +164,15 @@ config DYNAMIC_DEBUG See Documentation/admin-guide/dynamic-debug-howto.rst for additional information. +config SYMBOLIC_ERRNAME + bool "Support symbolic error names in printf" + default y if PRINTK + help + If you say Y here, the kernel's printf implementation will + be able to print symbolic error names 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..d740534057ed 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_ERRNAME) += errname.o obj-$(CONFIG_NLATTR) += nlattr.o diff --git a/lib/errname.c b/lib/errname.c new file mode 100644 index 000000000000..30d3bab99477 --- /dev/null +++ b/lib/errname.c @@ -0,0 +1,222 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/build_bug.h> +#include <linux/errno.h> +#include <linux/errname.h> +#include <linux/kernel.h> + +/* + * Ensure these tables do 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 *names_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 *names_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 + +static const char *__errname(unsigned err) +{ + if (err < ARRAY_SIZE(names_0)) + return names_0[err]; + if (err >= 512 && err - 512 < ARRAY_SIZE(names_512)) + return names_512[err - 512]; + /* But why? */ + if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */ + return "EDQUOT"; + return NULL; +} + +/* + * errname(EIO) -> "EIO" + * errname(-EIO) -> "-EIO" + */ +const char *errname(int err) +{ + bool pos = err > 0; + const char *name = __errname(err > 0 ? err : -err); + + return name ? name + pos : NULL; +} diff --git a/lib/test_printf.c b/lib/test_printf.c index 5d94cbff2120..4fa0ccf58420 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -593,6 +593,29 @@ flags(void) kfree(cmp_buffer); } +static void __init +errptr(void) +{ + char buf[PLAIN_BUF_SIZE]; + + test("-1234", "%pe", ERR_PTR(-1234)); + + /* Check that %pe with a non-ERR_PTR gets treated as ordinary %p. */ + BUILD_BUG_ON(IS_ERR(PTR)); + snprintf(buf, sizeof(buf), "(%p)", PTR); + test(buf, "(%pe)", PTR); + +#ifdef CONFIG_SYMBOLIC_ERRNAME + test("(-ENOTSOCK)", "(%pe)", ERR_PTR(-ENOTSOCK)); + test("(-EAGAIN)", "(%pe)", ERR_PTR(-EAGAIN)); + BUILD_BUG_ON(EAGAIN != EWOULDBLOCK); + test("(-EAGAIN)", "(%pe)", ERR_PTR(-EWOULDBLOCK)); + test("[-EIO ]", "[%-8pe]", ERR_PTR(-EIO)); + test("[ -EIO]", "[%8pe]", ERR_PTR(-EIO)); + test("-EPROBE_DEFER", "%pe", ERR_PTR(-EPROBE_DEFER)); +#endif +} + static void __init test_pointer(void) { @@ -615,6 +638,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 e78017a3e1bd..b54d252b398e 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/errname.h> #include <linux/module.h> /* for KSYM_SYMBOL_LEN */ #include <linux/types.h> #include <linux/string.h> @@ -613,6 +614,25 @@ static char *string_nocheck(char *buf, char *end, const char *s, return widen_string(buf, len, end, spec); } +static char *err_ptr(char *buf, char *end, void *ptr, + struct printf_spec spec) +{ + int err = PTR_ERR(ptr); + const char *sym = errname(err); + + if (sym) + return string_nocheck(buf, end, sym, spec); + + /* + * Somebody passed ERR_PTR(-1234) or some other non-existing + * Efoo - or perhaps CONFIG_SYMBOLIC_ERRNAME=n. Fall back to + * printing it as its decimal representation. + */ + spec.flags |= SIGN; + spec.base = 10; + return number(buf, end, err, spec); +} + /* Be careful: error messages must fit into the given buffer. */ static char *error_string(char *buf, char *end, const char *s, struct printf_spec spec) @@ -2187,6 +2207,11 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, return kobject_string(buf, end, ptr, spec, fmt); 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)) + break; + return err_ptr(buf, end, ptr, spec); } /* default is to _not_ leak addresses, hash before printing */ @@ -2823,6 +2848,7 @@ int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args) case 'f': case 'x': case 'K': + case 'e': save_arg(void *); break; default: @@ -2999,6 +3025,7 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf) case 'f': case 'x': case 'K': + case 'e': process = true; break; default: -- 2.20.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v4 1/1] printf: add support for printing symbolic error names 2019-10-11 13:36 ` [PATCH v4 1/1] " Rasmus Villemoes @ 2019-10-14 5:51 ` Uwe Kleine-König 2019-10-14 13:02 ` Petr Mladek 1 sibling, 0 replies; 42+ messages in thread From: Uwe Kleine-König @ 2019-10-14 5:51 UTC (permalink / raw) To: Rasmus Villemoes Cc: Joe Perches, Petr Mladek, Andrew Morton, linux-kernel, Andy Shevchenko, Jonathan Corbet [-- Attachment #1: Type: text/plain, Size: 2168 bytes --] On Fri, Oct 11, 2019 at 03:36:17PM +0200, 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 > implements that as a %p extension: With %pe, one can do > > if (IS_ERR(foo)) { > pr_err("Sorry, can't do that: %pe\n", foo); > return PTR_ERR(foo); > } > > instead of what is seen in quite a few places in the kernel: > > if (IS_ERR(foo)) { > pr_err("Sorry, can't do that: %ld\n", PTR_ERR(foo)); > return PTR_ERR(foo); > } > > If the value passed to %pe is an ERR_PTR, but the library function > errname() added here doesn't know about the value, the value is simply > printed in decimal. If the value passed to %pe is not an ERR_PTR, we > treat it as an ordinary %p and thus print the hashed value (passing > non-ERR_PTR values to %pe indicates a bug in the caller, but we can't > do much about that). > > With my embedded hat on, and because it's not very invasive to do, > I've made it possible to remove this. The errname() function and > associated lookup tables take up about 3K. For most, that's probably > quite acceptable and a price worth paying for more readable > dmesg (once this starts getting used), while for those that disable > printk() it's of very little use - I don't see a > procfs/sysfs/seq_printf() file reasonably making use of this - and > they clearly want to squeeze vmlinux as much as possible. Hence the > default y if PRINTK. > > 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. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> I like having an explicit code even better than relying on a plain %p to emit the code. So please readd my Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org> Best regards Uwe [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v4 1/1] printf: add support for printing symbolic error names 2019-10-11 13:36 ` [PATCH v4 1/1] " Rasmus Villemoes 2019-10-14 5:51 ` Uwe Kleine-König @ 2019-10-14 13:02 ` Petr Mladek 2019-10-14 13:10 ` Andy Shevchenko ` (2 more replies) 1 sibling, 3 replies; 42+ messages in thread From: Petr Mladek @ 2019-10-14 13:02 UTC (permalink / raw) To: Rasmus Villemoes Cc: Uwe Kleine-König, Joe Perches, Andy Shevchenko, Andrew Morton, Jonathan Corbet, linux-kernel On Fri 2019-10-11 15:36:17, 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 > implements that as a %p extension: With %pe, one can do Reviewed-by: Petr Mladek <pmladek@suse.com> I like the patch. There are only two rather cosmetic things. > diff --git a/lib/errname.c b/lib/errname.c > new file mode 100644 > index 000000000000..30d3bab99477 > --- /dev/null > +++ b/lib/errname.c > +const char *errname(int err) > +{ > + bool pos = err > 0; > + const char *name = __errname(err > 0 ? err : -err); > + > + return name ? name + pos : NULL; This made me to check C standard. It seems that "true" really has to be "1". But I think that I am not the only one who is not sure. I would prefer to make it less tricky and use, for example: const char *name = __errname(err > 0 ? err : -err); if (!name) return NULL; return err > 0 ? name + 1 : name; > +} > diff --git a/lib/test_printf.c b/lib/test_printf.c > index 5d94cbff2120..4fa0ccf58420 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -593,6 +593,29 @@ flags(void) > kfree(cmp_buffer); > } > > +static void __init > +errptr(void) > +{ > + char buf[PLAIN_BUF_SIZE]; > + > + test("-1234", "%pe", ERR_PTR(-1234)); > + > + /* Check that %pe with a non-ERR_PTR gets treated as ordinary %p. */ > + BUILD_BUG_ON(IS_ERR(PTR)); > + snprintf(buf, sizeof(buf), "(%p)", PTR); > + test(buf, "(%pe)", PTR); There is a small race. "(____ptrval____)" is used for %p before random numbers are initialized. The switch is done via workqueue work, see enable_ptr_key_workfn(). It means that it can be done in parallel. I doubt that anyone would ever hit the race. But it could be very confusing and hard to debug. I would replace it with: test_hashed("%pe", PTR); If would like to have the two things fixed. I am not sure if you want to send one more revision. Or I could also change it by follow up patch when pushing. What is your preference, please? Best Regards, Petr ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v4 1/1] printf: add support for printing symbolic error names 2019-10-14 13:02 ` Petr Mladek @ 2019-10-14 13:10 ` Andy Shevchenko 2019-10-15 12:17 ` Rasmus Villemoes 2019-10-15 13:44 ` David Laight 2 siblings, 0 replies; 42+ messages in thread From: Andy Shevchenko @ 2019-10-14 13:10 UTC (permalink / raw) To: Petr Mladek Cc: Rasmus Villemoes, Uwe Kleine-König, Joe Perches, Andrew Morton, Jonathan Corbet, Linux Kernel Mailing List On Mon, Oct 14, 2019 at 4:02 PM Petr Mladek <pmladek@suse.com> wrote: > On Fri 2019-10-11 15:36:17, 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 > > implements that as a %p extension: With %pe, one can do > > +const char *errname(int err) > > +{ > > + bool pos = err > 0; > > + const char *name = __errname(err > 0 ? err : -err); > > + > > + return name ? name + pos : NULL; > > This made me to check C standard. It seems that "true" really has > to be "1". You may guarantee it by using !!. return name ? name + !!(err > 0) : NULL; But to me it seems like forgotten use of pos in the other case above. Anyway, I would rather do abs(err) in the first place and simple use name + 1 in the second as you suggested with maybe a comment that we skip E letter. > But I think that I am not the only one who is not sure. > I would prefer to make it less tricky and use, for example: > > const char *name = __errname(err > 0 ? err : -err); > if (!name) > return NULL; > > return err > 0 ? name + 1 : name; > > +} -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v4 1/1] printf: add support for printing symbolic error names 2019-10-14 13:02 ` Petr Mladek 2019-10-14 13:10 ` Andy Shevchenko @ 2019-10-15 12:17 ` Rasmus Villemoes 2019-10-15 13:44 ` David Laight 2 siblings, 0 replies; 42+ messages in thread From: Rasmus Villemoes @ 2019-10-15 12:17 UTC (permalink / raw) To: Petr Mladek Cc: Uwe Kleine-König, Joe Perches, Andy Shevchenko, Andrew Morton, Jonathan Corbet, linux-kernel On 14/10/2019 15.02, Petr Mladek wrote: > On Fri 2019-10-11 15:36:17, 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 >> implements that as a %p extension: With %pe, one can do > > Reviewed-by: Petr Mladek <pmladek@suse.com> > > I like the patch. There are only two rather cosmetic things. > >> diff --git a/lib/errname.c b/lib/errname.c >> new file mode 100644 >> index 000000000000..30d3bab99477 >> --- /dev/null >> +++ b/lib/errname.c >> +const char *errname(int err) >> +{ >> + bool pos = err > 0; >> + const char *name = __errname(err > 0 ? err : -err); >> + >> + return name ? name + pos : NULL; > > This made me to check C standard. It seems that "true" really has > to be "1". > > But I think that I am not the only one who is not sure. > I would prefer to make it less tricky and use, for example: > > const char *name = __errname(err > 0 ? err : -err); > if (!name) > return NULL; > > return err > 0 ? name + 1 : name; I didn't even stop to think that using the value of a comparison operator/bool in arithmetic might be the littlest bit surprising for C programmers. But your suggestion isn't terrible, so go ahead and write it that way. And can I get you to fix the missing "-" in the MIPS "EDQUOT" special case while you're at it? >> +static void __init >> +errptr(void) >> +{ >> + char buf[PLAIN_BUF_SIZE]; >> + >> + test("-1234", "%pe", ERR_PTR(-1234)); >> + >> + /* Check that %pe with a non-ERR_PTR gets treated as ordinary %p. */ >> + BUILD_BUG_ON(IS_ERR(PTR)); >> + snprintf(buf, sizeof(buf), "(%p)", PTR); >> + test(buf, "(%pe)", PTR); > > There is a small race. "(____ptrval____)" is used for %p before > random numbers are initialized. The switch is done via workqueue > work, see enable_ptr_key_workfn(). It means that it can be done > in parallel. I know. > I doubt that anyone would ever hit the race. But it could be very confusing > and hard to debug. I thought about it and decided not to care, as long as the errptr test comes after the hashing test, because if the hashing is not initialized then one gets a warning. I also considered setting a flag in that case and then skipping the errptr hash test, but again, decided that the warning would be enough. > I would replace it with: > test_hashed("%pe", PTR); Sure, that will repeat the warning, but it doesn't seem to prevent a false positive: Between plain_hash_to_buffer emitting the warning (and returning 0) and the caller test_hashed() then performing the test() against the buffer contents, the hash can become initialized and thus change how %p[e] gets formatted. But ok, perhaps it is cleaner to reuse test_hashed and avoid the local buffer in errptr. So yeah, I suppose this on top is fine: diff --git a/lib/test_printf.c b/lib/test_printf.c index 4fa0ccf58420..030daeb4fe21 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -596,14 +596,11 @@ flags(void) static void __init errptr(void) { - char buf[PLAIN_BUF_SIZE]; - test("-1234", "%pe", ERR_PTR(-1234)); /* Check that %pe with a non-ERR_PTR gets treated as ordinary %p. */ BUILD_BUG_ON(IS_ERR(PTR)); - snprintf(buf, sizeof(buf), "(%p)", PTR); - test(buf, "(%pe)", PTR); + test_hashed("%pe", PTR); #ifdef CONFIG_SYMBOLIC_ERRNAME test("(-ENOTSOCK)", "(%pe)", ERR_PTR(-ENOTSOCK)); > If would like to have the two things fixed. I am not sure if you want > to send one more revision. Or I could also change it by follow > up patch when pushing. I prefer you to fold in both changes instead of an extra patch, and if you can't, I'll send a new revision. Rasmus ^ permalink raw reply related [flat|nested] 42+ messages in thread
* RE: [PATCH v4 1/1] printf: add support for printing symbolic error names 2019-10-14 13:02 ` Petr Mladek 2019-10-14 13:10 ` Andy Shevchenko 2019-10-15 12:17 ` Rasmus Villemoes @ 2019-10-15 13:44 ` David Laight 2 siblings, 0 replies; 42+ messages in thread From: David Laight @ 2019-10-15 13:44 UTC (permalink / raw) To: 'Petr Mladek', Rasmus Villemoes Cc: Uwe Kleine-König, Joe Perches, Andy Shevchenko, Andrew Morton, Jonathan Corbet, linux-kernel From: Petr Mladek > Sent: 14 October 2019 14:03 > On Fri 2019-10-11 15:36:17, 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 > > implements that as a %p extension: With %pe, one can do > > Reviewed-by: Petr Mladek <pmladek@suse.com> > > I like the patch. There are only two rather cosmetic things. > > > diff --git a/lib/errname.c b/lib/errname.c > > new file mode 100644 > > index 000000000000..30d3bab99477 > > --- /dev/null > > +++ b/lib/errname.c > > +const char *errname(int err) > > +{ > > + bool pos = err > 0; > > + const char *name = __errname(err > 0 ? err : -err); > > + > > + return name ? name + pos : NULL; > > This made me to check C standard. It seems that "true" really has > to be "1". What is thus "true" symbol you are talking about? Actually you could get rid of the "bool" as well. unsigned int pos = err > 0; const char *name = __errname(pos ? err : -err); return name ? &pos[name] : NULL; then it is all well defined. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v5] printf: add support for printing symbolic error names 2019-10-11 13:36 ` [PATCH v4 0/1] printf: add support for printing symbolic error names Rasmus Villemoes 2019-10-11 13:36 ` [PATCH v4 1/1] " Rasmus Villemoes @ 2019-10-15 19:07 ` Rasmus Villemoes 2019-10-16 13:49 ` Andy Shevchenko 2019-11-26 14:04 ` [PATCH v4 0/1] " Geert Uytterhoeven 2 siblings, 1 reply; 42+ messages in thread From: Rasmus Villemoes @ 2019-10-15 19:07 UTC (permalink / raw) To: linux-kernel, Jonathan Corbet Cc: Joe Perches, Andy Shevchenko, Andrew Morton, Rasmus Villemoes, Uwe Kleine-König, Petr Mladek, linux-doc It has been suggested several times to extend vsnprintf() to be able to convert the numeric value of ENOSPC to print "ENOSPC". This implements that as a %p extension: With %pe, one can do if (IS_ERR(foo)) { pr_err("Sorry, can't do that: %pe\n", foo); return PTR_ERR(foo); } instead of what is seen in quite a few places in the kernel: if (IS_ERR(foo)) { pr_err("Sorry, can't do that: %ld\n", PTR_ERR(foo)); return PTR_ERR(foo); } If the value passed to %pe is an ERR_PTR, but the library function errname() added here doesn't know about the value, the value is simply printed in decimal. If the value passed to %pe is not an ERR_PTR, we treat it as an ordinary %p and thus print the hashed value (passing non-ERR_PTR values to %pe indicates a bug in the caller, but we can't do much about that). With my embedded hat on, and because it's not very invasive to do, I've made it possible to remove this. The errname() function and associated lookup tables take up about 3K. For most, that's probably quite acceptable and a price worth paying for more readable dmesg (once this starts getting used), while for those that disable printk() it's of very little use - I don't see a procfs/sysfs/seq_printf() file reasonably making use of this - and they clearly want to squeeze vmlinux as much as possible. Hence the default y if PRINTK. 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> Reviewed-by: Petr Mladek <pmladek@suse.com> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- Found a few more things, so sending new revision anyway. v5: Fix cosmetic issues per Petr. Fix missing "-" in "EDQUOT". Fix a few comments on at the end of the names_0 array. Add A-b Uwe, R-b Petr. Documentation/core-api/printk-formats.rst | 12 ++ include/linux/errname.h | 16 ++ lib/Kconfig.debug | 9 + lib/Makefile | 1 + lib/errname.c | 223 ++++++++++++++++++++++ lib/test_printf.c | 21 ++ lib/vsprintf.c | 27 +++ 7 files changed, 309 insertions(+) create mode 100644 include/linux/errname.h create mode 100644 lib/errname.c diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index ecbebf4ca8e7..050f34f3a70f 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -79,6 +79,18 @@ has the added benefit of providing a unique identifier. On 64-bit machines the first 32 bits are zeroed. The kernel will print ``(ptrval)`` until it gathers enough entropy. If you *really* want the address see %px below. +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. + Symbols/Function Pointers ------------------------- diff --git a/include/linux/errname.h b/include/linux/errname.h new file mode 100644 index 000000000000..e8576ad90cb7 --- /dev/null +++ b/include/linux/errname.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_ERRNAME_H +#define _LINUX_ERRNAME_H + +#include <linux/stddef.h> + +#ifdef CONFIG_SYMBOLIC_ERRNAME +const char *errname(int err); +#else +static inline const char *errname(int err) +{ + return NULL; +} +#endif + +#endif /* _LINUX_ERRNAME_H */ diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 93d97f9b0157..99a6cf677140 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -164,6 +164,15 @@ config DYNAMIC_DEBUG See Documentation/admin-guide/dynamic-debug-howto.rst for additional information. +config SYMBOLIC_ERRNAME + bool "Support symbolic error names in printf" + default y if PRINTK + help + If you say Y here, the kernel's printf implementation will + be able to print symbolic error names 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..d740534057ed 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_ERRNAME) += errname.o obj-$(CONFIG_NLATTR) += nlattr.o diff --git a/lib/errname.c b/lib/errname.c new file mode 100644 index 000000000000..b67f58e29729 --- /dev/null +++ b/lib/errname.c @@ -0,0 +1,223 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/build_bug.h> +#include <linux/errno.h> +#include <linux/errname.h> +#include <linux/kernel.h> + +/* + * Ensure these tables do 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 *names_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), /* ECANCELLED */ + E(EAGAIN), /* EWOULDBLOCK */ + E(ECONNREFUSED), /* EREFUSED */ + E(EDEADLK), /* EDEADLOCK */ +}; +#undef E + +#define E(err) [err - 512 + BUILD_BUG_ON_ZERO(err < 512 || err > 550)] = "-" #err +static const char *names_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 + +static const char *__errname(unsigned err) +{ + if (err < ARRAY_SIZE(names_0)) + return names_0[err]; + if (err >= 512 && err - 512 < ARRAY_SIZE(names_512)) + return names_512[err - 512]; + /* But why? */ + if (IS_ENABLED(CONFIG_MIPS) && err == EDQUOT) /* 1133 */ + return "-EDQUOT"; + return NULL; +} + +/* + * errname(EIO) -> "EIO" + * errname(-EIO) -> "-EIO" + */ +const char *errname(int err) +{ + const char *name = __errname(err > 0 ? err : -err); + if (!name) + return NULL; + + return err > 0 ? name + 1 : name; +} diff --git a/lib/test_printf.c b/lib/test_printf.c index 5d94cbff2120..030daeb4fe21 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -593,6 +593,26 @@ flags(void) kfree(cmp_buffer); } +static void __init +errptr(void) +{ + test("-1234", "%pe", ERR_PTR(-1234)); + + /* Check that %pe with a non-ERR_PTR gets treated as ordinary %p. */ + BUILD_BUG_ON(IS_ERR(PTR)); + test_hashed("%pe", PTR); + +#ifdef CONFIG_SYMBOLIC_ERRNAME + test("(-ENOTSOCK)", "(%pe)", ERR_PTR(-ENOTSOCK)); + test("(-EAGAIN)", "(%pe)", ERR_PTR(-EAGAIN)); + BUILD_BUG_ON(EAGAIN != EWOULDBLOCK); + test("(-EAGAIN)", "(%pe)", ERR_PTR(-EWOULDBLOCK)); + test("[-EIO ]", "[%-8pe]", ERR_PTR(-EIO)); + test("[ -EIO]", "[%8pe]", ERR_PTR(-EIO)); + test("-EPROBE_DEFER", "%pe", ERR_PTR(-EPROBE_DEFER)); +#endif +} + static void __init test_pointer(void) { @@ -615,6 +635,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 e78017a3e1bd..b54d252b398e 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/errname.h> #include <linux/module.h> /* for KSYM_SYMBOL_LEN */ #include <linux/types.h> #include <linux/string.h> @@ -613,6 +614,25 @@ static char *string_nocheck(char *buf, char *end, const char *s, return widen_string(buf, len, end, spec); } +static char *err_ptr(char *buf, char *end, void *ptr, + struct printf_spec spec) +{ + int err = PTR_ERR(ptr); + const char *sym = errname(err); + + if (sym) + return string_nocheck(buf, end, sym, spec); + + /* + * Somebody passed ERR_PTR(-1234) or some other non-existing + * Efoo - or perhaps CONFIG_SYMBOLIC_ERRNAME=n. Fall back to + * printing it as its decimal representation. + */ + spec.flags |= SIGN; + spec.base = 10; + return number(buf, end, err, spec); +} + /* Be careful: error messages must fit into the given buffer. */ static char *error_string(char *buf, char *end, const char *s, struct printf_spec spec) @@ -2187,6 +2207,11 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, return kobject_string(buf, end, ptr, spec, fmt); 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)) + break; + return err_ptr(buf, end, ptr, spec); } /* default is to _not_ leak addresses, hash before printing */ @@ -2823,6 +2848,7 @@ int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args) case 'f': case 'x': case 'K': + case 'e': save_arg(void *); break; default: @@ -2999,6 +3025,7 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf) case 'f': case 'x': case 'K': + case 'e': process = true; break; default: -- 2.20.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v5] printf: add support for printing symbolic error names 2019-10-15 19:07 ` [PATCH v5] " Rasmus Villemoes @ 2019-10-16 13:49 ` Andy Shevchenko 2019-10-16 14:52 ` Petr Mladek 0 siblings, 1 reply; 42+ messages in thread From: Andy Shevchenko @ 2019-10-16 13:49 UTC (permalink / raw) To: Rasmus Villemoes Cc: Linux Kernel Mailing List, Jonathan Corbet, Joe Perches, Andrew Morton, Uwe Kleine-König, Petr Mladek, Linux Documentation List On Tue, Oct 15, 2019 at 10:07 PM Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > +const char *errname(int err) > +{ > + const char *name = __errname(err > 0 ? err : -err); Looks like mine comment left unseen. What about to simple use abs(err) here? > + if (!name) > + return NULL; > + > + return err > 0 ? name + 1 : name; > +} -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v5] printf: add support for printing symbolic error names 2019-10-16 13:49 ` Andy Shevchenko @ 2019-10-16 14:52 ` Petr Mladek 2019-10-16 16:31 ` Andy Shevchenko 0 siblings, 1 reply; 42+ messages in thread From: Petr Mladek @ 2019-10-16 14:52 UTC (permalink / raw) To: Andy Shevchenko Cc: Rasmus Villemoes, Uwe Kleine-König, Andrew Morton, Jonathan Corbet, Joe Perches, Linux Documentation List, Linux Kernel Mailing List On Wed 2019-10-16 16:49:41, Andy Shevchenko wrote: > On Tue, Oct 15, 2019 at 10:07 PM Rasmus Villemoes > <linux@rasmusvillemoes.dk> wrote: > > > +const char *errname(int err) > > +{ > > + const char *name = __errname(err > 0 ? err : -err); > > Looks like mine comment left unseen. > What about to simple use abs(err) here? Andy, would you want to ack the patch with this change? I could do it when pushing the patch. Otherwise, it looks ready to go. Thanks everybody involved for the patience. Best Regards, Petr ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v5] printf: add support for printing symbolic error names 2019-10-16 14:52 ` Petr Mladek @ 2019-10-16 16:31 ` Andy Shevchenko 2019-10-17 15:02 ` Petr Mladek 0 siblings, 1 reply; 42+ messages in thread From: Andy Shevchenko @ 2019-10-16 16:31 UTC (permalink / raw) To: Petr Mladek Cc: Rasmus Villemoes, Uwe Kleine-König, Andrew Morton, Jonathan Corbet, Joe Perches, Linux Documentation List, Linux Kernel Mailing List On Wed, Oct 16, 2019 at 5:52 PM Petr Mladek <pmladek@suse.com> wrote: > > On Wed 2019-10-16 16:49:41, Andy Shevchenko wrote: > > On Tue, Oct 15, 2019 at 10:07 PM Rasmus Villemoes > > <linux@rasmusvillemoes.dk> wrote: > > > > > +const char *errname(int err) > > > +{ > > > + const char *name = __errname(err > 0 ? err : -err); > > > > Looks like mine comment left unseen. > > What about to simple use abs(err) here? > > Andy, would you want to ack the patch with this change? > I could do it when pushing the patch. Looks good to me. Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Otherwise, it looks ready to go. > > Thanks everybody involved for the patience. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v5] printf: add support for printing symbolic error names 2019-10-16 16:31 ` Andy Shevchenko @ 2019-10-17 15:02 ` Petr Mladek 0 siblings, 0 replies; 42+ messages in thread From: Petr Mladek @ 2019-10-17 15:02 UTC (permalink / raw) To: Andy Shevchenko Cc: Uwe Kleine-König, Andrew Morton, Jonathan Corbet, Joe Perches, Rasmus Villemoes, Linux Documentation List, Linux Kernel Mailing List On Wed 2019-10-16 19:31:33, Andy Shevchenko wrote: > On Wed, Oct 16, 2019 at 5:52 PM Petr Mladek <pmladek@suse.com> wrote: > > > > On Wed 2019-10-16 16:49:41, Andy Shevchenko wrote: > > > On Tue, Oct 15, 2019 at 10:07 PM Rasmus Villemoes > > > <linux@rasmusvillemoes.dk> wrote: > > > > > > > +const char *errname(int err) > > > > +{ > > > > + const char *name = __errname(err > 0 ? err : -err); > > > > > > Looks like mine comment left unseen. > > > What about to simple use abs(err) here? > > > > Andy, would you want to ack the patch with this change? > > I could do it when pushing the patch. > > Looks good to me. > Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> The patch has been committed into printk.git, branch for-5.5. Best Regards, Petr ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v4 0/1] printf: add support for printing symbolic error names 2019-10-11 13:36 ` [PATCH v4 0/1] printf: add support for printing symbolic error names Rasmus Villemoes 2019-10-11 13:36 ` [PATCH v4 1/1] " Rasmus Villemoes 2019-10-15 19:07 ` [PATCH v5] " Rasmus Villemoes @ 2019-11-26 14:04 ` Geert Uytterhoeven 2019-11-27 8:54 ` Petr Mladek 2 siblings, 1 reply; 42+ messages in thread From: Geert Uytterhoeven @ 2019-11-26 14:04 UTC (permalink / raw) To: Rasmus Villemoes Cc: Joe Perches, Petr Mladek, Uwe Kleine-König, Andrew Morton, Linux Kernel Mailing List, Andy Shevchenko, Jonathan Corbet Hi Rasmus, Nice idea! On Fri, Oct 11, 2019 at 3:38 PM Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > This is a bit much for under the ---, so a separate cover letter for > this single patch. > > v4: Dropped Uwe's ack since it's changed quite a bit. Change > errcode->errname as suggested by Petr. Make it 'default y if PRINTK' > so it's available in the common case, while those who have gone to > great lengths to shave their kernel to the bare minimum are not > affected. > > Also require the caller to use %pe instead of printing all ERR_PTRs > symbolically. I can see some value in having the call site explicitly > indicate that they're printing an ERR_PTR (i.e., having the %pe), but > I also still believe it would make sense to print ordinary %p, > ERR_PTR() symbolically instead of as a random hash value that's not > stable across reboots. But in the interest of getting this in, I'll > leave that for now. It's easy enough to do later by just changing the > "case 'e'" to do a break (with an updated comment), then do an > IS_ERR() check after the switch. > > Something I've glossed over in previous versions, and nobody has > commented on, is that I produced "ENOSPC" while the 'fallback' would > print "-28" (i.e., there's no minus in the symbolic case). I don't > care much either way, but here I've tried to show how I'd do it if we > want the minus also in the symbolic case. At first, I tried just using > the standard idiom > > if (buf < end) > *buf = '-'; > buf++; > > followed by string(sym, ...). However, that doesn't work very well if > one wants to honour field width - for that to work, the whole string > including - must come from the errname() lookup and be handled by > string(). The simplest seemed to be to just unconditionally prefix all > strings with "-" when building the tables, and then change errname() > back to supporting both positive and negative error numbers. Still, it looks a bit wasteful to me to include the dash in each and every string value. Do you think you can code the +/- logic in string_nocheck() in less than the gain achieved by dropping the dashes from the tables? (e.g. by using the SIGN spec.flags? ;-) Or, do we need it? IS_ERR() doesn't consider positive values errors. Oh, what about the leading "E"? That one looks harder to get rid of, though ;-) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v4 0/1] printf: add support for printing symbolic error names 2019-11-26 14:04 ` [PATCH v4 0/1] " Geert Uytterhoeven @ 2019-11-27 8:54 ` Petr Mladek 0 siblings, 0 replies; 42+ messages in thread From: Petr Mladek @ 2019-11-27 8:54 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Rasmus Villemoes, Joe Perches, Uwe Kleine-König, Andrew Morton, Linux Kernel Mailing List, Andy Shevchenko, Jonathan Corbet On Tue 2019-11-26 15:04:06, Geert Uytterhoeven wrote: > Hi Rasmus, > > Nice idea! > > On Fri, Oct 11, 2019 at 3:38 PM Rasmus Villemoes > <linux@rasmusvillemoes.dk> wrote: > > This is a bit much for under the ---, so a separate cover letter for > > this single patch. > > > > v4: Dropped Uwe's ack since it's changed quite a bit. Change > > errcode->errname as suggested by Petr. Make it 'default y if PRINTK' > > so it's available in the common case, while those who have gone to > > great lengths to shave their kernel to the bare minimum are not > > affected. > > > > Also require the caller to use %pe instead of printing all ERR_PTRs > > symbolically. I can see some value in having the call site explicitly > > indicate that they're printing an ERR_PTR (i.e., having the %pe), but > > I also still believe it would make sense to print ordinary %p, > > ERR_PTR() symbolically instead of as a random hash value that's not > > stable across reboots. But in the interest of getting this in, I'll > > leave that for now. It's easy enough to do later by just changing the > > "case 'e'" to do a break (with an updated comment), then do an > > IS_ERR() check after the switch. > > > > Something I've glossed over in previous versions, and nobody has > > commented on, is that I produced "ENOSPC" while the 'fallback' would > > print "-28" (i.e., there's no minus in the symbolic case). I don't > > care much either way, but here I've tried to show how I'd do it if we > > want the minus also in the symbolic case. At first, I tried just using > > the standard idiom > > > > if (buf < end) > > *buf = '-'; > > buf++; > > > > followed by string(sym, ...). However, that doesn't work very well if > > one wants to honour field width - for that to work, the whole string > > including - must come from the errname() lookup and be handled by > > string(). The simplest seemed to be to just unconditionally prefix all > > strings with "-" when building the tables, and then change errname() > > back to supporting both positive and negative error numbers. > > Still, it looks a bit wasteful to me to include the dash in each and every > string value. > > Do you think you can code the +/- logic in string_nocheck() in less than > the gain achieved by dropping the dashes from the tables? > (e.g. by using the SIGN spec.flags? ;-) > Or, do we need it? IS_ERR() doesn't consider positive values errors. > > Oh, what about the leading "E"? That one looks harder to get rid of, > though ;-) It would be nice. But too big hack is not worth it. Anybody who cares about saving 0.2kB would likely disable this feature completely. Feel to provide a patch so that we could see how good/bad it is. Best Regards, Petr ^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2019-11-27 8:54 UTC | newest] Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-30 21:46 [PATCH] printf: add support for printing symbolic error codes Rasmus Villemoes 2019-08-30 21:53 ` Joe Perches 2019-08-30 22:03 ` Rasmus Villemoes 2019-08-30 22:21 ` Joe Perches 2019-08-30 22:50 ` Rasmus Villemoes 2019-09-02 9:07 ` David Laight 2019-08-31 9:38 ` kbuild test robot 2019-09-02 15:29 ` Uwe Kleine-König 2019-09-04 9:13 ` Rasmus Villemoes 2019-09-04 9:21 ` Uwe Kleine-König 2019-09-04 16:19 ` Andy Shevchenko 2019-09-04 16:28 ` Uwe Kleine-König 2019-09-05 11:40 ` Rasmus Villemoes 2019-09-09 20:38 ` [PATCH v2] " Rasmus Villemoes 2019-09-10 15:26 ` Andy Shevchenko 2019-09-11 0:15 ` Joe Perches 2019-09-11 6:43 ` Rasmus Villemoes 2019-09-11 9:37 ` Uwe Kleine-König 2019-09-11 10:14 ` Rasmus Villemoes 2019-09-15 9:43 ` Pavel Machek 2019-09-16 12:23 ` Uwe Kleine-König 2019-09-16 13:23 ` Rasmus Villemoes 2019-09-16 13:36 ` Uwe Kleine-König 2019-09-17 6:59 ` [PATCH v3] " Rasmus Villemoes 2019-09-25 14:36 ` Petr Mladek 2019-09-29 20:09 ` Rasmus Villemoes 2019-10-02 8:34 ` Petr Mladek 2019-10-05 21:48 ` Andrew Morton 2019-10-11 13:36 ` [PATCH v4 0/1] printf: add support for printing symbolic error names Rasmus Villemoes 2019-10-11 13:36 ` [PATCH v4 1/1] " Rasmus Villemoes 2019-10-14 5:51 ` Uwe Kleine-König 2019-10-14 13:02 ` Petr Mladek 2019-10-14 13:10 ` Andy Shevchenko 2019-10-15 12:17 ` Rasmus Villemoes 2019-10-15 13:44 ` David Laight 2019-10-15 19:07 ` [PATCH v5] " Rasmus Villemoes 2019-10-16 13:49 ` Andy Shevchenko 2019-10-16 14:52 ` Petr Mladek 2019-10-16 16:31 ` Andy Shevchenko 2019-10-17 15:02 ` Petr Mladek 2019-11-26 14:04 ` [PATCH v4 0/1] " Geert Uytterhoeven 2019-11-27 8:54 ` Petr Mladek
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.