[PATCH] kernel/futex.c: notice the return value after rt_mutex_finish_proxy_lock() fails

[PATCH] kernel/futex.c: notice the return value after rt_mutex_finish_proxy_lock() fails

[Chen Gang]

rt_mutex_finish_proxy_lock() can return failure code (e.g. -EINTR,
-ETIMEDOUT).

Original implementation has already noticed about it, but not check it
before next work.

Also let coments within 80 columns to pass "./scripts/checkpatch.pl".


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/futex.c |   30 ++++++++++++++++--------------
 1 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index c3a1a55..1a94e7d 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 		ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1);
 		debug_rt_mutex_free_waiter(&rt_waiter);
 
-		spin_lock(q.lock_ptr);
-		/*
-		 * Fixup the pi_state owner and possibly acquire the lock if we
-		 * haven't already.
-		 */
-		res = fixup_owner(uaddr2, &q, !ret);
-		/*
-		 * If fixup_owner() returned an error, proprogate that.  If it
-		 * acquired the lock, clear -ETIMEDOUT or -EINTR.
-		 */
-		if (res)
-			ret = (res < 0) ? res : 0;
+		if (!ret) {
+			spin_lock(q.lock_ptr);
+			/*
+			 * Fixup the pi_state owner and possibly acquire the
+			 * lock if we haven't already.
+			 */
+			res = fixup_owner(uaddr2, &q, !ret);
+			/*
+			 * If fixup_owner() returned an error, proprogate that.
+			 * If it acquired the lock, clear -ETIMEDOUT or -EINTR.
+			 */
+			if (res)
+				ret = (res < 0) ? res : 0;
 
-		/* Unqueue and drop the lock. */
-		unqueue_me_pi(&q);
+			/* Unqueue and drop the lock. */
+			unqueue_me_pi(&q);
+		}
 	}
 
 	/*
-- 
1.7.7.6

Re: [PATCH] kernel/futex.c: notice the return value after rt_mutex_finish_proxy_lock() fails

[Darren Hart]

HopingOn Tue, 2013-08-20 at 11:07 +0800, Chen Gang wrote:


Hi Chen,

> rt_mutex_finish_proxy_lock() can return failure code (e.g. -EINTR,
> -ETIMEDOUT).
> 
> Original implementation has already noticed about it, but not check it
> before next work.
> 
> Also let coments within 80 columns to pass "./scripts/checkpatch.pl".
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/futex.c |   30 ++++++++++++++++--------------
>  1 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index c3a1a55..1a94e7d 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
>  		ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1);
>  		debug_rt_mutex_free_waiter(&rt_waiter);
>  
> -		spin_lock(q.lock_ptr);
> -		/*
> -		 * Fixup the pi_state owner and possibly acquire the lock if we
> -		 * haven't already.
> -		 */
> -		res = fixup_owner(uaddr2, &q, !ret);


This call catches a corner case which appears to be skipped now. Or am I
missing how you accounted for that?


> -		/*
> -		 * If fixup_owner() returned an error, proprogate that.  If it
> -		 * acquired the lock, clear -ETIMEDOUT or -EINTR.
> -		 */
> -		if (res)
> -			ret = (res < 0) ? res : 0;
> +		if (!ret) {
> +			spin_lock(q.lock_ptr);
> +			/*
> +			 * Fixup the pi_state owner and possibly acquire the
> +			 * lock if we haven't already.
> +			 */
> +			res = fixup_owner(uaddr2, &q, !ret);
> +			/*
> +			 * If fixup_owner() returned an error, proprogate that.
> +			 * If it acquired the lock, clear -ETIMEDOUT or -EINTR.
> +			 */
> +			if (res)
> +				ret = (res < 0) ? res : 0;
>  
> -		/* Unqueue and drop the lock. */
> -		unqueue_me_pi(&q);
> +			/* Unqueue and drop the lock. */
> +			unqueue_me_pi(&q);
> +		}
>  	}
>  
>  	/*

Thanks,

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

Re: [PATCH] kernel/futex.c: notice the return value after rt_mutex_finish_proxy_lock() fails

[Chen Gang]

On 08/21/2013 12:19 AM, Darren Hart wrote:
> HopingOn Tue, 2013-08-20 at 11:07 +0800, Chen Gang wrote:
> 
> 
> Hi Chen,
> 
>> rt_mutex_finish_proxy_lock() can return failure code (e.g. -EINTR,
>> -ETIMEDOUT).
>>
>> Original implementation has already noticed about it, but not check it
>> before next work.
>>
>> Also let coments within 80 columns to pass "./scripts/checkpatch.pl".
>>
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>  kernel/futex.c |   30 ++++++++++++++++--------------
>>  1 files changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/kernel/futex.c b/kernel/futex.c
>> index c3a1a55..1a94e7d 100644
>> --- a/kernel/futex.c
>> +++ b/kernel/futex.c
>> @@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
>>  		ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1);
>>  		debug_rt_mutex_free_waiter(&rt_waiter);
>>  
>> -		spin_lock(q.lock_ptr);
>> -		/*
>> -		 * Fixup the pi_state owner and possibly acquire the lock if we
>> -		 * haven't already.
>> -		 */
>> -		res = fixup_owner(uaddr2, &q, !ret);
> 
> 
> This call catches a corner case which appears to be skipped now. Or am I
> missing how you accounted for that?
> 
> 

