Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1331711 > unrolled thread

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types Andrzej Hajda <a.hajda@samsung.com> - 2016-02-11 08:10 +0100
    Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types Arnd Bergmann <arnd@arndb.de> - 2016-02-11 17:50 +0100
      Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types Andrzej Hajda <a.hajda@samsung.com> - 2016-02-12 15:50 +0100
    Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more  types Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-11 22:20 +0100

#1331711 — Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

On 02/10/2016 10:01 PM, Arnd Bergmann wrote:
> On Tuesday 09 February 2016 09:42:26 Andrzej Hajda wrote:
>> +cc Rasmus Villemoes, I forgot to add him earlier.
>>
>> On 02/08/2016 01:01 PM, Arnd Bergmann wrote:
>>> On Monday 08 February 2016 09:45:55 Andrzej Hajda wrote:
>>>> On 02/05/2016 11:52 AM, Arnd Bergmann wrote:
>>>>> On Thursday 04 February 2016 10:59:31 Andrew Morton wrote:
>>>> My version produces shortest code, Arnd's is the same as the old one.
>>>> On the other side Rasmus proposition seems to be the most straightforward
>>>> to me. Anyway I am not sure if the code length is the most important here.
>>>>
>>>> By the way .data segment size grows almost 4 times between gcc 4.4 and
>>>> 4.8 :)
>>>> Also numbers for arm64 looks interesting.
>>>>
>>>> Just for the record below all proposed implementations:
>>>> #define IS_ERR_VALUE_old(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
>>>> #define IS_ERR_VALUE_andrzej(x) ((typeof(x))(-1) <= 0 \
>>>>                                 ? unlikely((x) <= -1) \
>>>>                                 : unlikely((x) >= (typeof(x))-MAX_ERRNO))
>>>> #define IS_ERR_VALUE_arnd(x)      (unlikely((unsigned long long)(x) >=
>>>> (unsigned long long)(typeof(x))-MAX_ERRNO))
>>>> #define IS_ERR_VALUE_rasmus(x) ({\
>>>>         typeof(x) _x = (x);\
>>>>         unlikely(_x >= (typeof(x))-MAX_ERRNO &&  _x <= (typeof(x))-1);\
>>>> })
>>>>
>>>>> Andrzej's version is a little shorter on ARM because in case of signed numbers
>>>>> it only checks for negative values, rather than checking for values in the
>>>>> [-MAX_ERRNO..-1] range. I think the original behavior is more logical
>>>>> in this case, and my version restores it.
>>>> As I looked at the usage of the macro in the kernel I have not found any
>>>> code
>>>> which could benefit from the original behavior, except some buggy code in
>>>> staging which have already pending fix[1].
>>>> But maybe it would be better to use IS_ERR_VALUE to always check if err
>>>> is in
>>>> range [-MAX_ERRNO..-1] and just use simple 'err < 0' in typical case of
>>>> signed types.
>>> If we do that, should we also make it illegal to use an invalid type
>>> for IS_ERR()? At least that could also catch any use of 'char' and 'unsigned
>>> char' that are still broken.
>> I meant rather to make such 'policy' for future code by adding some
>> comment to the macro. Optionally adding compile time warning
>> to encourage developers to change current usage, however I am
>> not sure if it is not too harsh.
>> This way it could be also good to use your version of the macro.
>> It could be also good to add compiletime_assert to prevent char types
>> as suggested by Rasmus.
>>
>> Finally it could look like:
>> /*
>>  * Use IS_ERR_VALUE only on unsigned types of at least two bytes size.
>>  * For signed types use '< 0' comparison.
>>  */
>> #define IS_ERR_VALUE(x)\
>> ({\
>>         compiletime_assert(sizeof(x) > 1, "IS_ERR_VALUE does not handle
>> byte-size types");\
>>         compiletime_assert_warning((typeof(x))(-1) > 0, "IS_ERR_VALUE
>> should be called on unsigned types only, use '< 0' instead");\
>>         (unlikely((unsigned long long)(x) >= (unsigned long
>> long)(typeof(x))-MAX_ERRNO));\
>> })
>>
> I think the easiest way to express this would be to ensure that the argument
> is 'unsigned long', like:
>
> #define IS_ERR_VALUE(x) ((unsigned long*)NULL == (typeof (x)*)NULL && \
>        unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))

This way you will limit it only to unsigned long type, which seems too
strict to me.
I think the macro should accept all long enough unsigned types, otherwise we
could end up with bunch of macros IS_ERR_VALUE_U32, IS_ERR_VALUE_ULL...

Regards
Andrzej

[toc] | [next] | [standalone]

#1332231

On Thursday 11 February 2016 08:00:54 Andrzej Hajda wrote:
> > I think the easiest way to express this would be to ensure that the argument
> > is 'unsigned long', like:
> >
> > #define IS_ERR_VALUE(x) ((unsigned long*)NULL == (typeof (x)*)NULL && \
> >        unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
> 
> This way you will limit it only to unsigned long type, which seems too
> strict to me.
> I think the macro should accept all long enough unsigned types, otherwise we
> could end up with bunch of macros IS_ERR_VALUE_U32, IS_ERR_VALUE_ULL...

I think in practice we only care about 'int' and 'unsigned long', which are
the ones that 90% of the existing users pass in today. u32 has never worked
on 64-bit architectures so far, so we don't necessarily have to make it work.
As Al mentioned, most users of IS_ERR_VALUE are wrong anyway and should
just use 'if (err < 0)' or 'if (err)'.

