Messages in this thread | | | Subject | Re: [PATCH 4/4] futex: warning corrections | From | Vitaliy Ivanov <> | Date | Fri, 08 Jul 2011 18:00:59 +0300 |
| |
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 <vitalivanov@gmail.com> > >>> 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 <vitalivanov@gmail.com> > >> > >> 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 <dvhart@linux.intel.com> > > > > 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?
- Vitaliy
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |