[PATCH 4/4] futex: warning corrections

[PATCH 4/4] futex: warning corrections

[Vitaliy Ivanov]

>From 3f7997d71fe2b5cb16e2913928f68023855d786d Mon Sep 17 00:00:00 2001
From: Vitaliy Ivanov <vitalivanov@gmail.com>
Date: Tue, 5 Jul 2011 02:07:42 +0300
Subject: [PATCH 4/4] futex: 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>
---
 kernel/futex.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index fe28dc2..516ebf9 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -825,7 +825,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this)
 {
 	struct task_struct *new_owner;
 	struct futex_pi_state *pi_state = this->pi_state;
-	u32 curval, newval;
+	u32 curval = 0, newval;
 
 	if (!pi_state)
 		return -EINVAL;
@@ -887,7 +887,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this)
 
 static int unlock_futex_pi(u32 __user *uaddr, u32 uval)
 {
-	u32 oldval;
+	u32 oldval = 0;
 
 	/*
 	 * There is no waiter, so we unlock the futex. The owner died
@@ -1546,7 +1546,7 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
 	u32 newtid = task_pid_vnr(newowner) | FUTEX_WAITERS;
 	struct futex_pi_state *pi_state = q->pi_state;
 	struct task_struct *oldowner = pi_state->owner;
-	u32 uval, curval, newval;
+	u32 uval, curval = 0, newval;
 	int ret;
 
 	/* Owner died? */
@@ -2451,7 +2451,7 @@ err_unlock:
  */
 int handle_futex_death(u32 __user *uaddr, struct task_struct *curr, int pi)
 {
-	u32 uval, nval, mval;
+	u32 uval, nval = 0, mval;
 
 retry:
 	if (get_user(uval, uaddr))
-- 
1.7.4.1

Re: [PATCH 4/4] futex: warning corrections

[Darren Hart]

On 07/04/2011 04:21 PM, Vitaliy Ivanov wrote:
> From 3f7997d71fe2b5cb16e2913928f68023855d786d Mon Sep 17 00:00:00 2001
> From: Vitaliy Ivanov <vitalivanov@gmail.com>
> Date: Tue, 5 Jul 2011 02:07:42 +0300
> Subject: [PATCH 4/4] futex: warning corrections
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>

Hi Vitaliy,

Thanks for looking to fix these warnings. Note that the compiler isn't
always aware of some of the subtleties involved with things like
cmpxchg. In cases where it thinks there may be an uninitialized usage,
be sure to confirm it is possible before adding the overhead of an
assignment to a hot path.

None of these assignments are necessary. Consider using __maybe_unused
instead.

--
Darren


> 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>
> ---
>  kernel/futex.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index fe28dc2..516ebf9 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -825,7 +825,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this)
>  {
>  	struct task_struct *new_owner;
>  	struct futex_pi_state *pi_state = this->pi_state;
> -	u32 curval, newval;
> +	u32 curval = 0, newval;
>  
>  	if (!pi_state)
>  		return -EINVAL;
> @@ -887,7 +887,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this)
>  
>  static int unlock_futex_pi(u32 __user *uaddr, u32 uval)
>  {
> -	u32 oldval;
> +	u32 oldval = 0;
>  
>  	/*
>  	 * There is no waiter, so we unlock the futex. The owner died
> @@ -1546,7 +1546,7 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
>  	u32 newtid = task_pid_vnr(newowner) | FUTEX_WAITERS;
>  	struct futex_pi_state *pi_state = q->pi_state;
>  	struct task_struct *oldowner = pi_state->owner;
> -	u32 uval, curval, newval;
> +	u32 uval, curval = 0, newval;
>  	int ret;
>  
>  	/* Owner died? */
> @@ -2451,7 +2451,7 @@ err_unlock:
>   */
>  int handle_futex_death(u32 __user *uaddr, struct task_struct *curr, int pi)
>  {
> -	u32 uval, nval, mval;
> +	u32 uval, nval = 0, mval;
>  
>  retry:
>  	if (get_user(uval, uaddr))

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

Re: [PATCH 4/4] futex: warning corrections

[Vitaliy Ivanov]

On Wed, 2011-07-06 at 10:00 -0700, Darren Hart wrote:
> On 07/04/2011 04:21 PM, Vitaliy Ivanov wrote:
> > From 3f7997d71fe2b5cb16e2913928f68023855d786d Mon Sep 17 00:00:00 2001
> > From: Vitaliy Ivanov <vitalivanov@gmail.com>
> > Date: Tue, 5 Jul 2011 02:07:42 +0300
> > Subject: [PATCH 4/4] futex: warning corrections
> Hi Vitaliy,
> 
> Thanks for looking to fix these warnings. Note that the compiler isn't
> always aware of some of the subtleties involved with things like
> cmpxchg. In cases where it thinks there may be an uninitialized usage,
> be sure to confirm it is possible before adding the overhead of an
> assignment to a hot path.
> 
> None of these assignments are necessary. Consider using __maybe_unused
> instead.

Darren,

Actually unused and uninitialized are different issues. __maybe_unused won't help in this case but there is another trick used in kernel for this:

#define uninitialized_var(x) x = x

So, here is updated patch.

>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>
---
 kernel/futex.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index fe28dc2..efb8e5b 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -825,7 +825,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this)
 {
 	struct task_struct *new_owner;
 	struct futex_pi_state *pi_state = this->pi_state;
-	u32 curval, newval;
+	u32 uninitialized_var(curval), newval;
 
 	if (!pi_state)
 		return -EINVAL;
@@ -887,7 +887,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this)
 
 static int unlock_futex_pi(u32 __user *uaddr, u32 uval)
 {
-	u32 oldval;
+	u32 uninitialized_var(oldval);
 
 	/*
 	 * There is no waiter, so we unlock the futex. The owner died
@@ -1546,7 +1546,7 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
 	u32 newtid = task_pid_vnr(newowner) | FUTEX_WAITERS;
 	struct futex_pi_state *pi_state = q->pi_state;
 	struct task_struct *oldowner = pi_state->owner;
-	u32 uval, curval, newval;
+	u32 uval, uninitialized_var(curval), newval;
 	int ret;
 
 	/* Owner died? */
@@ -2451,7 +2451,7 @@ err_unlock:
  */
 int handle_futex_death(u32 __user *uaddr, struct task_struct *curr, int pi)
 {
-	u32 uval, nval, mval;
+	u32 uval, uninitialized_var(nval), mval;
 
 retry:
 	if (get_user(uval, uaddr))
-- 
1.7.4.1

Re: [PATCH 4/4] futex: warning corrections

[Darren Hart]

On 07/06/2011 02:11 PM, Vitaliy Ivanov wrote:
> On Wed, 2011-07-06 at 10:00 -0700, Darren Hart wrote:
>> On 07/04/2011 04:21 PM, Vitaliy Ivanov wrote:
>>> From 3f7997d71fe2b5cb16e2913928f68023855d786d Mon Sep 17 00:00:00 2001
>>> From: Vitaliy Ivanov <vitalivanov@gmail.com>
>>> Date: Tue, 5 Jul 2011 02:07:42 +0300
>>> Subject: [PATCH 4/4] futex: warning corrections
>> Hi Vitaliy,
>>
>> Thanks for looking to fix these warnings. Note that the compiler isn't
>> always aware of some of the subtleties involved with things like
>> cmpxchg. In cases where it thinks there may be an uninitialized usage,
>> be sure to confirm it is possible before adding the overhead of an
>> assignment to a hot path.
>>
>> None of these assignments are necessary. Consider using __maybe_unused
>> instead.
> 
> Darren,
> 
> Actually unused and uninitialized are different issues. __maybe_unused won't help in this case but there is another trick used in kernel for this:
> 


Heh, duh. I recently worked with __maybe_unused and uninitialized_var(),
and got them crossed in my head.


> #define uninitialized_var(x) x = x


Yes, this is the right approach.


> 
> So, here is updated patch.
> 
> 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>


> ---
>  kernel/futex.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index fe28dc2..efb8e5b 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -825,7 +825,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this)
>  {
>  	struct task_struct *new_owner;
>  	struct futex_pi_state *pi_state = this->pi_state;
> -	u32 curval, newval;
> +	u32 uninitialized_var(curval), newval;
>  
>  	if (!pi_state)
>  		return -EINVAL;
> @@ -887,7 +887,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this)
>  
>  static int unlock_futex_pi(u32 __user *uaddr, u32 uval)
>  {
> -	u32 oldval;
> +	u32 uninitialized_var(oldval);
>  
>  	/*
>  	 * There is no waiter, so we unlock the futex. The owner died
> @@ -1546,7 +1546,7 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
>  	u32 newtid = task_pid_vnr(newowner) | FUTEX_WAITERS;
>  	struct futex_pi_state *pi_state = q->pi_state;
>  	struct task_struct *oldowner = pi_state->owner;
> -	u32 uval, curval, newval;
> +	u32 uval, uninitialized_var(curval), newval;
>  	int ret;
>  
>  	/* Owner died? */
> @@ -2451,7 +2451,7 @@ err_unlock:
>   */
>  int handle_futex_death(u32 __user *uaddr, struct task_struct *curr, int pi)
>  {
> -	u32 uval, nval, mval;
> +	u32 uval, uninitialized_var(nval), mval;
>  
>  retry:
>  	if (get_user(uval, uaddr))

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

Re: [PATCH 4/4] futex: warning corrections

[Vitaliy Ivanov]

>> 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.
There is simple assignment at the end. Seems like compiler simply
doesn't follow all the return cases.

Anyway, I'm going to resend patches to Jiri for his trivial-next tree.

Vitaliy

Re: [PATCH 4/4] futex: warning corrections

[Darren Hart]

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.

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

Re: [PATCH 4/4] futex: warning corrections

[Vitaliy Ivanov]

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

Re: [PATCH 4/4] futex: warning corrections

[Darren Hart]

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 <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?

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