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 <[email protected]>
---
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:
--
2.37.1
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 <[email protected]>
> ---
> 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?
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On Thu, Aug 18, 2022 at 8:17 PM Trond Myklebust <[email protected]> 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 <[email protected]>
> > ---
> > 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
> [email protected]
>
>
On Fri, Aug 19, 2022 at 10:37 AM Olga Kornievskaia <[email protected]> wrote:
>
> On Thu, Aug 18, 2022 at 8:17 PM Trond Myklebust <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
> > [email protected]
> >
> >
On Fri, 2022-08-19 at 20:07 -0400, Olga Kornievskaia wrote:
> On Fri, Aug 19, 2022 at 10:37 AM Olga Kornievskaia <[email protected]>
> wrote:
> >
> > On Thu, Aug 18, 2022 at 8:17 PM Trond Myklebust
> > <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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
[email protected]