[v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems.

[v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems.

[Arvind Yadav]

IS_ERR_VALUE() assumes that parameter is an unsigned long.
It can not be used to check if 'unsigned int' is passed insted.
Which tends to reflect an error.

In 64bit architectures sizeof (int) == 4 && sizeof (long) == 8.
IS_ERR_VALUE(x) is ((x) >= (unsigned long)-4095).

IS_ERR_VALUE() of 'unsigned int' is always false because the 32bit
value is zero extended to 64 bits.

Value of (unsigned int)-4095 is always less than value of
(unsigned long)-4095.

Now We are taking only first 32 bit for error checking rest of the 32 bit
we ignore such that we get appropriate comparison on 64bit system as well.

First 32bit of Value of (unsigned int)-4095 and (unsigned long)-4095 will
be equal.

Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
 include/linux/err.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

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)
+{
+	unsigned int first32bit = (error & 0xFFFFFFFF);
+
+	if (first32bit >= (unsigned int)-MAX_ERRNO)
+		return 1;
+	else
+		return 0;
+}
 
 static inline void * __must_check ERR_PTR(long error)
 {
-- 
1.9.1

[v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems.

[Arvind Yadav]

IS_ERR_VALUE() assumes that parameter is an unsigned long.
It can not be used to check if 'unsigned int' is passed insted.
Which tends to reflect an error.

In 64bit architectures sizeof (int) == 4 && sizeof (long) == 8.
IS_ERR_VALUE(x) is ((x) >= (unsigned long)-4095).

IS_ERR_VALUE() of 'unsigned int' is always false because the 32bit
value is zero extended to 64 bits.

Value of (unsigned int)-4095 is always less than value of
(unsigned long)-4095.

Now We are taking only first 32 bit for error checking rest of the 32 bit
we ignore such that we get appropriate comparison on 64bit system as well.

First 32bit of Value of (unsigned int)-4095 and (unsigned long)-4095 will
be equal.

Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
 include/linux/err.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

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)
+{
+	unsigned int first32bit = (error & 0xFFFFFFFF);
+
+	if (first32bit >= (unsigned int)-MAX_ERRNO)
+		return 1;
+	else
+		return 0;
+}
 
 static inline void * __must_check ERR_PTR(long error)
 {
-- 
1.9.1

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems.

[Arnd Bergmann]

On Sunday, July 31, 2016 4:48:44 PM CEST Arvind Yadav wrote:
> IS_ERR_VALUE() assumes that parameter is an unsigned long.
> It can not be used to check if 'unsigned int' is passed insted.
> Which tends to reflect an error.
> 
> In 64bit architectures sizeof (int) == 4 && sizeof (long) == 8.
> IS_ERR_VALUE(x) is ((x) >= (unsigned long)-4095).
> 
> IS_ERR_VALUE() of 'unsigned int' is always false because the 32bit
> value is zero extended to 64 bits.
> 
> Value of (unsigned int)-4095 is always less than value of
> (unsigned long)-4095.
> 
> Now We are taking only first 32 bit for error checking rest of the 32 bit
> we ignore such that we get appropriate comparison on 64bit system as well.

This is completely wrong: if you have a valid 64-bit pointer like
0x00001234ffffff00, this will be interpreted as an error now.

> First 32bit of Value of (unsigned int)-4095 and (unsigned long)-4095 will
> be equal.
> 
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
> ---
>  include/linux/err.h | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> 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.

	Arnd

Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems.

[Arnd Bergmann]

On Sunday, July 31, 2016 4:48:44 PM CEST Arvind Yadav wrote:
> IS_ERR_VALUE() assumes that parameter is an unsigned long.
> It can not be used to check if 'unsigned int' is passed insted.
> Which tends to reflect an error.
> 
> In 64bit architectures sizeof (int) == 4 && sizeof (long) == 8.
> IS_ERR_VALUE(x) is ((x) >= (unsigned long)-4095).
> 
> IS_ERR_VALUE() of 'unsigned int' is always false because the 32bit
> value is zero extended to 64 bits.
> 
> Value of (unsigned int)-4095 is always less than value of
> (unsigned long)-4095.
> 
> Now We are taking only first 32 bit for error checking rest of the 32 bit
> we ignore such that we get appropriate comparison on 64bit system as well.

This is completely wrong: if you have a valid 64-bit pointer like
0x00001234ffffff00, this will be interpreted as an error now.

> First 32bit of Value of (unsigned int)-4095 and (unsigned long)-4095 will
> be equal.
> 
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
> ---
>  include/linux/err.h | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> 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.

	Arnd
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems.

[Scott Wood]

On 08/01/2016 02:02 AM, Arnd Bergmann wrote:
> On Sunday, July 31, 2016 4:48:44 PM CEST Arvind Yadav wrote:
>> IS_ERR_VALUE() assumes that parameter is an unsigned long.
>> It can not be used to check if 'unsigned int' is passed insted.
>> Which tends to reflect an error.
>>
>> In 64bit architectures sizeof (int) == 4 && sizeof (long) == 8.
>> IS_ERR_VALUE(x) is ((x) >= (unsigned long)-4095).
>>
>> IS_ERR_VALUE() of 'unsigned int' is always false because the 32bit
>> value is zero extended to 64 bits.
>>
>> Value of (unsigned int)-4095 is always less than value of
>> (unsigned long)-4095.
>>
>> Now We are taking only first 32 bit for error checking rest of the 32 bit
>> we ignore such that we get appropriate comparison on 64bit system as well.
> 
> This is completely wrong: if you have a valid 64-bit pointer like
> 0x00001234ffffff00, this will be interpreted as an error now.
> 
>> First 32bit of Value of (unsigned int)-4095 and (unsigned long)-4095 will
>> be equal.
>>
>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
>> ---
>>  include/linux/err.h | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> 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)?

-Scott

Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems.

[Scott Wood]

On 08/01/2016 02:02 AM, Arnd Bergmann wrote:=0A=
> On Sunday, July 31, 2016 4:48:44 PM CEST Arvind Yadav wrote:=0A=
>> IS_ERR_VALUE() assumes that parameter is an unsigned long.=0A=
>> It can not be used to check if 'unsigned int' is passed insted.=0A=
>> Which tends to reflect an error.=0A=
>>=0A=
>> In 64bit architectures sizeof (int) =3D=3D 4 && sizeof (long) =3D=3D 8.=
=0A=
>> IS_ERR_VALUE(x) is ((x) >=3D (unsigned long)-4095).=0A=
>>=0A=
>> IS_ERR_VALUE() of 'unsigned int' is always false because the 32bit=0A=
>> value is zero extended to 64 bits.=0A=
>>=0A=
>> Value of (unsigned int)-4095 is always less than value of=0A=
>> (unsigned long)-4095.=0A=
>>=0A=
>> Now We are taking only first 32 bit for error checking rest of the 32 bi=
t=0A=
>> we ignore such that we get appropriate comparison on 64bit system as wel=
l.=0A=
> =0A=
> This is completely wrong: if you have a valid 64-bit pointer like=0A=
> 0x00001234ffffff00, this will be interpreted as an error now.=0A=
> =0A=
>> First 32bit of Value of (unsigned int)-4095 and (unsigned long)-4095 wil=
l=0A=
>> be equal.=0A=
>>=0A=
>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>=0A=
>> ---=0A=
>>  include/linux/err.h | 12 +++++++++++-=0A=
>>  1 file changed, 11 insertions(+), 1 deletion(-)=0A=
>>=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 (unsig=
ned 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=
> =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=
=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=
-Scott=0A=
=0A=

Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems.

[Arnd Bergmann]

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

Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems.

[Arnd Bergmann]

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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems.

[arvind Yadav]

[-- Attachment #1.1: Type: text/plain, Size: 1523 bytes --]



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.



[-- Attachment #1.2: Type: text/html, Size: 2373 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems.

[arvind Yadav]

[-- Attachment #1: Type: text/plain, Size: 1523 bytes --]



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.



[-- Attachment #2: Type: text/html, Size: 2373 bytes --]

Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems.

[arvind Yadav]

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. Need to work on it.

Arvind

Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems.

[Scott Wood]

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

Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems.

[Scott Wood]

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=

Re: [v4] Fix to avoid IS_ERR_VALUE and IS_ERR abuses on 64bit systems.

[arvind Yadav]

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