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

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: <354dc1f5-45b8-9e51-1ba0-b1fd368be45a@rasmusvillemoes.dk>
Date:   Wed, 11 Sep 2019 08:43:21 +0200
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Joe Perches <joe@...ches.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Jonathan Corbet <corbet@....net>,
        Petr Mladek <pmladek@...e.com>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Uwe Kleine-König <uwe@...ine-koenig.org>,
        Linux Documentation List <linux-doc@...r.kernel.org>
Subject: Re: [PATCH v2] printf: add support for printing symbolic error codes

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@...musvillemoes.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