Re: [PATCH 5/4] futex: silence uninitialized warnings

[Darren Hart <dvhart@linux.intel.com>]

On Sun, 2014-01-05 at 17:38 -0800, Davidlohr Bueso wrote:
> From: Davidlohr Bueso <davidlohr@hp.com>
> 
> Callers of cmpxchg_futex_value_locked() can trigger the following:
> 
> kernel/futex.c: In function ‘futex_lock_pi_atomic’:
> kernel/futex.c:725: warning: ‘curval’ may be used uninitialized in this function
> 
> This was initially addressed by commit 7cfdaf38, but others still remain. Silence
> these messages once and for all as the variables really aren't uninitialized.
> 

I've gone after such things myself and seen arguments against this as it
will mask any future such bugs. If, as in this case, the value is indeed
not being used uninitialized, I believe this is considered a compiler
bug. Do we mask compiler bugs in the kernel at the expense of catching
legitimate future bugs? There are certainly several examples of this
method being used throughout the kernel.

Alternatively - does anyone see something Davidlohr or I have missed in
this call chain? It seems to me that the only way for curval not be
initialized is if cmpxchg_futex_value_lock() returns an error, in which
case we return without using curval anyway.

Is there some rule about how to deal with this when inline ASM is
involved? (Although there is an explicit assignment following that ASM).

If not, given existing precedent, and the unlikelihood of this
particular path seeing significant changes, I'm inclined to agree with
the proposed fix:

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

--
Darren

> Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> ---
>  kernel/futex.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 5b4d09e..65ade8a 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -838,7 +838,7 @@ static int futex_lock_pi_atomic(u32 __user *uaddr, struct futex_hash_bucket *hb,
>  				struct task_struct *task, int set_waiters)
>  {
>  	int lock_taken, ret, force_take = 0;
> -	u32 uval, newval, curval, vpid = task_pid_vnr(task);
> +	u32 uval, newval, uninitialized_var(curval), vpid = task_pid_vnr(task);
>  
>  retry:
>  	ret = lock_taken = 0;
> @@ -2227,7 +2227,7 @@ static int futex_unlock_pi(u32 __user *uaddr, unsigned int flags)
>  	struct futex_hash_bucket *hb;
>  	struct futex_q *this, *next;
>  	union futex_key key = FUTEX_KEY_INIT;
> -	u32 uval, vpid = task_pid_vnr(current);
> +	u32 uninitialized_var(uval), vpid = task_pid_vnr(current);
>  	int ret;
>  
>  retry:
> @@ -2843,7 +2843,7 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
>  
>  static int __init futex_init(void)
>  {
> -	u32 curval;
> +	u32 uninitialized_var(curval);
>  	unsigned long i;
>  
>  #if CONFIG_BASE_SMALL

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