[PATCH] arch/tile: fix futex sanitization definition/prototype mismatch

[PATCH] arch/tile: fix futex sanitization definition/prototype mismatch

[Chris Metcalf]

Commit 8d7718aa082aaf30a0b4989e1f04858952f941bc changed "int"
to "u32" in the prototypes but not the definition.
I missed this when I saw the patch go by on LKML.

We cast "u32 *" to "int *" since we are tying into the underlying
atomics framework, and atomic_t uses int as its value type.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
 arch/tile/lib/atomic_32.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/tile/lib/atomic_32.c b/arch/tile/lib/atomic_32.c
index f02040d..4657021 100644
--- a/arch/tile/lib/atomic_32.c
+++ b/arch/tile/lib/atomic_32.c
@@ -202,32 +202,32 @@ static inline int *__futex_setup(int __user *v)
 	return __atomic_hashed_lock((int __force *)v);
 }
 
-struct __get_user futex_set(int __user *v, int i)
+struct __get_user futex_set(u32 __user *v, int i)
 {
 	return __atomic_xchg((int __force *)v, __futex_setup(v), i);
 }
 
-struct __get_user futex_add(int __user *v, int n)
+struct __get_user futex_add(u32 __user *v, int n)
 {
 	return __atomic_xchg_add((int __force *)v, __futex_setup(v), n);
 }
 
-struct __get_user futex_or(int __user *v, int n)
+struct __get_user futex_or(u32 __user *v, int n)
 {
 	return __atomic_or((int __force *)v, __futex_setup(v), n);
 }
 
-struct __get_user futex_andn(int __user *v, int n)
+struct __get_user futex_andn(u32 __user *v, int n)
 {
 	return __atomic_andn((int __force *)v, __futex_setup(v), n);
 }
 
-struct __get_user futex_xor(int __user *v, int n)
+struct __get_user futex_xor(u32 __user *v, int n)
 {
 	return __atomic_xor((int __force *)v, __futex_setup(v), n);
 }
 
-struct __get_user futex_cmpxchg(int __user *v, int o, int n)
+struct __get_user futex_cmpxchg(u32 __user *v, int o, int n)
 {
 	return __atomic_cmpxchg((int __force *)v, __futex_setup(v), o, n);
 }
-- 
1.6.5.2

Re: [PATCH] arch/tile: fix futex sanitization definition/prototype mismatch

[Michel Lespinasse]

On Sat, Mar 19, 2011 at 8:45 AM, Chris Metcalf <cmetcalf@tilera.com> wrote:
> Commit 8d7718aa082aaf30a0b4989e1f04858952f941bc changed "int"
> to "u32" in the prototypes but not the definition.
> I missed this when I saw the patch go by on LKML.
>
> We cast "u32 *" to "int *" since we are tying into the underlying
> atomics framework, and atomic_t uses int as its value type.
>
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>

Reviewed-by: Michel Lespinasse <walken@google.com>

Looks fine. I had held off from changing types within
futex_atomic_op_inuser() because the signed-ness of oparg and cmparg
was important there, but I agree changing at least the user pointer
types in futex_* operations is nice and safe.

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.

Re: [PATCH] arch/tile: fix futex sanitization definition/prototype mismatch

[Darren Hart]

On 03/19/2011 08:45 AM, Chris Metcalf wrote:
> Commit 8d7718aa082aaf30a0b4989e1f04858952f941bc changed "int"
> to "u32" in the prototypes but not the definition.
> I missed this when I saw the patch go by on LKML.
>
> We cast "u32 *" to "int *" since we are tying into the underlying
> atomics framework, and atomic_t uses int as its value type.
>
> Signed-off-by: Chris Metcalf<cmetcalf@tilera.com>

Should we have caught this with a compiler warning (mismatch definition 
and prototype) ? If not, why not?

Compile and boot tested?

Acked-by: Darren Hart <dvhart@linux.intel.com>

> ---
>   arch/tile/lib/atomic_32.c |   12 ++++++------
>   1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/tile/lib/atomic_32.c b/arch/tile/lib/atomic_32.c
> index f02040d..4657021 100644
> --- a/arch/tile/lib/atomic_32.c
> +++ b/arch/tile/lib/atomic_32.c
> @@ -202,32 +202,32 @@ static inline int *__futex_setup(int __user *v)
>   	return __atomic_hashed_lock((int __force *)v);
>   }
>
> -struct __get_user futex_set(int __user *v, int i)
> +struct __get_user futex_set(u32 __user *v, int i)
>   {
>   	return __atomic_xchg((int __force *)v, __futex_setup(v), i);
>   }
>
> -struct __get_user futex_add(int __user *v, int n)
> +struct __get_user futex_add(u32 __user *v, int n)
>   {
>   	return __atomic_xchg_add((int __force *)v, __futex_setup(v), n);
>   }
>
> -struct __get_user futex_or(int __user *v, int n)
> +struct __get_user futex_or(u32 __user *v, int n)
>   {
>   	return __atomic_or((int __force *)v, __futex_setup(v), n);
>   }
>
> -struct __get_user futex_andn(int __user *v, int n)
> +struct __get_user futex_andn(u32 __user *v, int n)
>   {
>   	return __atomic_andn((int __force *)v, __futex_setup(v), n);
>   }
>
> -struct __get_user futex_xor(int __user *v, int n)
> +struct __get_user futex_xor(u32 __user *v, int n)
>   {
>   	return __atomic_xor((int __force *)v, __futex_setup(v), n);
>   }
>
> -struct __get_user futex_cmpxchg(int __user *v, int o, int n)
> +struct __get_user futex_cmpxchg(u32 __user *v, int o, int n)
>   {
>   	return __atomic_cmpxchg((int __force *)v, __futex_setup(v), o, n);
>   }

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: [PATCH] arch/tile: fix futex sanitization definition/prototype mismatch

[Chris Metcalf]

On 3/21/2011 10:46 PM, Darren Hart wrote:
> On 03/19/2011 08:45 AM, Chris Metcalf wrote:
>> Commit 8d7718aa082aaf30a0b4989e1f04858952f941bc changed "int"
>> to "u32" in the prototypes but not the definition.
>> I missed this when I saw the patch go by on LKML.
>>
>> We cast "u32 *" to "int *" since we are tying into the underlying
>> atomics framework, and atomic_t uses int as its value type.
>>
>> Signed-off-by: Chris Metcalf<cmetcalf@tilera.com>
>
> Should we have caught this with a compiler warning (mismatch definition
> and prototype) ? If not, why not?
>
> Compile and boot tested?
>
> Acked-by: Darren Hart <dvhart@linux.intel.com>

Thanks for the acked-by, though it's already been pushed up to Linus's tree
(after I got the Reviewed-by from Michel).

The problem is that Michel provided the suggested change to the tile tree,
and I acked it for him back when it was being reviewed, but I missed that
he didn't change both prototype and definition since I didn't actually try
to build it in my tree.  Instead I caught it next time I synced up my tree,
and that's when I pushed the fix above.

I'm not sure if Michel tried builds on more than one architecture or not,
but even if he had wanted to do a tile build, it's still trickier than
other architectures, since Tilera is still in the process of pushing the
tile changes for binutils and gcc back to the community.  There are
tarballs and instructions for building our cross-tools on our website
(http://www.tilera.com/scm) but I doubt many kernel submitters are willing
to build up a whole separate framework for build-testing our architecture.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com