From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-x241.google.com (mail-pf0-x241.google.com [IPv6:2607:f8b0:400e:c00::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3s4F2S2ZlwzDqQn for ; Wed, 3 Aug 2016 23:55:28 +1000 (AEST) Received: by mail-pf0-x241.google.com with SMTP id i6so14737581pfe.0 for ; Wed, 03 Aug 2016 06:55:28 -0700 (PDT) Subject: Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems. To: Scott Wood , Arnd Bergmann , "linuxppc-dev@lists.ozlabs.org" References: <1469963924-8800-1-git-send-email-arvind.yadav.cs@gmail.com> <1956647.cOmaJREgOE@wuerfel> <2484583.95xFmO7xir@wuerfel> <57A0BD8E.9050305@gmail.com> Cc: "qiang.zhao@freescale.com" , "viresh.kumar@linaro.org" , "zajec5@gmail.com" , "linux-wireless@vger.kernel.org" , "David.Laight@aculab.com" , "netdev@vger.kernel.org" , "scottwood@freescale.com" , "akpm@linux-foundation.org" , "davem@davemloft.net" , "linux@roeck-us.net" , Li Yang From: arvind Yadav Message-ID: <57A1F7C4.1090302@gmail.com> Date: Wed, 3 Aug 2016 19:25:16 +0530 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Wednesday 03 August 2016 01:27 AM, Scott Wood wrote: > 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 Yes, It will work for 32bit machine. But will not safe for 64bit. Example : ugeth->tx_bd_ring_offset[j] = qe_muram_alloc(length UCC_GETH_TX_BD_RING_ALIGNMENT); if (!IS_ERR_VALUE(ugeth->tx_bd_ring_offset[j])) ugeth->p_tx_bd_ring[j] = (u8 __iomem *) qe_muram_addr(ugeth-> tx_bd_ring_offset[j]); If qe_muram_alloc will return any error, IS_ERR_VALUE will always return 0 (IS_ERR_VALUE will always pass for 'unsigned int'). Now qe_muram_addr will return wrong virtual address. Which can cause an error. -Arvind