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

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: <877fiknj37.fsf@rasmusvillemoes.dk>
Date:	Fri, 05 Feb 2016 00:37:00 +0100
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	Andrzej Hajda <a.hajda@...sung.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	linux-kernel@...r.kernel.org (open list)
Subject: Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

On Wed, Feb 03 2016, Andrzej Hajda <a.hajda@...sung.com> wrote:

> Current implementation of IS_ERR_VALUE works correctly only with
> following types:
> - unsigned long,
> - short, int, long.
> Other types are handled incorrectly either on 32-bit either on 64-bit
> either on both architectures.
> The patch fixes it by comparing argument with MAX_ERRNO casted
> to argument's type for unsigned types and comparing with zero for signed
> types. As a result all integer types bigger than char are handled properly.
>
>
> Signed-off-by: Andrzej Hajda <a.hajda@...sung.com>
> ---
> v3:
>  - use '<= -1' instead of '< 0' to silence verbose warnings for gcc
>    older than 4.8,
> v2:
>  - use '<= 0' instead of '< 0' to silence gcc verbose warnings,
>  - expand commit message.
> ---
>  include/linux/err.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/err.h b/include/linux/err.h
> index 56762ab..b7d4a9f 100644
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -18,7 +18,9 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
> +				? unlikely((x) <= -1) \
> +				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
>  

I'm a bit worried that you consider any negative value an error when x
is signed - at least that's a change which deserves some comment why
that's ok. For example, I could imagine someone using e.g. INT_MIN as a
sentinel return value meaning 'not an error, but something special
still'.

I think that, since we're there, one should stick in a BUILD_BUG_ON to
catch people passing in a u8 or s8.

Something that seems that it would work for all (wide enough) types
is

({
typeof(x) _x = (x);
BUILD_BUG_ON(sizeof(_x) < 2);
unlikely(_x >= (typeof(x))-MAX_ERRNO &&  _x <= (typeof(x))-1);
})

Whether x is signed or not we want the first condition. If x is unsigned
and at least as wide as int, the -1 in the second condition will be
promoted to the max value typeof(x) can hold, so gcc should trivially
remove the check, and if x is signed, this keeps the previous
semantics. It's also a pattern I expect gcc to be able to optimize
pretty well (that is, if x has type signed int, I expect it'll recognize
that these tests are better done as a single unsigned comparison). Maybe
it'll still give a -Wtype-limit warning for the unsigned case.

If we go with the above, maybe one should also stick in something which
ensures x is not a pointer; (void)(_x+_x); should do it. 

Rasmus