From mboxrd@z Thu Jan 1 00:00:00 1970 From: arvind Yadav Subject: Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems. Date: Tue, 2 Aug 2016 21:04:38 +0530 Message-ID: <57A0BD8E.9050305@gmail.com> References: <1469963924-8800-1-git-send-email-arvind.yadav.cs@gmail.com> <1956647.cOmaJREgOE@wuerfel> <2484583.95xFmO7xir@wuerfel> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1941726227298117907==" Cc: "qiang.zhao@freescale.com" , "viresh.kumar@linaro.org" , "zajec5@gmail.com" , "linux-wireless@vger.kernel.org" , Scott Wood , "David.Laight@aculab.com" , "netdev@vger.kernel.org" , "scottwood@freescale.com" , "akpm@linux-foundation.org" , "davem@davemloft.net" , "linux@roeck-us.net" To: Arnd Bergmann , linuxppc-dev@lists.ozlabs.org Return-path: In-Reply-To: <2484583.95xFmO7xir@wuerfel> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@lists.ozlabs.org Sender: "Linuxppc-dev" List-Id: netdev.vger.kernel.org This is a multi-part message in MIME format. --===============1941726227298117907== Content-Type: multipart/alternative; boundary="------------030904010008080602080808" This is a multi-part message in MIME format. --------------030904010008080602080808 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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. > > Arnd Yes, we will fix caller. Caller api is not safe on 64bit. Even qe_muram_addr(a.k.a. cpm_muram_addr )passing value unsigned int, but it should be unsigned long. --------------030904010008080602080808 Content-Type: text/html; charset=windows-1252 Content-Transfer-Encoding: 7bit

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.

	Arnd

    

    Yes, we will fix caller. Caller api is not safe on 64bit. 

    Even qe_muram_addr(a.k.a. cpm_muram_addr )passing value unsigned
    int,

    but it should be unsigned long.

    
    
    

    

  


--------------030904010008080602080808--

--===============1941726227298117907==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline

X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KTGludXhwcGMt
ZGV2IG1haWxpbmcgbGlzdApMaW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9yZwpodHRwczovL2xp
c3RzLm96bGFicy5vcmcvbGlzdGluZm8vbGludXhwcGMtZGV2

--===============1941726227298117907==--

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: 
Received: from mail-pf0-x235.google.com (mail-pf0-x235.google.com
 [IPv6:2607:f8b0:400e:c00::235])
 (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 3s3gHT5Y1HzDqQ3
 for ; Wed,  3 Aug 2016 01:34:45 +1000 (AEST)
Received: by mail-pf0-x235.google.com with SMTP id p64so67605941pfb.1
 for ; Tue, 02 Aug 2016 08:34:45 -0700 (PDT)
Subject: Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems.
To: 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>
Cc: Scott Wood ,
 "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" 
From: arvind Yadav 
Message-ID: <57A0BD8E.9050305@gmail.com>
Date: Tue, 2 Aug 2016 21:04:38 +0530
MIME-Version: 1.0
In-Reply-To: <2484583.95xFmO7xir@wuerfel>
Content-Type: multipart/alternative;
 boundary="------------030904010008080602080808"
List-Id: Linux on PowerPC Developers Mail List 
List-Unsubscribe: ,
 
List-Archive: 
List-Post: 
List-Help: 
List-Subscribe: ,
 

This is a multi-part message in MIME format.
--------------030904010008080602080808
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit



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.
>
> 	Arnd
Yes, we will fix caller. Caller api is not safe on 64bit.
Even qe_muram_addr(a.k.a. cpm_muram_addr )passing value unsigned int,
but it should be unsigned long.



--------------030904010008080602080808
Content-Type: text/html; charset=windows-1252
Content-Transfer-Encoding: 7bit


  
    
  
  
    

    

    
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.

	Arnd

    

    Yes, we will fix caller. Caller api is not safe on 64bit. 

    Even qe_muram_addr(a.k.a. cpm_muram_addr )passing value unsigned
    int,

    but it should be unsigned long.

    
    
    

    

  


--------------030904010008080602080808--