| Message ID | 1565576171-6586-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: fix dentry leak upon mkdir failure. | expand |
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...
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]. Thanks! But it might be clearer to do this in the caller, in the same place the dentry was allocated? --b. commit d6846bfbeeac Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Date: Mon Aug 12 11:16:11 2019 +0900 nfsd: fix dentry leak upon mkdir failure. 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]. [1] https://syzkaller.appspot.com/bug?id=ce41a1f769ea4637ebffedf004a803e8405b4674 Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Reported-by: syzbot <syzbot+2c95195d5d433f6ed6cb@syzkaller.appspotmail.com> Fixes: e8a79fb14f6b76b5 ("nfsd: add nfsd/clients directory") [bfields: clean up in nfsd_mkdir instead of __nfsd_mkdir] Signed-off-by: J. Bruce Fields <bfields@redhat.com> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c index 13c548733860..928a0b2c05dc 100644 --- a/fs/nfsd/nfsctl.c +++ b/fs/nfsd/nfsctl.c @@ -1205,6 +1205,7 @@ static struct dentry *nfsd_mkdir(struct dentry *parent, struct nfsdfs_client *nc inode_unlock(dir); return dentry; out_err: + dput(dentry); dentry = ERR_PTR(ret); goto out; }
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@redhat.com> 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@zeniv.linux.org.uk> Signed-off-by: J. Bruce Fields <bfields@redhat.com> 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;
J. Bruce Fields 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]. > > Thanks! But it might be clearer to do this in the caller, in the same > place the dentry was allocated? I'm fine with that.
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c index 13c5487..e32dc1c 100644 --- a/fs/nfsd/nfsctl.c +++ b/fs/nfsd/nfsctl.c @@ -1176,8 +1176,10 @@ static int __nfsd_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode) struct inode *inode; inode = nfsd_get_inode(dir->i_sb, mode); - if (!inode) + if (!inode) { + dput(dentry); return -ENOMEM; + } d_add(dentry, inode); inc_nlink(dir); fsnotify_mkdir(dir, dentry);
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]. [1] https://syzkaller.appspot.com/bug?id=ce41a1f769ea4637ebffedf004a803e8405b4674 Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Reported-by: syzbot <syzbot+2c95195d5d433f6ed6cb@syzkaller.appspotmail.com> Fixes: e8a79fb14f6b76b5 ("nfsd: add nfsd/clients directory") Cc: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/nfsctl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)