On Fri, 2022-08-19 at 20:07 -0400, Olga Kornievskaia wrote:
> 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.
Thanks!
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
