Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <166086695960.5425.17748020864798390841@noble.neil.brown.name>
 <d6a1c7378a4c666be93d22707405e0e0136a01fa.camel@hammerspace.com> <CAN-5tyHdSSfJLVff0DsW1+zq=tTxF152fA_BipN1He=q1LroZA@mail.gmail.com>
In-Reply-To: <CAN-5tyHdSSfJLVff0DsW1+zq=tTxF152fA_BipN1He=q1LroZA@mail.gmail.com>
From:   Olga Kornievskaia <aglo@umich.edu>
Date:   Fri, 19 Aug 2022 20:07:52 -0400
Message-ID: <CAN-5tyFFXGadpyT0mR7-YTAQmzi0w7k950T6Tnh=KhnZ1OF+Rw@mail.gmail.com>
Subject: Re: [PATCH v2] NFS: unlink/rmdir shouldn't call d_delete() twice on ENOENT
To:     Trond Myklebust <trondmy@hammerspace.com>
Cc:     "neilb@suse.de" <neilb@suse.de>,
        "hooanon05g@gmail.com" <hooanon05g@gmail.com>,
        "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Aug 19, 2022 at 10:37 AM Olga Kornievskaia <aglo@umich.edu> wrote:
>
> On Thu, Aug 18, 2022 at 8:17 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
> >
> > On Fri, 2022-08-19 at 09:55 +1000, NeilBrown wrote:
> > >
> > > nfs_unlink() calls d_delete() twice if it receives ENOENT from the
> > > server - once in nfs_dentry_handle_enoent() from nfs_safe_remove and
> > > once in nfs_dentry_remove_handle_error().
> > >
> > > nfs_rmddir() also calls it twice - the nfs_dentry_handle_enoent()
> > > call
> > > is direct and inside a region locked with ->rmdir_sem
> > >
> > > It is safe to call d_delete() twice if the refcount > 1 as the dentry
> > > is
> > > simply unhashed.
> > > If the refcount is 1, the first call sets d_inode to NULL and the
> > > second
> > > call crashes.
> > >
> > > This patch guards the d_delete() call from nfs_dentry_handle_enoent()
> > > leaving the one under ->remdir_sem in case that is important.
> > >
> > > In mainline it would be safe to remove the d_delete() call.  However
> > > in
> > > older kernels to which this might be backported, that would change
> > > the
> > > behaviour of nfs_unlink().  nfs_unlink() used to unhash the dentry
> > > which
> > > resulted in nfs_dentry_handle_enoent() not calling d_delete().  So in
> > > older kernels we need the d_delete() in
> > > nfs_dentry_remove_handle_error()
> > > when called from nfs_unlink() but not when called from nfs_rmdir().
> > >
> > > To make the code work correctly for old and new kernels, and from
> > > both
> > > nfs_unlink() and nfs_rmdir(), we protect the d_delete() call with
> > > simple_positive().  This ensures it is never called in a circumstance
> > > where it could crash.
> > >
> > > Fixes: 3c59366c207e ("NFS: don't unhash dentry during unlink/rename")
> > > Fixes: 9019fb391de0 ("NFS: Label the dentry with a verifier in
> > > nfs_rmdir() and nfs_unlink()")
> > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > ---
> > >  fs/nfs/dir.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > > index dbab3caa15ed..8f26f848818d 100644
> > > --- a/fs/nfs/dir.c
> > > +++ b/fs/nfs/dir.c
> > > @@ -2382,7 +2382,8 @@ static void
> > > nfs_dentry_remove_handle_error(struct inode *dir,
> > >  {
> > >         switch (error) {
> > >         case -ENOENT:
> > > -               d_delete(dentry);
> > > +               if (d_really_is_positive(dentry))
> > > +                       d_delete(dentry);
> > >                 nfs_set_verifier(dentry,
> > > nfs_save_change_attribute(dir));
> > >                 break;
> > >         case 0:
> >
> > OK. I've kicked v1 out of my linux-next branch, and applied v2 to my
> > testing branch. I'll try to give it some testing tomorrow.
> >
> > Olga, will you be able to test v2 to see if it fixes your bug report as
> > well?
>
> Will do.

Finished testing successfullly. Tested-by.

>
> >
> > --
> > Trond Myklebust
> > Linux NFS client maintainer, Hammerspace
> > trond.myklebust@hammerspace.com
> >
> >