From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753994AbcBBIXs (ORCPT ); Tue, 2 Feb 2016 03:23:48 -0500 Received: from mailout3.w1.samsung.com ([210.118.77.13]:51534 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753515AbcBBIXr (ORCPT ); Tue, 2 Feb 2016 03:23:47 -0500 X-AuditID: cbfec7f5-f79b16d000005389-e3-56b067915757 Subject: Re: [PATCH v2] err.h: allow IS_ERR_VALUE to handle properly more types To: Andrew Morton References: <201601072342.xanwLeaS%fengguang.wu@intel.com> <1453969648-4036-1-git-send-email-a.hajda@samsung.com> <20160201222351.dbbca353.akpm@linux-foundation.org> Cc: open list , Bartlomiej Zolnierkiewicz , Marek Szyprowski From: Andrzej Hajda Message-id: <56B0675E.8070309@samsung.com> Date: Tue, 02 Feb 2016 09:22:54 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-version: 1.0 In-reply-to: <20160201222351.dbbca353.akpm@linux-foundation.org> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrELMWRmVeSWpSXmKPExsVy+t/xy7oT0zeEGTTcYLWYs34Nm8XGGetZ LS7vmsNmsfbIXXYHFo8TM36zePRtWcXo8XmTXABzFJdNSmpOZllqkb5dAlfGkmV7WAsWSVW8 WLiOrYHxr0gXIyeHhICJxOGvy9ghbDGJC/fWs3UxcnEICSxllPg2/w07hPOcUaL9Zg8bSJWw gL/E7EWPmEFsEQFdiVXPd4HZQgIrGSUa1weCNDALTGaUWN91nxEkwSagKfF3802wZl4BLYkP cyaANbAIqEpMbmpiBbFFBSIkDnd2sUPUCEr8mHyPBcTmFHCQOP7uF1A9B9BQPYn7F7VAwswC 8hKb17xlnsAoMAtJxyyEqllIqhYwMq9iFE0tTS4oTkrPNdIrTswtLs1L10vOz93ECAnZrzsY lx6zOsQowMGoxMPbsXZ9mBBrYllxZe4hRgkOZiURXiGNDWFCvCmJlVWpRfnxRaU5qcWHGKU5 WJTEeWfueh8iJJCeWJKanZpakFoEk2Xi4JRqYFy7iHGVeuCTc93Mm9e3dqU3RfbIvK8MnZM0 /cqdzp1Hp/keseFUj5n+raZ8XdYz7VTts//4FFkZtv1r0d1/88YrJweFoPvRK5bld8XpFfyI amFXkY/71jrvt+P+3cJG/VOYXtqF//3JWqMt7Xx7anFNeOx7UcvuGuu488oOubKpEb+t+f3S lFiKMxINtZiLihMBitQJJlUCAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/02/2016 07:23 AM, Andrew Morton wrote: > On Thu, 28 Jan 2016 09:27:28 +0100 Andrzej Hajda 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. >> >> I have analyzed usage of IS_ERR_VALUE using coccinelle and in about 35 >> cases it is used incorrectly, ie it can hide errors depending of 32/64 bit >> architecture. Instead of fixing usage I propose to enhance the macro >> to cover more types. >> And just for the record: the macro is used 101 times with signed variables, >> I am not sure if it should be preferred over simple comparison "ret < 0", >> but the new version can do it as well. >> >> And below list of detected potential errors: >> >> ... >> >> --- 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) < 0) \ >> + : unlikely((x) >= (typeof(x))-MAX_ERRNO)) >> > hm, seems complicated. Can we simply cast the value to long? > > #define IS_ERR_VALUE(x) ((long)x < 0) && (long)x >= (long)-MAX_ERRNO) > > and simplify that to > > #define IS_ERR_VALUE(x) ((unsigned long)(long)x >= (unsigned long)-MAX_ERRNO) > > or something like that. It will not work with u32 on 64bit systems. Short rationales behind my implementation: 1. Typical usage pattern of the macro looks like: T x; ... x = -ESOME_ERROR; ... if (IS_ERR_VALUE(x)) ... In error assignment we have casting of -ESOME_ERROR to type T. Casting of -MAX_ERRNO to the same type in the macro assures that comparison will be sane, at least for types big enough. In short we ends at following expression (for unsigned types): (T)-ESOME_ERROR >= (T)-MAX_ERRNO In old implementation we ended at: (unsigned)(T)-ESOME_ERROR >= (unsigned)-MAX_ERRNO Different castings for -ESOME_ERROR and for -MAX_ERRNO makes this comparison incorrect for some types T. 2. Error checking is completely different for signed and unsigned vars: a. signed are compared to 0: ret < 0. b. unsigned are compared with some high value: ret >= (-MAX_ERRNO). This dualism is clearly visible and emphasized in this implementation. In old implementation IS_ERR_VALUE works correctly for some signed types due to obscure C casting rules. Summarizing: current implementation is short but tricky, answering why it works/fails for certain types is quite challenging. On the other side proposed implementation is longer but more straightforward, and of course is correct for more types :) Maybe, to make it more clear, it could be good to use separate macro for signedness: #define IS_SIGNED_TYPE(t) ((t)(-1) <= 0) #define IS_ERR_VALUE(x) (IS_SIGNED_TYPE(typeof(x)) \ ? unlikely((x) < 0) \ : unlikely((x) >= (typeof(x))-MAX_ERRNO)) Regards Andrzej