From patchwork Fri Nov 10 23:57:57 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefano Stabellini X-Patchwork-Id: 10053955 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id BC54E60637 for ; Sat, 11 Nov 2017 00:00:48 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id AF78B2B526 for ; Sat, 11 Nov 2017 00:00:48 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id A39902B52B; Sat, 11 Nov 2017 00:00:48 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 1C4402B526 for ; Sat, 11 Nov 2017 00:00:47 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1eDJBT-0006xF-VG; Fri, 10 Nov 2017 23:58:03 +0000 Received: from mail6.bemta6.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1eDJBT-0006x9-0K for xen-devel@lists.xenproject.org; Fri, 10 Nov 2017 23:58:03 +0000 Received: from [85.158.143.35] by server-11.bemta-6.messagelabs.com id B8/DC-20813-A0D360A5; Fri, 10 Nov 2017 23:58:02 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrBIsWRWlGSWpSXmKPExsVybKJssi6nLVu Uwd6Fqhbft0xmcmD0OPzhCksAYxRrZl5SfkUCa8b0/5EF16Qr9l5qZ2pgbBLvYuTiEBJYwiRx 4P8sxi5GTg4WAQeJ9W9uANkcHIwCMRIPfliDhBkFwiQmX17CClGiLXH6RhMTiM0mYCjx98kmN pByCSB7yWcOkLCIgLLEx9ZedhCbWaBA4t2L78wgJcICARKzpmSDhDkFbCSaD60EW8or4C2x7P EiNhBbSKBU4uXfVrC4qICuxKF/f9ggagQlTs58wgIxUkti+fRtLBMYBWYhSc1CklrAyLSKUaM 4tagstUjXyEwvqSgzPaMkNzEzR9fQwEwvN7W4ODE9NScxqVgvOT93EyMw/BiAYAfjmQWBhxgl OZiURHkNL7BGCfEl5adUZiQWZ8QXleakFh9ilOHgUJLgbbJmixISLEpNT61Iy8wBRgJMWoKDR 0mE1xkkzVtckJhbnJkOkTrFaMxxbNPlP0wcz2a+bmAWYsnLz0uVEudtBSkVACnNKM2DGwSL0E uMslLCvIxApwnxFKQW5WaWoMq/YhTnYFQS5p0IMoUnM68Ebt8roFOYgE6JZmcBOaUkESEl1cA oNjPuojivwWte8fuNXxjnFjmkZz81OsyUeenX+398HmcTtGzd+he13e5b2auqb54WelKEsXi9 467itpJ/wY4BCk3GTy8mXjj7q15FeJOyvs+JNwrfmct1xX0iLJseJYu8vn5I232Dzpy5au9kz 36NmHUhm+/+n8zw1coxn1dNWnrriKGB7j8lluKMREMt5qLiRAB2uSAnywIAAA== X-Env-Sender: sstabellini@kernel.org X-Msg-Ref: server-6.tower-21.messagelabs.com!1510358280!58451706!1 X-Originating-IP: [198.145.29.99] X-SpamReason: No, hits=0.0 required=7.0 tests= X-StarScan-Received: X-StarScan-Version: 9.4.45; banners=-,-,- X-VirusChecked: Checked Received: (qmail 40924 invoked from network); 10 Nov 2017 23:58:01 -0000 Received: from mail.kernel.org (HELO mail.kernel.org) (198.145.29.99) by server-6.tower-21.messagelabs.com with DHE-RSA-AES256-GCM-SHA384 encrypted SMTP; 10 Nov 2017 23:58:01 -0000 Received: from [10.149.184.130] (162-198-228-33.lightspeed.wlfrct.sbcglobal.net [162.198.228.33]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 8572F214C5; Fri, 10 Nov 2017 23:57:59 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8572F214C5 Date: Fri, 10 Nov 2017 15:57:57 -0800 (PST) From: Stefano Stabellini X-X-Sender: sstabellini@sstabellini-ThinkPad-X260 To: Juergen Gross In-Reply-To: Message-ID: References: <1510006679-6381-1-git-send-email-sstabellini@kernel.org> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Cc: xen-devel@lists.xenproject.org, boris.ostrovsky@oracle.com, Stefano Stabellini Subject: Re: [Xen-devel] [PATCH] xen/pvcalls: fix potential endless loop in pvcalls-front.c X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Tue, 7 Nov 2017, Juergen Gross wrote: > On 06/11/17 23:17, Stefano Stabellini wrote: > > mutex_trylock() returns 1 if you take the lock and 0 if not. Assume you > > take in_mutex on the first try, but you can't take out_mutex. Next times > > you call mutex_trylock() in_mutex is going to fail. It's an endless > > loop. > > > > Solve the problem by moving the two mutex_trylock calls to two separate > > loops. > > > > Reported-by: Dan Carpenter > > Signed-off-by: Stefano Stabellini > > CC: boris.ostrovsky@oracle.com > > CC: jgross@suse.com > > --- > > drivers/xen/pvcalls-front.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c > > index 0c1ec68..047dce7 100644 > > --- a/drivers/xen/pvcalls-front.c > > +++ b/drivers/xen/pvcalls-front.c > > @@ -1048,8 +1048,9 @@ int pvcalls_front_release(struct socket *sock) > > * is set to NULL -- we only need to wait for the existing > > * waiters to return. > > */ > > - while (!mutex_trylock(&map->active.in_mutex) || > > - !mutex_trylock(&map->active.out_mutex)) > > + while (!mutex_trylock(&map->active.in_mutex)) > > + cpu_relax(); > > + while (!mutex_trylock(&map->active.out_mutex)) > > cpu_relax(); > > Any reason you don't just use mutex_lock()? Hi Juergen, sorry for the late reply. Yes, you are right. Given the patch, it would be just the same to use mutex_lock. This is where I realized that actually we have a problem: no matter if we use mutex_lock or mutex_trylock, there are no guarantees that we'll be the last to take the in/out_mutex. Other waiters could be still outstanding. We solved the same problem using a refcount in pvcalls_front_remove. In this case, I was thinking of reusing the mutex internal counter for efficiency, instead of adding one more refcount. For using the mutex as a refcount, there is really no need to call mutex_trylock or mutex_lock. I suggest checking on the mutex counter directly: while (atomic_long_read(&map->active.in_mutex.owner) != 0UL || atomic_long_read(&map->active.out_mutex.owner) != 0UL) cpu_relax(); Cheers, Stefano --- xen/pvcalls: fix potential endless loop in pvcalls-front.c mutex_trylock() returns 1 if you take the lock and 0 if not. Assume you take in_mutex on the first try, but you can't take out_mutex. Next time you call mutex_trylock() in_mutex is going to fail. It's an endless loop. Actually, we don't want to use mutex_trylock at all: we don't need to take the mutex, we only need to wait until the last mutex waiter/holder releases it. Instead of calling mutex_trylock or mutex_lock, just check on the mutex refcount instead. Reported-by: Dan Carpenter Signed-off-by: Stefano Stabellini CC: boris.ostrovsky@oracle.com CC: jgross@suse.com diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c index 0c1ec68..9f33cb8 100644 --- a/drivers/xen/pvcalls-front.c +++ b/drivers/xen/pvcalls-front.c @@ -1048,8 +1048,8 @@ int pvcalls_front_release(struct socket *sock) * is set to NULL -- we only need to wait for the existing * waiters to return. */ - while (!mutex_trylock(&map->active.in_mutex) || - !mutex_trylock(&map->active.out_mutex)) + while (atomic_long_read(&map->active.in_mutex.owner) != 0UL || + atomic_long_read(&map->active.out_mutex.owner) != 0UL) cpu_relax(); pvcalls_front_free_map(bedata, map);