Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <166063173439.5425.8345694210902041629@noble.neil.brown.name>
In-Reply-To: <166063173439.5425.8345694210902041629@noble.neil.brown.name>
From:   Olga Kornievskaia <aglo@umich.edu>
Date:   Thu, 18 Aug 2022 17:41:01 -0400
Message-ID: <CAN-5tyHdCigw38VYXFCOdsG2gcKfLp9jUFm_MFLK0PQi+vpjrg@mail.gmail.com>
Subject: Re: [PATCH] NFS: unlink/rmdir shouldn't call d_delete() twice on ENOENT
To:     NeilBrown <neilb@suse.de>
Cc:     Trond Myklebust <trondmy@hammerspace.com>,
        Anna Schumaker <anna@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Aug 16, 2022 at 4:20 AM NeilBrown <neilb@suse.de> 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.
>
> Refcount is almost always 1 in nfs_unlink() so this must almost never
> happen else we would have seen crashes before.
>
> This patch removes the d_delete() call from nfs_dentry_handle_enoent()
> so as to leave the one under ->remdir_sem in case that is important.
>
> Fixes: 9019fb391de0 ("NFS: Label the dentry with a verifier in nfs_rmdir() and nfs_unlink()")
> Signed-off-by: NeilBrown <neilb@suse.de>

This patch addresses the problem I previously reported. So Tested-by.

> ---
>  fs/nfs/dir.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index dbab3caa15ed..4648b421025c 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -2382,7 +2382,6 @@ static void nfs_dentry_remove_handle_error(struct inode *dir,
>  {
>         switch (error) {
>         case -ENOENT:
> -               d_delete(dentry);
>                 nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
>                 break;
>         case 0:
> --
> 2.37.1
>