From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <stable-owner@vger.kernel.org>
MIME-Version: 1.0
References: <20181127211634.4995-1-willy@infradead.org> <20181127211634.4995-3-willy@infradead.org>
 <20181128115330.GB15604@quack2.suse.cz>
In-Reply-To: <20181128115330.GB15604@quack2.suse.cz>
From: Dan Williams <dan.j.williams@intel.com>
Date: Wed, 28 Nov 2018 09:08:40 -0800
Message-ID: <CAPcyv4iqYNq1FMwU9W-oq+2mWGHotK=Ff9YvEyJOxRz3E+eDzQ@mail.gmail.com>
Subject: Re: [PATCH 2/2] dax: Don't access a freed inode
To: Jan Kara <jack@suse.cz>
Cc: Matthew Wilcox <willy@infradead.org>,
        Dave Jiang <dave.jiang@intel.com>,
        linux-nvdimm <linux-nvdimm@lists.01.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        stable <stable@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: stable-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On Wed, Nov 28, 2018 at 3:54 AM Jan Kara <jack@suse.cz> wrote:
>
> On Tue 27-11-18 13:16:34, Matthew Wilcox wrote:
> > After we drop the i_pages lock, the inode can be freed at any time.
> > The get_unlocked_entry() code has no choice but to reacquire the lock,
> > so it can't be used here.  Create a new wait_entry_unlocked() which takes
> > care not to acquire the lock or dereference the address_space in any way.
> >
> > Fixes: c2a7d2a11552 ("filesystem-dax: Introduce dax_lock_mapping_entry()")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Matthew Wilcox <willy@infradead.org>
>
> The patch looks good. You can add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
> Just one nit below:
>
> > +/*
> > + * The only thing keeping the address space around is the i_pages lock
> > + * (it's cycled in clear_inode() after removing the entries from i_pages)
> > + * After we call xas_unlock_irq(), we cannot touch xas->xa.
> > + */
> > +static void wait_entry_unlocked(struct xa_state *xas, void *entry)
> > +{
> > +     struct wait_exceptional_entry_queue ewait;
> > +     wait_queue_head_t *wq;
> > +
> > +     init_wait(&ewait.wait);
> > +     ewait.wait.func = wake_exceptional_entry_func;
> > +
> > +     wq = dax_entry_waitqueue(xas, entry, &ewait.key);
> > +     prepare_to_wait_exclusive(wq, &ewait.wait, TASK_UNINTERRUPTIBLE);
> > +     xas_unlock_irq(xas);
> > +     schedule();
> > +     finish_wait(wq, &ewait.wait);
>
> Can we add a comment here like:
>
>         /*
>          * Entry lock waits are exclusive. Wake up the next waiter since we
>          * aren't sure we will acquire the entry lock and thus wake the
>          * next waiter up on unlock.
>          */
>
> Because I always wonder for a moment why this is needed.

Looks good, I'll add that when applying.