From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from zeniv.linux.org.uk ([195.92.253.2]:39768 "EHLO
        ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1727436AbeI3Jt5 (ORCPT
        <rfc822;linux-fsdevel@vger.kernel.org>);
        Sun, 30 Sep 2018 05:49:57 -0400
Date: Sun, 30 Sep 2018 04:17:52 +0100
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Matthew Wilcox <willy@infradead.org>
Cc: NeilBrown <neilb@suse.com>, linux-fsdevel@vger.kernel.org
Subject: Re: [RFC] bloody odd logics in md_exit()
Message-ID: <20180930031750.GK32577@ZenIV.linux.org.uk>
References: <20180929033334.GG32577@ZenIV.linux.org.uk>
 <87k1n3gb2s.fsf@notabene.neil.brown.name>
 <20180930021356.GA32010@bombadil.infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180930021356.GA32010@bombadil.infradead.org>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On Sat, Sep 29, 2018 at 07:13:56PM -0700, Matthew Wilcox wrote:
> On Sun, Sep 30, 2018 at 09:04:11AM +1000, NeilBrown wrote:
> > On Sat, Sep 29 2018, Al Viro wrote:
> > > 	Rationale in e2f23b606b94 (md: avoid oops on unload if some
> > > process is in poll or select) is very odd.  Waitqueue code _does_
> > > provide a way to remove all listeners from a waitqueue - it's simply
> > > wake_up_all().  Once the wakeup callback has been executed (and it
> > > runs in context of wake_up_all() caller), we don't *care* if md.o
> > > is still there - all waiters are gone from the queue and the callback
> > > (pollwake() and friends) doesn't reinsert them.
> >
> >  I don't think wake_up_all() does remove anything from the queue.
> >  It simply wakes up the various processes that are waiting.
> >  They remain on the queue until they call remove_wait_queue(), which
> >  could be delayed arbitrarily.
> >  If it was delayed until after the module was unloaded and
> >  "md_event_waiters" no longer existed, the unlink attempt would cause an
> >  invalid memory access.
> 
> init_wait_entry() initialises wq_entry->func to autoremove_wake_function
> which calls list_del_init() when it's called from __wake_up_common().
> If we look at the AIO path, it sets ->func to aio_poll_wake() which
> also calls list_del_init().  So I think Al is right, but I haven't
> looked at _every_ code path.

Alas, no - poll_wait() does
        init_waitqueue_func_entry(&entry->wait, pollwake);
and pollwake() does not do autoremove.  Worse, the way wakeups are done in
actual drivers, we _can't_ do autoremove there - it is possible to get
several non-specific wakeups for the same file before we finally get the
condition select(2) is waiting for.

Look, for example, at drivers/char/virtio_console.c; the same queue is used
for poll, for read waiting for data and for write waiting for possiblity to
send.  Worse, there's no obvious way to tell which conditions change in given
wakeup source.

And yeah, we do have broken stuff - there's four hundred or so instances, so
I didn't get anywhere near complete audit, but we do have some buggered ones.
Both on rmmod and, at least in one case, on pcmcia card removal...