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>
In-Reply-To: <d6a1c7378a4c666be93d22707405e0e0136a01fa.camel@hammerspace.com>
From:   Olga Kornievskaia <aglo@umich.edu>
Date:   Fri, 19 Aug 2022 10:37:26 -0400
Message-ID: <CAN-5tyHdSSfJLVff0DsW1+zq=tTxF152fA_BipN1He=q1LroZA@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 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.

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