Pardon ?

Hmm... this patch lets related code block in "if(!ret) {...}", should
not remove any code.

Please help check again for whether what I have done is correct or not.

Thanks.

>> -		/*
>> -		 * If fixup_owner() returned an error, proprogate that.  If it
>> -		 * acquired the lock, clear -ETIMEDOUT or -EINTR.
>> -		 */
>> -		if (res)
>> -			ret = (res < 0) ? res : 0;
>> +		if (!ret) {
>> +			spin_lock(q.lock_ptr);
>> +			/*
>> +			 * Fixup the pi_state owner and possibly acquire the
>> +			 * lock if we haven't already.
>> +			 */
>> +			res = fixup_owner(uaddr2, &q, !ret);
>> +			/*
>> +			 * If fixup_owner() returned an error, proprogate that.
>> +			 * If it acquired the lock, clear -ETIMEDOUT or -EINTR.
>> +			 */
>> +			if (res)
>> +				ret = (res < 0) ? res : 0;
>>  
>> -		/* Unqueue and drop the lock. */
>> -		unqueue_me_pi(&q);
>> +			/* Unqueue and drop the lock. */
>> +			unqueue_me_pi(&q);
>> +		}
>>  	}
>>  
>>  	/*
> 
> Thanks,
> 


-- 
Chen Gang

Re: [PATCH] kernel/futex.c: notice the return value after rt_mutex_finish_proxy_lock() fails

[Chen Gang]

Hello Maintainers:

Please help check this patch, when you have time.


Thanks.

On 08/21/2013 11:48 AM, Chen Gang wrote:
> On 08/21/2013 12:19 AM, Darren Hart wrote:
>> HopingOn Tue, 2013-08-20 at 11:07 +0800, Chen Gang wrote:
>>
>>
>> Hi Chen,
>>
>>> rt_mutex_finish_proxy_lock() can return failure code (e.g. -EINTR,
>>> -ETIMEDOUT).
>>>
>>> Original implementation has already noticed about it, but not check it
>>> before next work.
>>>
>>> Also let coments within 80 columns to pass "./scripts/checkpatch.pl".
>>>
>>>
>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>> ---
>>>  kernel/futex.c |   30 ++++++++++++++++--------------
>>>  1 files changed, 16 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/kernel/futex.c b/kernel/futex.c
>>> index c3a1a55..1a94e7d 100644
>>> --- a/kernel/futex.c
>>> +++ b/kernel/futex.c
>>> @@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
>>>  		ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1);
>>>  		debug_rt_mutex_free_waiter(&rt_waiter);
>>>  
>>> -		spin_lock(q.lock_ptr);
>>> -		/*
>>> -		 * Fixup the pi_state owner and possibly acquire the lock if we
>>> -		 * haven't already.
>>> -		 */
>>> -		res = fixup_owner(uaddr2, &q, !ret);
>>
>>
>> This call catches a corner case which appears to be skipped now. Or am I
>> missing how you accounted for that?
>>
>>
> 
> Pardon ?
> 
> Hmm... this patch lets related code block in "if(!ret) {...}", should
> not remove any code.
> 
> Please help check again for whether what I have done is correct or not.
> 
> Thanks.
> 
>>> -		/*
>>> -		 * If fixup_owner() returned an error, proprogate that.  If it
>>> -		 * acquired the lock, clear -ETIMEDOUT or -EINTR.
>>> -		 */
>>> -		if (res)
>>> -			ret = (res < 0) ? res : 0;
>>> +		if (!ret) {
>>> +			spin_lock(q.lock_ptr);
>>> +			/*
>>> +			 * Fixup the pi_state owner and possibly acquire the
>>> +			 * lock if we haven't already.
>>> +			 */
>>> +			res = fixup_owner(uaddr2, &q, !ret);
>>> +			/*
>>> +			 * If fixup_owner() returned an error, proprogate that.
>>> +			 * If it acquired the lock, clear -ETIMEDOUT or -EINTR.
>>> +			 */
>>> +			if (res)
>>> +				ret = (res < 0) ? res : 0;
>>>  
>>> -		/* Unqueue and drop the lock. */
>>> -		unqueue_me_pi(&q);
>>> +			/* Unqueue and drop the lock. */
>>> +			unqueue_me_pi(&q);
>>> +		}
>>>  	}
>>>  
>>>  	/*
>>
>> Thanks,
>>
> 
> 


-- 
Chen Gang

Re: [PATCH] kernel/futex.c: notice the return value after rt_mutex_finish_proxy_lock() fails

[Thomas Gleixner]

On Tue, 20 Aug 2013, Chen Gang wrote:

> rt_mutex_finish_proxy_lock() can return failure code (e.g. -EINTR,
> -ETIMEDOUT).
> 
> Original implementation has already noticed about it, but not check it
> before next work.
> 
> Also let coments within 80 columns to pass "./scripts/checkpatch.pl".
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/futex.c |   30 ++++++++++++++++--------------
>  1 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index c3a1a55..1a94e7d 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
>  		ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1);
>  		debug_rt_mutex_free_waiter(&rt_waiter);
>  
> -		spin_lock(q.lock_ptr);
> -		/*
> -		 * Fixup the pi_state owner and possibly acquire the lock if we
> -		 * haven't already.
> -		 */
> -		res = fixup_owner(uaddr2, &q, !ret);
> -		/*
> -		 * If fixup_owner() returned an error, proprogate that.  If it
> -		 * acquired the lock, clear -ETIMEDOUT or -EINTR.
> -		 */
> -		if (res)
> -			ret = (res < 0) ? res : 0;
> +		if (!ret) {

Again. This is completely wrong!  

We MUST call fixup_owner even if finish_proxy_lock() returned with an
error code. Simply because finish_proxy_lock() is called outside of
the spin_lock(q.lock_ptr) region and another thread might have
modified the futex state. So we need to handle the corner cases
otherwise we might leave the futex in some undefined state.

You're reintroducing a hard to decode bug, which got analyzed and
fixed in futex_lock_pi() years ago. See the history for the
explanation.

Sigh.

	tglx

Re: [PATCH] kernel/futex.c: notice the return value after rt_mutex_finish_proxy_lock() fails

[Darren Hart]

On Thu, 2013-09-12 at 16:32 +0200, Thomas Gleixner wrote:
> On Tue, 20 Aug 2013, Chen Gang wrote:
> 
> > rt_mutex_finish_proxy_lock() can return failure code (e.g. -EINTR,
> > -ETIMEDOUT).
> > 
> > Original implementation has already noticed about it, but not check it
> > before next work.
> > 
> > Also let coments within 80 columns to pass "./scripts/checkpatch.pl".
> > 
> > 
> > Signed-off-by: Chen Gang <gang.chen@asianux.com>
> > ---
> >  kernel/futex.c |   30 ++++++++++++++++--------------
> >  1 files changed, 16 insertions(+), 14 deletions(-)
> > 
> > diff --git a/kernel/futex.c b/kernel/futex.c
> > index c3a1a55..1a94e7d 100644
> > --- a/kernel/futex.c
> > +++ b/kernel/futex.c
> > @@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
> >               ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1);
> >               debug_rt_mutex_free_waiter(&rt_waiter);
> >  
> > -             spin_lock(q.lock_ptr);
> > -             /*
> > -              * Fixup the pi_state owner and possibly acquire the lock if we
> > -              * haven't already.
> > -              */
> > -             res = fixup_owner(uaddr2, &q, !ret);
> > -             /*
> > -              * If fixup_owner() returned an error, proprogate that.  If it
> > -              * acquired the lock, clear -ETIMEDOUT or -EINTR.
> > -              */
> > -             if (res)
> > -                     ret = (res < 0) ? res : 0;
> > +             if (!ret) {
> 
> Again. This is completely wrong!  
> 
> We MUST call fixup_owner even if finish_proxy_lock() returned with an
> error code. Simply because finish_proxy_lock() is called outside of
> the spin_lock(q.lock_ptr) region and another thread might have
> modified the futex state. So we need to handle the corner cases
> otherwise we might leave the futex in some undefined state.
> 
> You're reintroducing a hard to decode bug, which got analyzed and
> fixed in futex_lock_pi() years ago. See the history for the
> explanation.
> 
> Sigh.
> 
>         tglx

Chen, perhaps you can let us know what the failure scenario is that you
are trying to address with this patch. I only replied the once as I
pointed out the corner-case and expected you to follow up with that.
This region of code is very fragile to modifications as it has become
more corner-cases than core logic in some places :-)

