Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752045Ab3JGFPc (ORCPT ); Mon, 7 Oct 2013 01:15:32 -0400 Received: from intranet.asianux.com ([58.214.24.6]:56066 "EHLO intranet.asianux.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751691Ab3JGFPb (ORCPT ); Mon, 7 Oct 2013 01:15:31 -0400 X-Spam-Score: -100.7 Message-ID: <52524333.70107@asianux.com> Date: Mon, 07 Oct 2013 13:14:27 +0800 From: Chen Gang User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130110 Thunderbird/17.0.2 MIME-Version: 1.0 To: Thomas Gleixner CC: Darren Hart , ccross@android.com, Mel Gorman , Andrew Morton , "linux-kernel@vger.kernel.org" , Li Zefan , Joe Perches Subject: Re: [PATCH] kernel/futex.c: notice the return value after rt_mutex_finish_proxy_lock() fails References: <5212DD71.7070208@asianux.com> <1379025421.1285.55.camel@dvhart-mobl4.amr.corp.intel.com> <52326FDE.5010303@asianux.com> In-Reply-To: <52326FDE.5010303@asianux.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3781 Lines: 123 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 -- 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/