From: Scott Wood <scott.wood@nxp.com> To: arvind Yadav <arvind.yadav.cs@gmail.com>, Arnd Bergmann <arnd@arndb.de>, "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org> Cc: "qiang.zhao@freescale.com" <qiang.zhao@freescale.com>, "viresh.kumar@linaro.org" <viresh.kumar@linaro.org>, "zajec5@gmail.com" <zajec5@gmail.com>, "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>, "David.Laight@aculab.com" <David.Laight@aculab.com>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "scottwood@freescale.com" <scottwood@freescale.com>, "akpm@linux-foundation.org" <akpm@linux-foundation.org>, "davem@davemloft.net" <davem@davemloft.net>, "linux@roeck-us.net" <linux@roeck-us.net>, Li Yang <leoli@freescale.com> Subject: Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems. Date: Tue, 2 Aug 2016 19:57:48 +0000 [thread overview] Message-ID: <DB5PR0401MB1928DE1F195A57160DED735D91050@DB5PR0401MB1928.eurprd04.prod.outlook.com> (raw) In-Reply-To: 57A0BD8E.9050305@gmail.com On 08/02/2016 10:34 AM, arvind Yadav wrote: > > > On Tuesday 02 August 2016 01:15 PM, Arnd Bergmann wrote: >> On Monday, August 1, 2016 4:55:43 PM CEST Scott Wood wrote: >>> On 08/01/2016 02:02 AM, Arnd Bergmann wrote: >>>>> diff --git a/include/linux/err.h b/include/linux/err.h >>>>> index 1e35588..c2a2789 100644 >>>>> --- a/include/linux/err.h >>>>> +++ b/include/linux/err.h >>>>> @@ -18,7 +18,17 @@ >>>>> >>>>> #ifndef __ASSEMBLY__ >>>>> >>>>> -#define IS_ERR_VALUE(x) unlikely((unsigned long)(void *)(x) >= (unsigned long)-MAX_ERRNO) >>>>> +#define IS_ERR_VALUE(x) unlikely(is_error_check(x)) >>>>> + >>>>> +static inline int is_error_check(unsigned long error) >>>> Please leave the existing macro alone. I think you were looking for >>>> something specific to the return code of qe_muram_alloc() function, >>>> so please add a helper in that subsystem if you need it, not in >>>> the generic header files. >>> qe_muram_alloc (a.k.a. cpm_muram_alloc) returns unsigned long. The >>> problem is certain callers that store the return value in a u32. Why >>> not just fix those callers to store it in unsigned long (at least until >>> error checking is done)? >>> >> Yes, that would also address another problem with code like >> >> kfree((void *)ugeth->tx_bd_ring_offset[i]); >> >> which is not 64-bit safe when tx_bd_ring_offset is a 32-bit value >> that also holds the return value of qe_muram_alloc. Well, hopefully it doesn't hold a return of qe_muram_alloc() when it's being passed to kfree()... There's also the code that casts kmalloc()'s return to u32, etc. ucc_geth is not 64-bit clean in general. >> >> Arnd > Yes, we will fix caller. Caller api is not safe on 64bit. The API is fine (or at least, I haven't seen a valid issue pointed out yet). The problem is the ucc_geth driver. > Even qe_muram_addr(a.k.a. cpm_muram_addr )passing value unsigned int, > but it should be unsigned long. cpm_muram_addr takes unsigned long as a parameter, not that it matters since you can't pass errors into it and a muram offset should never exceed 32 bits. -Scott
WARNING: multiple messages have this Message-ID (diff)
From: Scott Wood <scott.wood@nxp.com> To: arvind Yadav <arvind.yadav.cs@gmail.com>, Arnd Bergmann <arnd@arndb.de>, "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org> Cc: "qiang.zhao@freescale.com" <qiang.zhao@freescale.com>, "viresh.kumar@linaro.org" <viresh.kumar@linaro.org>, "zajec5@gmail.com" <zajec5@gmail.com>, "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>, "David.Laight@aculab.com" <David.Laight@aculab.com>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "scottwood@freescale.com" <scottwood@freescale.com>, "akpm@linux-foundation.org" <akpm@linux-foundation.org>, "davem@davemloft.net" <davem@davemloft.net>, "linux@roeck-us.net" <linux@roeck-us.net>, Li Yang <leoli@freescale.com> Subject: Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems. Date: Tue, 2 Aug 2016 19:57:48 +0000 [thread overview] Message-ID: <DB5PR0401MB1928DE1F195A57160DED735D91050@DB5PR0401MB1928.eurprd04.prod.outlook.com> (raw) In-Reply-To: 57A0BD8E.9050305@gmail.com On 08/02/2016 10:34 AM, arvind Yadav wrote:=0A= > =0A= > =0A= > On Tuesday 02 August 2016 01:15 PM, Arnd Bergmann wrote:=0A= >> On Monday, August 1, 2016 4:55:43 PM CEST Scott Wood wrote:=0A= >>> On 08/01/2016 02:02 AM, Arnd Bergmann wrote:=0A= >>>>> diff --git a/include/linux/err.h b/include/linux/err.h=0A= >>>>> index 1e35588..c2a2789 100644=0A= >>>>> --- a/include/linux/err.h=0A= >>>>> +++ b/include/linux/err.h=0A= >>>>> @@ -18,7 +18,17 @@=0A= >>>>> =0A= >>>>> #ifndef __ASSEMBLY__=0A= >>>>> =0A= >>>>> -#define IS_ERR_VALUE(x) unlikely((unsigned long)(void *)(x) >=3D (un= signed long)-MAX_ERRNO)=0A= >>>>> +#define IS_ERR_VALUE(x) unlikely(is_error_check(x))=0A= >>>>> +=0A= >>>>> +static inline int is_error_check(unsigned long error)=0A= >>>> Please leave the existing macro alone. I think you were looking for=0A= >>>> something specific to the return code of qe_muram_alloc() function,=0A= >>>> so please add a helper in that subsystem if you need it, not in=0A= >>>> the generic header files.=0A= >>> qe_muram_alloc (a.k.a. cpm_muram_alloc) returns unsigned long. The=0A= >>> problem is certain callers that store the return value in a u32. Why= =0A= >>> not just fix those callers to store it in unsigned long (at least until= =0A= >>> error checking is done)?=0A= >>>=0A= >> Yes, that would also address another problem with code like=0A= >>=0A= >> kfree((void *)ugeth->tx_bd_ring_offset[i]);=0A= >>=0A= >> which is not 64-bit safe when tx_bd_ring_offset is a 32-bit value=0A= >> that also holds the return value of qe_muram_alloc.=0A= =0A= Well, hopefully it doesn't hold a return of qe_muram_alloc() when it's=0A= being passed to kfree()...=0A= =0A= There's also the code that casts kmalloc()'s return to u32, etc.=0A= ucc_geth is not 64-bit clean in general.=0A= =0A= >>=0A= >> Arnd=0A= > Yes, we will fix caller. Caller api is not safe on 64bit.=0A= =0A= The API is fine (or at least, I haven't seen a valid issue pointed out=0A= yet). The problem is the ucc_geth driver.=0A= =0A= > Even qe_muram_addr(a.k.a. cpm_muram_addr )passing value unsigned int,=0A= > but it should be unsigned long.=0A= =0A= cpm_muram_addr takes unsigned long as a parameter, not that it matters=0A= since you can't pass errors into it and a muram offset should never=0A= exceed 32 bits.=0A= =0A= -Scott=0A= =0A=
next prev parent reply other threads:[~2016-08-02 20:29 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-07-31 11:18 [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems Arvind Yadav 2016-07-31 11:18 ` Arvind Yadav 2016-08-01 7:02 ` Arnd Bergmann 2016-08-01 7:02 ` Arnd Bergmann 2016-08-01 16:55 ` Scott Wood 2016-08-01 16:55 ` Scott Wood 2016-08-02 7:45 ` Arnd Bergmann 2016-08-02 7:45 ` Arnd Bergmann 2016-08-02 15:34 ` arvind Yadav 2016-08-02 15:34 ` arvind Yadav 2016-08-02 19:57 ` Scott Wood [this message] 2016-08-02 19:57 ` Scott Wood 2016-08-03 13:55 ` arvind Yadav 2016-08-02 15:48 ` arvind Yadav
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=DB5PR0401MB1928DE1F195A57160DED735D91050@DB5PR0401MB1928.eurprd04.prod.outlook.com \ --to=scott.wood@nxp.com \ --cc=David.Laight@aculab.com \ --cc=akpm@linux-foundation.org \ --cc=arnd@arndb.de \ --cc=arvind.yadav.cs@gmail.com \ --cc=davem@davemloft.net \ --cc=leoli@freescale.com \ --cc=linux-wireless@vger.kernel.org \ --cc=linux@roeck-us.net \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=netdev@vger.kernel.org \ --cc=qiang.zhao@freescale.com \ --cc=scottwood@freescale.com \ --cc=viresh.kumar@linaro.org \ --cc=zajec5@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.