For starters, I'm not following your second sentence in the commit log.
Can you elaborate on the following?

"Original implementation has already noticed about it, but not check it
before next work."

Do you have a test-case that demonstrates a failure mode?

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

Re: [PATCH] kernel/futex.c: notice the return value after rt_mutex_finish_proxy_lock() fails

[Thomas Gleixner]

On Thu, 12 Sep 2013, Darren Hart wrote:
> On Thu, 2013-09-12 at 16:32 +0200, Thomas Gleixner wrote:
> > On Tue, 20 Aug 2013, Chen Gang wrote:
> > 
> > > rt_mutex_finish_proxy_lock() can return failure code (e.g. -EINTR,
> > > -ETIMEDOUT).
> > > 
> > > Original implementation has already noticed about it, but not check it
> > > before next work.
> > > 
> > > Also let coments within 80 columns to pass "./scripts/checkpatch.pl".
> > > 
> > > 
> > > Signed-off-by: Chen Gang <gang.chen@asianux.com>
> > > ---
> > >  kernel/futex.c |   30 ++++++++++++++++--------------
> > >  1 files changed, 16 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/kernel/futex.c b/kernel/futex.c
> > > index c3a1a55..1a94e7d 100644
> > > --- a/kernel/futex.c
> > > +++ b/kernel/futex.c
> > > @@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
> > >               ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1);
> > >               debug_rt_mutex_free_waiter(&rt_waiter);
> > >  
> > > -             spin_lock(q.lock_ptr);
> > > -             /*
> > > -              * Fixup the pi_state owner and possibly acquire the lock if we
> > > -              * haven't already.
> > > -              */
> > > -             res = fixup_owner(uaddr2, &q, !ret);
> > > -             /*
> > > -              * If fixup_owner() returned an error, proprogate that.  If it
> > > -              * acquired the lock, clear -ETIMEDOUT or -EINTR.
> > > -              */
> > > -             if (res)
> > > -                     ret = (res < 0) ? res : 0;
> > > +             if (!ret) {
> > 
> > Again. This is completely wrong!  
> > 
> > We MUST call fixup_owner even if finish_proxy_lock() returned with an
> > error code. Simply because finish_proxy_lock() is called outside of
> > the spin_lock(q.lock_ptr) region and another thread might have
> > modified the futex state. So we need to handle the corner cases
> > otherwise we might leave the futex in some undefined state.
> > 
> > You're reintroducing a hard to decode bug, which got analyzed and
> > fixed in futex_lock_pi() years ago. See the history for the
> > explanation.
> > 
> > Sigh.
> > 
> >         tglx
> 
> Chen, perhaps you can let us know what the failure scenario is that you
> are trying to address with this patch.

No failure scenario at all.

Chen is on a self defined agenda to fix random kernel bugs in random
kernel subdirectories on a given rate by all means. (Google yourself
for the details.)

That crusade does not involve any failure analysis or test cases. It's
just driven by mechanically checking the code for inconsistencies. Now
he tripped over a non obvious return value chain in the futex code. So
instead of figuring out why it is coded this way, he just mechanically
decided that there is a missing check. Though:

The return value is checked and it needs deep understanding of the way
how futexes work to grok why it's necessary to invoke fixup_owner()
independent of the rt_mutex_finish_proxy_lock() return value.

The code in question is:

	ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1);

	spin_lock(q.lock_ptr);
	/*
	 * Fixup the pi_state owner and possibly acquire the lock if we
	 * haven't already.
	 */
	res = fixup_owner(uaddr2, &q, !ret);
	/*
	 * If fixup_owner() returned an error, proprogate that.  If it
	 * acquired the lock, clear -ETIMEDOUT or -EINTR. 
	 */
	if (res)
		ret = (res < 0) ? res : 0;

