linux-kernel - Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20190824165829.7d330367992c62dab87f6652@linux-foundation.org>
Date:   Sat, 24 Aug 2019 16:58:29 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Uwe Kleine-König <uwe@...ine-koenig.org>
Cc:     Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Petr Mladek <pmladek@...e.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants


(cc printk maintainers).

On Sun, 25 Aug 2019 01:37:23 +0200 Uwe Kleine-König <uwe@...ine-koenig.org> wrote:

> 	pr_info("probing failed (%dE)\n", ret);
> 
> expands to
> 
> 	probing failed (EIO)
> 
> if ret holds -EIO (or EIO). This introduces an array of error codes. If
> the error code is missing, %dE falls back to %d and so prints the plain
> number.

Huh.  I'm surprised we don't already have this.  Seems that this will
be applicable in a lot of places?  Although we shouldn't go blindly
converting everything in sight - that would risk breaking userspace
which parses kernel strings.

Is it really necessary to handle the positive errnos?  Does much kernel
code actually do that (apart from kernel code which is buggy)?

> Signed-off-by: Uwe Kleine-König <uwe@...ine-koenig.org>
> ---
> Hello
> 
> there are many code sites that benefit from this. Just grep for
> "(%d)" ...

Yup.  This observation shouldn't be below the "^---$" ;) An approximate
grep|wc would be interesting.

> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -533,6 +533,192 @@ char *number(char *buf, char *end, unsigned long long num,
>  	return buf;
>  }
>  
> +#define ERRORCODE(x) { .str = #x, .err = x }
> +
> +static const struct {
> +	const char *str;
> +	int err;
> +} errorcodes[] = {

It's a bit of a hack, but an array of char*'s and a separate array of
ushorts would save a bit of space.

> +	ERRORCODE(EPERM),
> +	ERRORCODE(ENOENT),
> +	ERRORCODE(ESRCH),
>
> ...
>
> +static noinline_for_stack

Why this?  I'm suspecting this will actually increase stack use?

> +char *errstr(char *buf, char *end, unsigned long long num,
> +	     struct printf_spec spec)
> +{
> +	char *errname = NULL;
> +	size_t errnamelen, copy;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(errorcodes); ++i) {
> +		if (num == errorcodes[i].err || num == -errorcodes[i].err) {
> +			errname = errorcodes[i].str;
> +			break;
> +		}
> +	}
> +
> +	if (!errname) {
> +		/* fall back to ordinary number */
> +		return number(buf, end, num, spec);
> +	}
> +
> +	copy = errnamelen = strlen(errname);
> +	if (copy > end - buf)
> +		copy = end - buf;
> +	buf = memcpy(buf, errname, copy);
> +
> +	return buf + errnamelen;
> +}

OK, I guess `errstr' is an OK name for a static function and we can use
this to add a new strerror() should the need arise.

>
> ...
>