| 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
| ||
|
Message-id: <56B855C3.5010006@samsung.com>
Date: Mon, 08 Feb 2016 09:45:55 +0100
From: Andrzej Hajda <a.hajda@...sung.com>
To: Arnd Bergmann <arnd@...db.de>,
Andrew Morton <akpm@...ux-foundation.org>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
Marek Szyprowski <m.szyprowski@...sung.com>,
open list <linux-kernel@...r.kernel.org>,
Bob Peterson <rpeterso@...hat.com>
Subject: Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types
On 02/05/2016 11:52 AM, Arnd Bergmann wrote:
> On Thursday 04 February 2016 10:59:31 Andrew Morton wrote:
>> On Thu, 04 Feb 2016 13:40:38 +0100 Arnd Bergmann <arnd@...db.de> wrote:
>>
>>> diff --git a/include/linux/err.h b/include/linux/err.h
>>> index b7d4a9ff6342..bd4936a2c352 100644
>>> --- a/include/linux/err.h
>>> +++ b/include/linux/err.h
>>> @@ -18,9 +18,7 @@
>>>
>>> #ifndef __ASSEMBLY__
>>>
>>> -#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
>>> - ? unlikely((x) <= -1) \
>>> - : unlikely((x) >= (typeof(x))-MAX_ERRNO))
>>> +#define IS_ERR_VALUE(x) (unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
>>>
>>> static inline void * __must_check ERR_PTR(long error)
>>> {
>>>
>>>
>>> I'm not sure if the cast to 'unsigned long long' might cause less
>>> efficient code to be generated by gcc. I would hope that it is smart
>>> enough to not actually extend shorter variables to 64 bit before
>>> doing the comparison but I have not checked yet.
>> I did a quick test with i386 on drivers/nvmem/core.o. The patch takes
>> the text size from 9098 bytes to 9133. That file has 11 instances of
>> IS_ERR_VALUE().
> This seems to be because it brings back the logic to what it was before
> in case of 'int' arguments. I checked the assembly output and found mine
> to be identical to v4.4 in this case:
>
> text data bss dec hex filename
> v4.4 9942 1872 2856 14670 394e drivers/nvmem/core.o
> a.hajda 9922 1872 2856 14650 393a drivers/nvmem/core.o
> arnd 9942 1872 2856 14670 394e drivers/nvmem/core.o
I have compared all proposed version with all compilers I have:
text data bss dec hex filename
gcc-4.4
old 8188 1016 2968 12172 2f8c
.x86/drivers/nvmem/core.o
andrzej 8155 1016 2968 12139 2f6b
.x86/drivers/nvmem/core.o
arnd 8188 1016 2968 12172 2f8c
.x86/drivers/nvmem/core.o
rasmus 8266 1016 2968 12250 2fda
.x86/drivers/nvmem/core.o
---
gcc-4.7
old 7642 3816 3248 14706 3972
.x86/drivers/nvmem/core.o
andrzej 7606 3816 3248 14670 394e
.x86/drivers/nvmem/core.o
arnd 7642 3816 3248 14706 3972
.x86/drivers/nvmem/core.o
rasmus 7719 3816 3248 14783 39bf
.x86/drivers/nvmem/core.o
---
gcc-4.8
old 7735 3888 3272 14895 3a2f
.x86/drivers/nvmem/core.o
andrzej 7698 3888 3272 14858 3a0a
.x86/drivers/nvmem/core.o
arnd 7735 3888 3272 14895 3a2f
.x86/drivers/nvmem/core.o
rasmus 7812 3888 3272 14972 3a7c
.x86/drivers/nvmem/core.o
---
arm-linux-gnueabi-gcc-4.7
old 12776 1680 3432 17888 45e0
.arm/drivers/nvmem/core.o
andrzej 12772 1680 3432 17884 45dc
.arm/drivers/nvmem/core.o
arnd 12776 1680 3432 17888 45e0
.arm/drivers/nvmem/core.o
rasmus 12948 1680 3432 18060 468c
.arm/drivers/nvmem/core.o
---
aarch64-linux-gnu-gcc-4.8
old 5967 440 48 6455 1937
.arm64/drivers/nvmem/core.o
andrzej 5947 440 48 6435 1923
.arm64/drivers/nvmem/core.o
arnd 5967 440 48 6455 1937
.arm64/drivers/nvmem/core.o
rasmus 5991 440 48 6479 194f
.arm64/drivers/nvmem/core.o
---
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.
[1]: http://permalink.gmane.org/gmane.comp.file-systems.lustre.devel/4164
Regards
Andrzej
Powered by blists - more mailing lists