If you can understand the comments in the code and you are able to
follow the implementation of fixup_owner() and the usage of "!ret" as
an argument you really should be able to figure out, why this is
correct.

I'm well aware, as you are, that this code is hard to grok. BUT:

If this code in futex_wait_requeue_pi() is wrong why did Chen's
correctness checker not trigger on the following code in
futex_lock_pi()?:

	if (!trylock)
		ret = rt_mutex_timed_lock(&q.pi_state->pi_mutex, to, 1);
	else {
		ret = rt_mutex_trylock(&q.pi_state->pi_mutex);
		/* Fixup the trylock return value: */
		ret = ret ? 0 : -EWOULDBLOCK;
	}

	spin_lock(q.lock_ptr);
	/*
	 * Fixup the pi_state owner and possibly acquire the lock if we
	 * haven't already.
	 */
	res = fixup_owner(uaddr, &q, !ret);
	/*
	 * If fixup_owner() returned an error, proprogate that.  If it acquired
	 * the lock, clear our -ETIMEDOUT or -EINTR.
	 */
	if (res)
		ret = (res < 0) ? res : 0;

It's the very same pattern and according to Chen's logic broken as
well.

As I recommended to Chen to read the history of futex.c, I just can
recommend the same thing to you to figure out why the heck this is the
correct way to handle it.

Hint: The relevant commit starts with: cdf

The code has changed quite a bit since then, but the issue which is
described quite well in the commit log is still the same.

Just for the record:

     Line 48 of futex.c says: "The futexes are also cursed."

Thanks,

	tglx

Re: [PATCH] kernel/futex.c: notice the return value after rt_mutex_finish_proxy_lock() fails

[Chen Gang]

Firstly, I am glad to see that you did not redirect all my mails to
"/dev/null".  ;-)


