From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752487AbdLNNKk (ORCPT <rfc822;w@1wt.eu>);
        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 <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 14 Dec 2017 08:10:39 -0500
Date: Thu, 14 Dec 2017 14:10:37 +0100
From: Christoph Hellwig <hch@lst.de>
To: Matthew Wilcox <willy@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
        kernel test robot <fengguang.wu@intel.com>,
        Christoph Hellwig <hch@lst.de>, LKP <lkp@01.org>,
        linux-kernel@vger.kernel.org,
        Linux Memory Management List <linux-mm@kvack.org>, wfg@linux.intel.com,
        Stephen Rothwell <sfr@canb.auug.org.au>
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: <linux-kernel.vger.kernel.org>
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.