On Thu, Aug 18, 2022 at 8:17 PM Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> 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@xxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
>
>
