From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753707Ab1GHRH6 (ORCPT ); Fri, 8 Jul 2011 13:07:58 -0400 Received: from mga14.intel.com ([143.182.124.37]:14685 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752080Ab1GHRH5 (ORCPT ); Fri, 8 Jul 2011 13:07:57 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.65,500,1304319600"; d="scan'208";a="24520389" Message-ID: <4E17396D.1070001@linux.intel.com> Date: Fri, 08 Jul 2011 10:07:57 -0700 From: Darren Hart User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.17) Gecko/20110424 Lightning/1.0b2 Thunderbird/3.1.10 MIME-Version: 1.0 To: Vitaliy Ivanov CC: Jiri Kosina , Thomas Gleixner , Linus Torvalds , lkml , "trivial@kernel.org" Subject: Re: [PATCH 4/4] futex: warning corrections References: <1309821688.4527.10.camel@vitaliy-Vostro-1400> <4E1494A8.7070109@linux.intel.com> <1309986669.27362.3.camel@vitaliy-Vostro-1400> <4E14E1C6.1010202@linux.intel.com> <4E15F5BC.6050801@linux.intel.com> <1310137259.26209.65.camel@vivanov> In-Reply-To: <1310137259.26209.65.camel@vivanov> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/08/2011 08:00 AM, Vitaliy Ivanov wrote: > On Thu, 2011-07-07 at 11:06 -0700, Darren Hart wrote: >> >> On 07/07/2011 05:39 AM, Vitaliy Ivanov wrote: >>>>> From 8eeaa5a97697bcc606aea23d32028aea7b271a96 Mon Sep 17 00:00:00 2001 >>>>> From: Vitaliy Ivanov >>>>> Date: Thu, 7 Jul 2011 00:05:05 +0300 >>>>> Subject: [PATCH] futex: uninitialized warning corrections >>>>> MIME-Version: 1.0 >>>>> Content-Type: text/plain; charset=UTF-8 >>>>> Content-Transfer-Encoding: 8bit >>>>> >>>>> kernel/futex.c: In function ‘fixup_pi_state_owner.clone.17’: >>>>> kernel/futex.c:1582:6: warning: ‘curval’ may be used uninitialized in this function >>>>> kernel/futex.c: In function ‘handle_futex_death’: >>>>> kernel/futex.c:2486:6: warning: ‘nval’ may be used uninitialized in this function >>>>> kernel/futex.c: In function ‘do_futex’: >>>>> kernel/futex.c:863:11: warning: ‘curval’ may be used uninitialized in this function >>>>> kernel/futex.c:828:6: note: ‘curval’ was declared here >>>>> kernel/futex.c:898:5: warning: ‘oldval’ may be used uninitialized in this function >>>>> kernel/futex.c:890:6: note: ‘oldval’ was declared here >>>>> >>>>> Signed-off-by: Vitaliy Ivanov >>>> >>>> Please include a blurb in the commit message as to why you used >>>> uninitialized_var() rather than just assigning it. This will save people >>>> the time of wondering why, and me the time of nacking "it's simpler to >>>> just initialize to zero" patches :-) >>>> >>>> Acked-by: Darren Hart >>> >>> Darren, >>> >>> Thanks for your comments. I think the description is pretty obvious >>> here as I don't think any of these variables are affected by cmpxchg. >> >> Not so. Consider the following: >> >> u32 curval; >> if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval)) >> ret = -EFAULT; >> else if (curval != uval) >> ret = -EINVAL; >> >> the cmpxchg here assigns curval to newval if *uaddr==uval or to *uaddr >> otherwise. This is where curval gets assigned so that it can then be >> read in the following if block. gcc didn't recognize this as an >> assignment and is why it complained about it being used uninitialized. >> >> >>> There is simple assignment at the end. Seems like compiler simply >>> doesn't follow all the return cases. >> >> No, the compiler complained about the test of the value, this doesn't >> have anything to do with the return cases. > > Here is what we have: > > ------------ > static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q, > struct task_struct *newowner) > { > u32 curval; > ... > retry: > if (get_futex_value_locked(&uval, uaddr)) > goto handle_fault; > > while (1) { > newval = (uval & FUTEX_OWNER_DIED) | newtid; > > if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval)) > goto handle_fault; > if (curval == uval) > break; > uval = curval; > } > ... > } > > ------------ > static int cmpxchg_futex_value_locked(u32 *curval, u32 __user *uaddr, > u32 uval, u32 newval) > { > int ret; > > pagefault_disable(); > ret = futex_atomic_cmpxchg_inatomic(curval, uaddr, uval, newval); > pagefault_enable(); > > return ret; > } > ------------ > > And for x86: > > static inline int futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, > u32 oldval, u32 newval) > { > int ret = 0; > > #if defined(CONFIG_X86_32) && !defined(CONFIG_X86_BSWAP) > /* Real i386 machines have no cmpxchg instruction */ > if (boot_cpu_data.x86 == 3) > return -ENOSYS; > #endif > > if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32))) > return -EFAULT; > > asm volatile("1:\t" LOCK_PREFIX "cmpxchgl %4, %2\n" > "2:\t.section .fixup, \"ax\"\n" > "3:\tmov %3, %0\n" > "\tjmp 2b\n" > "\t.previous\n" > _ASM_EXTABLE(1b, 3b) > : "+r" (ret), "=a" (oldval), "+m" (*uaddr) > : "i" (-EFAULT), "r" (newval), "1" (oldval) > : "memory" > ); > > *uval = oldval; <------------- uval is being changed here only. it's not modified by asm cmpxchgl. > return ret; > } > > > Am I missing something? Nope, I was incorrect - I didn't realize that futex_atomic_cmpxchg_inatomic didn't set uval on EFAULT. gcc is not detecting that curval is only read if futex_atomic_cmpxchg_inatomic succeeds, so I believe the uninitialized_var() fix is correct. This is one of the reasons why we need to include the reasoning in the commit log - specifically that: "futex_atomic_cmpxchg_inatomic only assigns curval on success, but on failure curval is not read, so instruct gcc to ignore the uninitialized warning." -- Darren Hart Intel Open Source Technology Center Yocto Project - Linux Kernel