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

[Chen Gang <gang.chen@asianux.com>]

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