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?
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
