linux-kernel - Re: [PATCH] lib/idr.c: remove duplicated bound checking in sub

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] [day] [month] [year] [list]

Message-ID: <CAMjEyscOTvjTo91kjGnKYtUd9s7bzapy0gnnMai=C1PB3NXMhw@mail.gmail.com>
Date:	Sat, 21 Mar 2015 15:47:08 +0800
From:	Liu Sha <rednoax@...il.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Liu Sha <rednoax@...com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] lib/idr.c: remove duplicated bound checking in sub_alloc

On Sat, Mar 21, 2015 at 5:28 AM, Andrew Morton
<akpm@...ux-foundation.org> wrote:
> On Thu, 19 Mar 2015 17:57:07 +0000 Liu Sha <rednoax@...com> wrote:
>
>> From: Liu Sha <rednoax@...il.com>
>>
>> The INT_MAX bound checking in sub_alloc checks two conditions to see
>> whether the signed integer "id" is beyond INT_MAX:
>>
>>               if ((id >= MAX_IDR_BIT) || (id < 0))
>>                       return -ENOSPC;
>>
>> These two conditions are actually the same for "int" variable so one
>> of them can be removed. If the above snippet is compiled with -Os option
>> of gcc, only one checking will remain in disassembly code.
>>
>> --- a/lib/idr.c
>> +++ b/lib/idr.c
>> @@ -262,7 +262,7 @@ static int sub_alloc(struct idr *idp, int *starting_id, struct idr_layer **pa,
>>                       sh = IDR_BITS*l;
>>                       id = ((id >> sh) ^ n ^ m) << sh;
>>               }
>> -             if ((id >= MAX_IDR_BIT) || (id < 0))
>> +             if (id >= MAX_IDR_BIT)
>>                       return -ENOSPC;
>>               if (l == 0)
>>                       break;
>
> Well.  This only works because MAX_IDR_BIT happens to have unsigned
> type, so the comparison is done with unsigned arithmetic.
>
> The patch makes no difference to code size with my gcc and I'm inclined
> to leave the code as-is for reasons of safety and clarity.
Thanks for your explaination. Because address "qq.com" of this mail is
among the "not liked source for email" of LKML and has been returned. I
wrote 2nd mail with the same patch using another email address. You may
have received two similar mail. Please ignore the 2nd.

I think it may be better to shrink this checking for the following reasons :)

1. two equivalent conditions is a little confusing. When I first see it, I know
what "id < 0" means but not 100% sure about "id > MAX_IDR_BIT". "id" is
signed while "MAX_IDR_BIT" is unsigned. So after test I belive it becomes
an unsigned comparsion, as you said. And its function is equivalent to "id < 0".
I tried two c files, using "id > MAX_IDR_BIT" and "id < 0" respectively, gcc
will interpreter both as "id < 0". So just one condition will be much clear to
let people know that it only want to filter "id < 0".

2. The top Makefile specifies either "-Os" or "-O2" for gcc. So there is not
any difference in generated assembly code in idr.lst, no matter which one of
these two conditions is removed.  The valid idr range is [0, INT_MAX], which
has been so since 2.6.12. There are some other places in idr.c checking INT_MAX.
They either use "> MAX_IDR_BIT" or "< 0", but not both. So only one is safe
enough.
--
Liu Sha
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/