From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752487AbdLNNKk (ORCPT ); Thu, 14 Dec 2017 08:10:40 -0500 Received: from verein.lst.de ([213.95.11.211]:41726 "EHLO newverein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752414AbdLNNKj (ORCPT ); Thu, 14 Dec 2017 08:10:39 -0500 Date: Thu, 14 Dec 2017 14:10:37 +0100 From: Christoph Hellwig To: Matthew Wilcox Cc: Andrew Morton , kernel test robot , Christoph Hellwig , LKP , linux-kernel@vger.kernel.org, Linux Memory Management List , wfg@linux.intel.com, Stephen Rothwell Subject: Re: d1fc031747 ("sched/wait: assert the wait_queue_head lock is .."): EIP: __wake_up_common Message-ID: <20171214131037.GD10791@lst.de> References: <5a31cac7.i9WLKx5al8+rBn73%fengguang.wu@intel.com> <20171213170300.b0bb26900dd00641819b4872@linux-foundation.org> <20171214125809.GB30288@bombadil.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171214125809.GB30288@bombadil.infradead.org> User-Agent: Mutt/1.5.17 (2007-11-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 14, 2017 at 04:58:09AM -0800, Matthew Wilcox wrote: > On Wed, Dec 13, 2017 at 05:03:00PM -0800, Andrew Morton wrote: > > > sched/wait: assert the wait_queue_head lock is held in __wake_up_common > > > > > > Better ensure we actually hold the lock using lockdep than just commenting > > > on it. Due to the various exported _locked interfaces it is far too easy > > > to get the locking wrong. > > > > I'm probably sitting on an older version. I've dropped > > > > epoll: use the waitqueue lock to protect ep->wq > > sched/wait: assert the wait_queue_head lock is held in __wake_up_common > > Looks pretty clear to me that userfaultfd is also abusing the wake_up_locked > interfaces: > > spin_lock(&ctx->fault_pending_wqh.lock); > __wake_up_locked_key(&ctx->fault_pending_wqh, TASK_NORMAL, &range); > __wake_up_locked_key(&ctx->fault_wqh, TASK_NORMAL, &range); > spin_unlock(&ctx->fault_pending_wqh.lock); > > Sure, it's locked, but not by the lock you thought it was going to be. > > There doesn't actually appear to be a bug here; fault_wqh is always serialised > by fault_pending_wqh.lock, but lockdep can't know that. I think this patch > will solve the problem. Or userfaultfd could just always use the waitqueue lock, similar to what we are doing in epoll. But unless someone care about micro-optimizatations I'm tempted to add your patch to the next iteration of the series. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f200.google.com (mail-wr0-f200.google.com [209.85.128.200]) by kanga.kvack.org (Postfix) with ESMTP id BFA9C6B025E for ; Thu, 14 Dec 2017 08:10:39 -0500 (EST) Received: by mail-wr0-f200.google.com with SMTP id l33so3201152wrl.5 for ; Thu, 14 Dec 2017 05:10:39 -0800 (PST) Received: from newverein.lst.de (verein.lst.de. [213.95.11.211]) by mx.google.com with ESMTPS id z12si3275208wrb.55.2017.12.14.05.10.38 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 14 Dec 2017 05:10:38 -0800 (PST) Date: Thu, 14 Dec 2017 14:10:37 +0100 From: Christoph Hellwig Subject: Re: d1fc031747 ("sched/wait: assert the wait_queue_head lock is .."): EIP: __wake_up_common Message-ID: <20171214131037.GD10791@lst.de> References: <5a31cac7.i9WLKx5al8+rBn73%fengguang.wu@intel.com> <20171213170300.b0bb26900dd00641819b4872@linux-foundation.org> <20171214125809.GB30288@bombadil.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171214125809.GB30288@bombadil.infradead.org> Sender: owner-linux-mm@kvack.org List-ID: To: Matthew Wilcox Cc: Andrew Morton , kernel test robot , Christoph Hellwig , LKP , linux-kernel@vger.kernel.org, Linux Memory Management List , wfg@linux.intel.com, Stephen Rothwell On Thu, Dec 14, 2017 at 04:58:09AM -0800, Matthew Wilcox wrote: > On Wed, Dec 13, 2017 at 05:03:00PM -0800, Andrew Morton wrote: > > > sched/wait: assert the wait_queue_head lock is held in __wake_up_common > > > > > > Better ensure we actually hold the lock using lockdep than just commenting > > > on it. Due to the various exported _locked interfaces it is far too easy > > > to get the locking wrong. > > > > I'm probably sitting on an older version. I've dropped > > > > epoll: use the waitqueue lock to protect ep->wq > > sched/wait: assert the wait_queue_head lock is held in __wake_up_common > > Looks pretty clear to me that userfaultfd is also abusing the wake_up_locked > interfaces: > > spin_lock(&ctx->fault_pending_wqh.lock); > __wake_up_locked_key(&ctx->fault_pending_wqh, TASK_NORMAL, &range); > __wake_up_locked_key(&ctx->fault_wqh, TASK_NORMAL, &range); > spin_unlock(&ctx->fault_pending_wqh.lock); > > Sure, it's locked, but not by the lock you thought it was going to be. > > There doesn't actually appear to be a bug here; fault_wqh is always serialised > by fault_pending_wqh.lock, but lockdep can't know that. I think this patch > will solve the problem. Or userfaultfd could just always use the waitqueue lock, similar to what we are doing in epoll. But unless someone care about micro-optimizatations I'm tempted to add your patch to the next iteration of the series. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============0184247200085284238==" MIME-Version: 1.0 From: Christoph Hellwig To: lkp@lists.01.org Subject: Re: d1fc031747 ("sched/wait: assert the wait_queue_head lock is .."): EIP: __wake_up_common Date: Thu, 14 Dec 2017 14:10:37 +0100 Message-ID: <20171214131037.GD10791@lst.de> In-Reply-To: <20171214125809.GB30288@bombadil.infradead.org> List-Id: --===============0184247200085284238== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable On Thu, Dec 14, 2017 at 04:58:09AM -0800, Matthew Wilcox wrote: > On Wed, Dec 13, 2017 at 05:03:00PM -0800, Andrew Morton wrote: > > > sched/wait: assert the wait_queue_head lock is held in __wake_up_= common > > > = > > > Better ensure we actually hold the lock using lockdep than just c= ommenting > > > on it. Due to the various exported _locked interfaces it is far = too easy > > > to get the locking wrong. > > = > > I'm probably sitting on an older version. I've dropped > > = > > epoll: use the waitqueue lock to protect ep->wq > > sched/wait: assert the wait_queue_head lock is held in __wake_up_common > = > Looks pretty clear to me that userfaultfd is also abusing the wake_up_loc= ked > interfaces: > = > spin_lock(&ctx->fault_pending_wqh.lock); > __wake_up_locked_key(&ctx->fault_pending_wqh, TASK_NORMAL, &range= ); > __wake_up_locked_key(&ctx->fault_wqh, TASK_NORMAL, &range); > spin_unlock(&ctx->fault_pending_wqh.lock); > = > Sure, it's locked, but not by the lock you thought it was going to be. > = > There doesn't actually appear to be a bug here; fault_wqh is always seria= lised > by fault_pending_wqh.lock, but lockdep can't know that. I think this pat= ch > will solve the problem. Or userfaultfd could just always use the waitqueue lock, similar to what we are doing in epoll. But unless someone care about micro-optimizatations I'm tempted to add your patch to the next iteration of the series. --===============0184247200085284238==--