From patchwork Wed Nov 15 21:20:21 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefano Stabellini X-Patchwork-Id: 10060285 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 D61696023A for ; Wed, 15 Nov 2017 21:23:13 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id BB2A52A30A for ; Wed, 15 Nov 2017 21:23:13 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id AF8872A319; Wed, 15 Nov 2017 21:23:13 +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 139F22A30A for ; Wed, 15 Nov 2017 21:23:11 +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 1eF56n-0006C8-B7; Wed, 15 Nov 2017 21:20:33 +0000 Received: from mail6.bemta6.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1eF56l-0006BU-OF for xen-devel@lists.xenproject.org; Wed, 15 Nov 2017 21:20:31 +0000 Received: from [85.158.143.35] (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256 bits)) by server-2.bemta-6.messagelabs.com id B5/0A-05188-F9FAC0A5; Wed, 15 Nov 2017 21:20:31 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrBIsWRWlGSWpSXmKPExsVybKJssu689Tx RBt8nCVt83zKZyYHR4/CHKywBjFGsmXlJ+RUJrBlLLhxnKWjVqZh6YzN7A+M/hS5GTg4hgVYm iWXHPUFsFgEHiVdbHjF2MXJwMArESDz4YQ0SZhQIk5h8eQkrRIm2RMOPuUwgNpuAocTfJ5vYQ MolgOwlnzlAwiICBhLvr08CCzMLlErs+68JYgoLBEjMmpINUsEpYCexsHkz2BBeAW+JQ4/vsH QxcgHdsppFYueV6ewgCVEBXYlD//6wQRQJSpyc+YQFxGYW0JJYPn0bywRGgVlIUrOQpBYwMq1 i1ChOLSpLLdI1MtZLKspMzyjJTczM0TU0MNPLTS0uTkxPzUlMKtZLzs/dxAgMPwYg2MH4Z37g IUZJDiYlUV7n39xRQnxJ+SmVGYnFGfFFpTmpxYcYZTg4lCR45dbxRAkJFqWmp1akZeYAIwEmL cHBoyTCywmS5i0uSMwtzkyHSJ1iNOY4tunyHyaOZzNfNzALseTl56VKifNmgJQKgJRmlObBDY JF6CVGWSlhXkag04R4ClKLcjNLUOVfMYpzMCoJ8/KBTOHJzCuB2/cK6BQmoFNsbnCDnFKSiJC SamAMrRHc5bOiqq6qZKG+y1mOMsY7jFNuV+3a5s5z4VbdFJ8d+741Tfw0n03O5ILxqqpKxiUR 65cJ+5ptmm8VZfNa1yzqdkJ3E8u/PXfyNPqnLEwUV5yUuJGjv4ZFj2Pu5ksTbThfC+dsEvm98 NgdvdPbnbbqPvxdOUFQ7m0qP39khBirnKCo8XklluKMREMt5qLiRACU5lXoywIAAA== X-Env-Sender: sstabellini@kernel.org X-Msg-Ref: server-11.tower-21.messagelabs.com!1510780828!79062355!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 7493 invoked from network); 15 Nov 2017 21:20:30 -0000 Received: from mail.kernel.org (HELO mail.kernel.org) (198.145.29.99) by server-11.tower-21.messagelabs.com with DHE-RSA-AES256-GCM-SHA384 encrypted SMTP; 15 Nov 2017 21:20:30 -0000 Received: from sstabellini-ThinkPad-X260 (unknown [172.56.38.180]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id E8F46218DB; Wed, 15 Nov 2017 21:20:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E8F46218DB Date: Wed, 15 Nov 2017 13:20:21 -0800 (PST) From: Stefano Stabellini X-X-Sender: sstabellini@sstabellini-ThinkPad-X260 To: Boris Ostrovsky In-Reply-To: Message-ID: References: <1510006679-6381-1-git-send-email-sstabellini@kernel.org> <12bea1d0-6504-d7c6-c47c-7cd197264536@suse.com> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Cc: Juergen Gross , xen-devel@lists.xenproject.org, 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 Wed, 15 Nov 2017, Boris Ostrovsky wrote: > On 11/15/2017 02:09 PM, Stefano Stabellini wrote: > > On Wed, 15 Nov 2017, Juergen Gross wrote: > >>>>> while(mutex_is_locked(&map->active.in_mutex.owner) || > >>>>> mutex_is_locked(&map->active.out_mutex.owner)) > >>>>> cpu_relax(); > >>>>> > >>>>> ? > >>>> I'm not convinced there isn't a race. > >>>> > >>>> In pvcalls_front_recvmsg() sock->sk->sk_send_head is being read and only > >>>> then in_mutex is taken. What happens if pvcalls_front_release() resets > >>>> sk_send_head and manages to test the mutex before the mutex is locked? > >>>> > >>>> Even in case this is impossible: the whole construct seems to be rather > >>>> fragile. > > I agree it looks fragile, and I agree that it might be best to avoid the > > usage of in_mutex and out_mutex as refcounts. More comments on this > > below. > > > > > >>> I think we can wait until pvcalls_refcount is 1 (i.e. it's only us) and > >>> not rely on mutex state. > >> Yes, this would work. > > Yes, I agree it would work and for the sake of getting something in > > shape for the merge window I am attaching a patch for it. Please go > > ahead with it. Let me know if you need anything else immediately, and > > I'll work on it ASAP. > > > > > > > > However, I should note that this is a pretty big hammer we are using: > > the refcount is global, while we only need to wait until it's only us > > _on this specific socket_. > > Can you explain why socket is important? Yes, of course: there are going to be many open sockets on a given pvcalls connection. pvcalls_refcount is global: waiting on pvcalls_refcount means waiting until any operations on any unrelated sockets stop. While we only need to wait until the operations on the one socket we want to close stop. > > > > We really need a per socket refcount. If we don't want to use the mutex > > internal counters, then we need another one. > > > > See the appended patch that introduces a per socket refcount. However, > > for the merge window, also using pvcalls_refcount is fine. > > > > The race Juergen is concerned about is only theoretically possible: > > > > recvmsg: release: > > > > test sk_send_head clear sk_send_head > > > > grab in_mutex > > > > test in_mutex > > > > Without kernel preemption is not possible for release to clear > > sk_send_head and test in_mutex after recvmsg tests sk_send_head and > > before recvmsg grabs in_mutex. > > Sorry, I don't follow --- what does preemption have to do with this? If > recvmsg and release happen on different processors the order of > operations can be > > CPU0 CPU1 > > test sk_send_head > > clear sk_send_head > > > test in_mutex > free everything > grab in_mutex > > I actually think RCU should take care of all of this. Preemption could cause something very similar to happen, but your example is very good too, even better, because it could trigger the issue even with preemption disabled. I'll think more about this and submit a separate patch on top of the simple pvcalls_refcount patch below. > But for now I will take your refcount-based patch. However, it also > needs comment update. > > How about > > /* > * We need to make sure that send/rcvmsg on this socket has not started > * before we've cleared sk_send_head here. The easiest (though not optimal) > * way to guarantee this is to see that no pvcall (other than us) is in > progress. > */ Yes, this is the patch: --- 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 times you call mutex_trylock() in_mutex is going to fail. It's an endless loop. Solve the problem by waiting until the global refcount is 1 instead (the refcount is 1 when the only active pvcalls frontend function is pvcalls_front_release). 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..54c0fda 100644 --- a/drivers/xen/pvcalls-front.c +++ b/drivers/xen/pvcalls-front.c @@ -1043,13 +1043,12 @@ int pvcalls_front_release(struct socket *sock) wake_up_interruptible(&map->active.inflight_conn_req); /* - * Wait until there are no more waiters on the mutexes. - * We know that no new waiters can be added because sk_send_head - * 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)) + * We need to make sure that sendmsg/recvmsg on this socket have + * not started before we've cleared sk_send_head here. The + * easiest (though not optimal) way to guarantee this is to see + * that no pvcall (other than us) is in progress. + */ + while (atomic_read(&pvcalls_refcount) > 1) cpu_relax(); pvcalls_front_free_map(bedata, map);