On Fri, Aug 19, 2022 at 10:37 AM Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
>
> 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.
Finished testing successfullly. Tested-by.
>
> >
> > --
> > Trond Myklebust
> > Linux NFS client maintainer, Hammerspace
> > trond.myklebust@xxxxxxxxxxxxxxx
> >
> >