We could also consider making just 'int' and 'unsigned long' allowed types
for the moment, and then change all users passing 'int' before forbidding them.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1332791

On 02/11/2016 05:39 PM, Arnd Bergmann wrote:
> On Thursday 11 February 2016 08:00:54 Andrzej Hajda wrote:
>>> I think the easiest way to express this would be to ensure that the argument
>>> is 'unsigned long', like:
>>>
>>> #define IS_ERR_VALUE(x) ((unsigned long*)NULL == (typeof (x)*)NULL && \
>>>        unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
>> This way you will limit it only to unsigned long type, which seems too
>> strict to me.
>> I think the macro should accept all long enough unsigned types, otherwise we
>> could end up with bunch of macros IS_ERR_VALUE_U32, IS_ERR_VALUE_ULL...
> I think in practice we only care about 'int' and 'unsigned long', which are
> the ones that 90% of the existing users pass in today. u32 has never worked
> on 64-bit architectures so far, so we don't necessarily have to make it work.
> As Al mentioned, most users of IS_ERR_VALUE are wrong anyway and should
> just use 'if (err < 0)' or 'if (err)'.
>
> We could also consider making just 'int' and 'unsigned long' allowed types
> for the moment, and then change all users passing 'int' before forbidding them.
>
> 	Arnd
>
>

OK so in short we need to fix 140 usages of the macro? Who should do them?
I can create cocci patch for more obvious cases. What about these less
obvious?
As I understand we do not touch the macro till fixes are merged?

Regards
Andrzej

[toc] | [prev] | [next] | [standalone]

#1332370 — Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

On Thu, Feb 11, 2016 at 08:00:54AM +0100, Andrzej Hajda wrote:

> This way you will limit it only to unsigned long type, which seems too
> strict to me.
> I think the macro should accept all long enough unsigned types, otherwise we
> could end up with bunch of macros IS_ERR_VALUE_U32, IS_ERR_VALUE_ULL...

... or use it a whole lot less.  Which is a Good Thing(tm), because it
corrects the damage caused by seriously flawed concept of "it's a bad
value, mmkay?" kind of predicate that could be used in all situations,
without having to look at the calling conventions of the functions
used to produce the value.  It doesn't work and it has already spread
around too much.

For starters, it conflates "0 on success, -E... on error" with "non-zero
on success, 0 on failure" with "-E... on error, 0 or positive on success"
with "positive on success, non-positive on error" with "address of some
object or -E..., 0 to be treated as -ENOMEM", etc.

*All* of those are present in the kernel.  Promoting blind use of magic
macro will keep causing bugs, and extra degree of polymorphism will only
encourage such blind use.

Look at the actual users.  IS_ERR() aside (and don't get me started on
the abortion that is IS_ERR_OR_NULL()), there is one more or less common
legitimate use.  Treatement of vm_mmap()/do_mmap_pgoff()/etc. return values.
The rest is very mixed bag.  To pick a random one (in net/9p/client.c):
                ename = NULL;
                err = p9pdu_readf(req->rc, c->proto_version, "s?d",
                                  &ename, &ecode);
                if (err)
                        goto out_err;

                if (p9_is_proto_dotu(c))
                        err = -ecode;

                if (!err || !IS_ERR_VALUE(err)) {
                        err = p9_errstr2errno(ename, strlen(ename));

                        p9_debug(P9_DEBUG_9P, "<<< RERROR (%d) %s\n",
                                 -ecode, ename);
                }
                kfree(ename);

What's going on here?  We have an RERROR or RLERROR reply coming from server.
We want to convert that to kernel-recognizable error value.  Normal 9P
uses strings for errors; kernel uses numbers.  String is represented as
16bit little-endian length + that many characters; that's all that plain
9P puts into RERROR payload.  9P.U (unix extensions) appends suggested 32bit
little-endian numeric value after that.  9P.L simply puts the 32bit l-e
numeric value, with no strings involved.

The quoted code deals with 9P and 9P.U.  Response is parsed (FWIW, '?' in
format is "ignore the rest for plain 9P"), any failure is returned as
an error in its own right, then for 9P.U we look at the numeric value
and if it's from 1 to MAX_ERRNO we just accept that.  Otherwise (plain
9P or a strange numeric value in 9P.U) we look at the string part and
convert it to E...  For 9P.L we simply accept the error value given.

Incidentally, that use of numeric values assumes that all relevant error
values will have the same encoding on all architectures - 9P is a network
protocol, after all.  <checks the list of error values used> Uh-oh...
        {"Resource temporarily unavailable", EAGAIN},
        {"exclusive use file already open", EAGAIN},
        {"file is in use", EAGAIN},
... and while the normal value of EAGAIN is 11, on alpha it's 35.  Oops...
It also contains a lot more values than arch-consistent subset in errno-base.h
- there are only 34 in the latter (and some architectures redefine some of
those, like alpha does to EAGAIN) and there's more than twice as much in the
former...  Some of those are fairly unlikely to be generated by server, but
e.g. ENAMETOOLONG (63 on alpha and sparc, 78 on mips, 248 on parisc, 36 on
everything else) is not impossible for a fileserver at all.  Looks like
trouble...

Further investigation belongs on 9P list, but in this case a blind use of
IS_ERR_VALUE() has turned out to be an indication of a bug.  QED.  Any such
places need careful review, and no amount of magic will avoid the need to
understand the surrounding code.  I'd be seriously tempted to add "uses
IS_ERR_VALUE outside of few known-good places" to checkpatch.pl, if not
for the fact that inevitable flood of mindless "fixes" is precisely what
we do *NOT* need in this case...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web