On 09/13/2013 07:36 AM, Thomas Gleixner wrote:
> On Thu, 12 Sep 2013, Darren Hart wrote:
>> On Thu, 2013-09-12 at 16:32 +0200, Thomas Gleixner wrote:
>>> On Tue, 20 Aug 2013, Chen Gang wrote:
>>>
>>>> rt_mutex_finish_proxy_lock() can return failure code (e.g. -EINTR,
>>>> -ETIMEDOUT).
>>>>
>>>> Original implementation has already noticed about it, but not check it
>>>> before next work.
>>>>
>>>> Also let coments within 80 columns to pass "./scripts/checkpatch.pl".
>>>>
>>>>
>>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>>> ---
>>>>  kernel/futex.c |   30 ++++++++++++++++--------------
>>>>  1 files changed, 16 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/kernel/futex.c b/kernel/futex.c
>>>> index c3a1a55..1a94e7d 100644
>>>> --- a/kernel/futex.c
>>>> +++ b/kernel/futex.c
>>>> @@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
>>>>               ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1);
>>>>               debug_rt_mutex_free_waiter(&rt_waiter);
>>>>  
>>>> -             spin_lock(q.lock_ptr);
>>>> -             /*
>>>> -              * Fixup the pi_state owner and possibly acquire the lock if we
>>>> -              * haven't already.
>>>> -              */
>>>> -             res = fixup_owner(uaddr2, &q, !ret);
>>>> -             /*
>>>> -              * If fixup_owner() returned an error, proprogate that.  If it
>>>> -              * acquired the lock, clear -ETIMEDOUT or -EINTR.
>>>> -              */
>>>> -             if (res)
>>>> -                     ret = (res < 0) ? res : 0;
>>>> +             if (!ret) {
>>>
>>> Again. This is completely wrong!  
>>>

Yeah, really it is.


>>> We MUST call fixup_owner even if finish_proxy_lock() returned with an
>>> error code. Simply because finish_proxy_lock() is called outside of
>>> the spin_lock(q.lock_ptr) region and another thread might have
>>> modified the futex state. So we need to handle the corner cases
>>> otherwise we might leave the futex in some undefined state.
>>>
>>> You're reintroducing a hard to decode bug, which got analyzed and
>>> fixed in futex_lock_pi() years ago. See the history for the
>>> explanation.
>>>

Thank you for your details explanation.


>>> Sigh.
>>>
>>>         tglx
>>
>> Chen, perhaps you can let us know what the failure scenario is that you
>> are trying to address with this patch.
> 
> No failure scenario at all.
> 
> Chen is on a self defined agenda to fix random kernel bugs in random
> kernel subdirectories on a given rate by all means. (Google yourself
> for the details.)
> 

Hmm... what you said is partly correct -- it is part of my goal (at
least, I feel it is valuable to kernel).

Others which you did not mention, but still related with kernel:

  1. LTP (Linux Test Project), which I will start at q4 of 2013, which can let me provide more tests on kernel (also can find more kernel issues).

  2. gcc/binutils: which can find more issues both for kernel and gcc/binutils (I am also communicating with gcc folks too).

  3. Documents (or trivial patches): which I am trying, but seems I did not do quite well.


> That crusade does not involve any failure analysis or test cases. It's
> just driven by mechanically checking the code for inconsistencies. Now
> he tripped over a non obvious return value chain in the futex code. So
> instead of figuring out why it is coded this way, he just mechanically
> decided that there is a missing check. Though:
> 
> The return value is checked and it needs deep understanding of the way
> how futexes work to grok why it's necessary to invoke fixup_owner()
> independent of the rt_mutex_finish_proxy_lock() return value.
> 
> The code in question is:
> 
> 	ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1);
> 
> 	spin_lock(q.lock_ptr);
> 	/*
> 	 * Fixup the pi_state owner and possibly acquire the lock if we
> 	 * haven't already.
> 	 */
> 	res = fixup_owner(uaddr2, &q, !ret);
> 	/*
> 	 * If fixup_owner() returned an error, proprogate that.  If it
> 	 * acquired the lock, clear -ETIMEDOUT or -EINTR. 
> 	 */
> 	if (res)
> 		ret = (res < 0) ? res : 0;
> 
> If you can understand the comments in the code and you are able to
> follow the implementation of fixup_owner() and the usage of "!ret" as
> an argument you really should be able to figure out, why this is
> correct.
> 
> I'm well aware, as you are, that this code is hard to grok. BUT:
> 
> If this code in futex_wait_requeue_pi() is wrong why did Chen's
> correctness checker not trigger on the following code in
> futex_lock_pi()?:
> 
> 	if (!trylock)
> 		ret = rt_mutex_timed_lock(&q.pi_state->pi_mutex, to, 1);
> 	else {
> 		ret = rt_mutex_trylock(&q.pi_state->pi_mutex);
> 		/* Fixup the trylock return value: */
> 		ret = ret ? 0 : -EWOULDBLOCK;
> 	}
> 
> 	spin_lock(q.lock_ptr);
> 	/*
> 	 * Fixup the pi_state owner and possibly acquire the lock if we
> 	 * haven't already.
> 	 */
> 	res = fixup_owner(uaddr, &q, !ret);
> 	/*
> 	 * If fixup_owner() returned an error, proprogate that.  If it acquired
> 	 * the lock, clear our -ETIMEDOUT or -EINTR.
> 	 */
> 	if (res)
> 		ret = (res < 0) ? res : 0;
> 
> It's the very same pattern and according to Chen's logic broken as
> well.
> 
> As I recommended to Chen to read the history of futex.c, I just can
> recommend the same thing to you to figure out why the heck this is the
> correct way to handle it.
> 
> Hint: The relevant commit starts with: cdf
> 
> The code has changed quite a bit since then, but the issue which is
> described quite well in the commit log is still the same.
> 
> Just for the record:
> 
>      Line 48 of futex.c says: "The futexes are also cursed."
> 

