Re: [PATCH] nfsd: fix dentry leak upon mkdir failure.

[bfields@xxxxxxxxxxxx (J. Bruce Fields)]

On Mon, Aug 12, 2019 at 04:03:54AM +0100, Al Viro wrote:
> On Mon, Aug 12, 2019 at 11:16:11AM +0900, Tetsuo Handa wrote:
> > syzbot is reporting that nfsd_mkdir() forgot to remove dentry created by
> > d_alloc_name() when __nfsd_mkdir() failed (due to memory allocation fault
> > injection) [1].
> 
> That's not the only problem I see there.
>         ret = __nfsd_mkdir(d_inode(parent), dentry, S_IFDIR | 0600);
>         if (ret)
>                 goto out_err;
>         if (ncl) {
>                 d_inode(dentry)->i_private = ncl;
>                 kref_get(&ncl->cl_ref);
>         }
> and we are doing that to inode *after* it has become visible via dcache -
> __nfsd_mkdir() has already done d_add() (and pinged f-snotify, at that).
> Looks fishy...

Whoops, thanks.  Considering the following (untested).

--b.

commit 930f7dd94cc2
Author: J. Bruce Fields <bfields@xxxxxxxxxx>
Date:   Thu Aug 15 16:18:26 2019 -0400

    nfsd: initialize i_private before d_add
    
    A process could race in an open and attempt to read one of these files
    before i_private is initialized, and get a spurious error.
    
    Reported-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
    Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index b14f825c62fe..3cf4f6aa48d6 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1171,13 +1171,17 @@ static struct inode *nfsd_get_inode(struct super_block *sb, umode_t mode)
 	return inode;
 }
 
-static int __nfsd_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
+static int __nfsd_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode, struct nfsdfs_client *ncl)
 {
 	struct inode *inode;
 
 	inode = nfsd_get_inode(dir->i_sb, mode);
 	if (!inode)
 		return -ENOMEM;
+	if (ncl) {
+		inode->i_private = ncl;
+		kref_get(&ncl->cl_ref);
+	}
 	d_add(dentry, inode);
 	inc_nlink(dir);
 	fsnotify_mkdir(dir, dentry);
@@ -1194,13 +1198,9 @@ static struct dentry *nfsd_mkdir(struct dentry *parent, struct nfsdfs_client *nc
 	dentry = d_alloc_name(parent, name);
 	if (!dentry)
 		goto out_err;
-	ret = __nfsd_mkdir(d_inode(parent), dentry, S_IFDIR | 0600);
+	ret = __nfsd_mkdir(d_inode(parent), dentry, S_IFDIR | 0600, ncl);
 	if (ret)
 		goto out_err;
-	if (ncl) {
-		d_inode(dentry)->i_private = ncl;
-		kref_get(&ncl->cl_ref);
-	}
 out:
 	inode_unlock(dir);
 	return dentry;