Thank you for your explanation (especially spend you expensive time
resources on it).

It is my fault:

  the 'ret' which return from rt_mutex_finish_proxy_lock(), is used by the next fixup_owner().


Thanks.

> Thanks,
> 
> 	tglx
> 
> 

-- 
Chen Gang

Re: [PATCH] kernel/futex.c: notice the return value after rt_mutex_finish_proxy_lock() fails

[Chen Gang]

After read the code again, I have addtional opinion for discussing,
please check thanks.

The related contents are at bottom.

On 09/13/2013 09:52 AM, Chen Gang wrote:
> On 09/13/2013 07:36 AM, Thomas Gleixner wrote:
>> That crusade does not involve any failure analysis or test cases. It's
>> just driven by mechanically checking the code for inconsistencies. Now
>> he tripped over a non obvious return value chain in the futex code. So
>> instead of figuring out why it is coded this way, he just mechanically
>> decided that there is a missing check. Though:
>>
>> The return value is checked and it needs deep understanding of the way
>> how futexes work to grok why it's necessary to invoke fixup_owner()
>> independent of the rt_mutex_finish_proxy_lock() return value.
>>
>> The code in question is:
>>
>> 	ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1);
>>
>> 	spin_lock(q.lock_ptr);
>> 	/*
>> 	 * Fixup the pi_state owner and possibly acquire the lock if we
>> 	 * haven't already.
>> 	 */
>> 	res = fixup_owner(uaddr2, &q, !ret);
>> 	/*
>> 	 * If fixup_owner() returned an error, proprogate that.  If it
>> 	 * acquired the lock, clear -ETIMEDOUT or -EINTR. 
>> 	 */
>> 	if (res)
>> 		ret = (res < 0) ? res : 0;
>>
>> If you can understand the comments in the code and you are able to
>> follow the implementation of fixup_owner() and the usage of "!ret" as
>> an argument you really should be able to figure out, why this is
>> correct.
>>
>> I'm well aware, as you are, that this code is hard to grok. BUT:
>>
>> If this code in futex_wait_requeue_pi() is wrong why did Chen's
>> correctness checker not trigger on the following code in
>> futex_lock_pi()?:
>>
>> 	if (!trylock)
>> 		ret = rt_mutex_timed_lock(&q.pi_state->pi_mutex, to, 1);
>> 	else {
>> 		ret = rt_mutex_trylock(&q.pi_state->pi_mutex);
>> 		/* Fixup the trylock return value: */
>> 		ret = ret ? 0 : -EWOULDBLOCK;
>> 	}
>>
>> 	spin_lock(q.lock_ptr);
>> 	/*
>> 	 * Fixup the pi_state owner and possibly acquire the lock if we
>> 	 * haven't already.
>> 	 */
>> 	res = fixup_owner(uaddr, &q, !ret);
>> 	/*
>> 	 * If fixup_owner() returned an error, proprogate that.  If it acquired
>> 	 * the lock, clear our -ETIMEDOUT or -EINTR.
>> 	 */
>> 	if (res)
>> 		ret = (res < 0) ? res : 0;
>>
>> It's the very same pattern and according to Chen's logic broken as
>> well.
>>
>> As I recommended to Chen to read the history of futex.c, I just can
>> recommend the same thing to you to figure out why the heck this is the
>> correct way to handle it.
>>
>> Hint: The relevant commit starts with: cdf
>>
>> The code has changed quite a bit since then, but the issue which is
>> described quite well in the commit log is still the same.
>>
>> Just for the record:
>>
>>      Line 48 of futex.c says: "The futexes are also cursed."
>>

fixup_owner() can return 0 for "success, lock not taken".

If rt_mutex_finish_proxy_lock() fail (ret !=0), fixup_owner() may also
return 0 (and may printk error message in it), 'ret' will still hold the
original error code, and continue.

Is that OK? (for the next checking statement "if (ret == -EFAULT)",
according to its comments near above, "if fixup_pi_state_owner() faulted
...", it seems we need skip it in our case).


Thanks.

> 
> Thank you for your explanation (especially spend you expensive time
> resources on it).
> 
> It is my fault:
> 
>   the 'ret' which return from rt_mutex_finish_proxy_lock(), is used by the next fixup_owner().
> 
> 
> Thanks.
> 
>> Thanks,
>>
>> 	tglx
>>
>>
> 


-- 
Chen Gang

Re: [PATCH] kernel/futex.c: notice the return value after rt_mutex_finish_proxy_lock() fails

[Chen Gang]

In futex_lock_pi(), after rt_mutex_timed_lock() fails, the followed
fixup_owner() still may return 0 to express "success, lock not taken"
(may printk kernel error in it).

When it happens, 'res' is zero, 'ret' is still none-zero, and
"rt_mutex_owner(&q.pi_state->pi_mutex) == current", it will call
rt_mutex_unlock().

At least, they are conflict with the comment near above the related
checking statements. and one possible fix is below (only a demo for
discussion, not real patch for applying).


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/futex.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index c3a1a55..64e7100 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2071,7 +2071,7 @@ retry_private:
 	 * If fixup_owner() faulted and was unable to handle the fault, unlock
 	 * it and return the fault to userspace.
 	 */
-	if (ret && (rt_mutex_owner(&q.pi_state->pi_mutex) == current))
+	if (ret && res && (rt_mutex_owner(&q.pi_state->pi_mutex) == current))
 		rt_mutex_unlock(&q.pi_state->pi_mutex);
 
 	/* Unqueue and drop the lock */
-- 
1.7.7.6


On 10/07/2013 01:14 PM, Chen Gang wrote:
> 
> After read the code again, I have addtional opinion for discussing,
> please check thanks.
> 
> The related contents are at bottom.
> 
> On 09/13/2013 09:52 AM, Chen Gang wrote:
>> On 09/13/2013 07:36 AM, Thomas Gleixner wrote:
>>> That crusade does not involve any failure analysis or test cases. It's
>>> just driven by mechanically checking the code for inconsistencies. Now
>>> he tripped over a non obvious return value chain in the futex code. So
>>> instead of figuring out why it is coded this way, he just mechanically
>>> decided that there is a missing check. Though:
>>>
>>> The return value is checked and it needs deep understanding of the way
>>> how futexes work to grok why it's necessary to invoke fixup_owner()
>>> independent of the rt_mutex_finish_proxy_lock() return value.
>>>
>>> The code in question is:
>>>
>>> 	ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1);
>>>
>>> 	spin_lock(q.lock_ptr);
>>> 	/*
>>> 	 * Fixup the pi_state owner and possibly acquire the lock if we
>>> 	 * haven't already.
>>> 	 */
>>> 	res = fixup_owner(uaddr2, &q, !ret);
>>> 	/*
>>> 	 * If fixup_owner() returned an error, proprogate that.  If it
>>> 	 * acquired the lock, clear -ETIMEDOUT or -EINTR. 
>>> 	 */
>>> 	if (res)
>>> 		ret = (res < 0) ? res : 0;
>>>
>>> If you can understand the comments in the code and you are able to
>>> follow the implementation of fixup_owner() and the usage of "!ret" as
>>> an argument you really should be able to figure out, why this is
>>> correct.
>>>
>>> I'm well aware, as you are, that this code is hard to grok. BUT:
>>>
>>> If this code in futex_wait_requeue_pi() is wrong why did Chen's
>>> correctness checker not trigger on the following code in
>>> futex_lock_pi()?:
>>>
>>> 	if (!trylock)
>>> 		ret = rt_mutex_timed_lock(&q.pi_state->pi_mutex, to, 1);
>>> 	else {
>>> 		ret = rt_mutex_trylock(&q.pi_state->pi_mutex);
>>> 		/* Fixup the trylock return value: */
>>> 		ret = ret ? 0 : -EWOULDBLOCK;
>>> 	}
>>>
>>> 	spin_lock(q.lock_ptr);
>>> 	/*
>>> 	 * Fixup the pi_state owner and possibly acquire the lock if we
>>> 	 * haven't already.
>>> 	 */
>>> 	res = fixup_owner(uaddr, &q, !ret);
>>> 	/*
>>> 	 * If fixup_owner() returned an error, proprogate that.  If it acquired
>>> 	 * the lock, clear our -ETIMEDOUT or -EINTR.
>>> 	 */
>>> 	if (res)
>>> 		ret = (res < 0) ? res : 0;
>>>
>>> It's the very same pattern and according to Chen's logic broken as
>>> well.
>>>
>>> As I recommended to Chen to read the history of futex.c, I just can
>>> recommend the same thing to you to figure out why the heck this is the
>>> correct way to handle it.
>>>
>>> Hint: The relevant commit starts with: cdf
>>>
>>> The code has changed quite a bit since then, but the issue which is
>>> described quite well in the commit log is still the same.
>>>
>>> Just for the record:
>>>
>>>      Line 48 of futex.c says: "The futexes are also cursed."
>>>
> 
> fixup_owner() can return 0 for "success, lock not taken".
> 
> If rt_mutex_finish_proxy_lock() fail (ret !=0), fixup_owner() may also
> return 0 (and may printk error message in it), 'ret' will still hold the
> original error code, and continue.
> 
> Is that OK? (for the next checking statement "if (ret == -EFAULT)",
> according to its comments near above, "if fixup_pi_state_owner() faulted
> ...", it seems we need skip it in our case).
> 
> 
> Thanks.
> 
>>
>> Thank you for your explanation (especially spend you expensive time
>> resources on it).
>>
>> It is my fault:
>>
>>   the 'ret' which return from rt_mutex_finish_proxy_lock(), is used by the next fixup_owner().
>>
>>
>> Thanks.
>>
>>> Thanks,
>>>
>>> 	tglx
>>>
>>>
>>
> 
> 


-- 
Chen Gang

Re: [PATCH] kernel/futex.c: notice the return value after rt_mutex_finish_proxy_lock() fails

[Chen Gang]

On 09/13/2013 06:37 AM, Darren Hart wrote:
> On Thu, 2013-09-12 at 16:32 +0200, Thomas Gleixner wrote:
>> On Tue, 20 Aug 2013, Chen Gang wrote:
>>
>>> rt_mutex_finish_proxy_lock() can return failure code (e.g. -EINTR,
>>> -ETIMEDOUT).
>>>
>>> Original implementation has already noticed about it, but not check it
>>> before next work.
>>>
>>> Also let coments within 80 columns to pass "./scripts/checkpatch.pl".
>>>
>>>
>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>> ---
>>>  kernel/futex.c |   30 ++++++++++++++++--------------
>>>  1 files changed, 16 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/kernel/futex.c b/kernel/futex.c
>>> index c3a1a55..1a94e7d 100644
>>> --- a/kernel/futex.c
>>> +++ b/kernel/futex.c
>>> @@ -2373,21 +2373,23 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
>>>               ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1);
>>>               debug_rt_mutex_free_waiter(&rt_waiter);
>>>  
>>> -             spin_lock(q.lock_ptr);
>>> -             /*
>>> -              * Fixup the pi_state owner and possibly acquire the lock if we
>>> -              * haven't already.
>>> -              */
>>> -             res = fixup_owner(uaddr2, &q, !ret);
>>> -             /*
>>> -              * If fixup_owner() returned an error, proprogate that.  If it
>>> -              * acquired the lock, clear -ETIMEDOUT or -EINTR.
>>> -              */
>>> -             if (res)
>>> -                     ret = (res < 0) ? res : 0;
>>> +             if (!ret) {
>>
>> Again. This is completely wrong!  
>>
>> We MUST call fixup_owner even if finish_proxy_lock() returned with an
>> error code. Simply because finish_proxy_lock() is called outside of
>> the spin_lock(q.lock_ptr) region and another thread might have
>> modified the futex state. So we need to handle the corner cases
>> otherwise we might leave the futex in some undefined state.
>>
>> You're reintroducing a hard to decode bug, which got analyzed and
>> fixed in futex_lock_pi() years ago. See the history for the
>> explanation.
>>
>> Sigh.
>>
>>         tglx
> 
> Chen, perhaps you can let us know what the failure scenario is that you
> are trying to address with this patch. I only replied the once as I
> pointed out the corner-case and expected you to follow up with that.
> This region of code is very fragile to modifications as it has become
> more corner-cases than core logic in some places :-)
> 

Oh, thanks, it is my fault:

  the 'ret' which return from rt_mutex_finish_proxy_lock(), is used by
the next fixup_owner().

Hmm... excuse me, my English is not quite well, it seems you already
know about it, but not say straightly and directly?

next, when find/feel something wrong, can say directly, I can/should
understand it (and I need/should thank you, too), that will be more
efficient (can save both of us time resources).

:-)

> For starters, I'm not following your second sentence in the commit log.
> Can you elaborate on the following?
> 
> "Original implementation has already noticed about it, but not check it
> before next work."
> 
> Do you have a test-case that demonstrates a failure mode?
> 

No, I just 'found' it, and give a simply 'fix' to let related experts
check (and now, we know it is just a spam).

Hmm... for 'test', it is really an 'important thing' to me (not 'urgent
thing'), I have plan to start to use LTP (Linux Test Project) in q4 of
2013 (start at 2013-10-01).


Thanks.
-- 
Chen Gang