From 154372e67d4053e56591245eb413686621941333 Mon Sep 17 00:00:00 2001 From: Eric Van Hensbergen Date: Wed, 23 Sep 2020 22:11:43 +0800 Subject: fs/9p: fix create-unlink-getattr idiom Fixes several outstanding bug reports of not being able to getattr from an open file after an unlink. This patch cleans up transient fids on an unlink and will search open fids on a client if it detects a dentry that appears to have been unlinked. This search is necessary because fstat does not pass fd information through the VFS API to the filesystem, only the dentry which for 9p has an imperfect match to fids. Inherent in this patch is also a fix for the qid handling on create/open which apparently wasn't being set correctly and was necessary for the search to succeed. A possible optimization over this fix is to include accounting of open fids with the inode in the private data (in a similar fashion to the way we track transient fids with dentries). This would allow a much quicker search for a matching open fid. (changed v9fs_fid_find_global to v9fs_fid_find_inode in comment) Link: http://lkml.kernel.org/r/20200923141146.90046-2-jianyong.wu@arm.com Signed-off-by: Eric Van Hensbergen Signed-off-by: Greg Kurz Signed-off-by: Jianyong Wu Signed-off-by: Dominique Martinet --- fs/9p/fid.c | 30 ++++++++++++++++++++++++++++++ fs/9p/vfs_inode.c | 4 ++++ 2 files changed, 34 insertions(+) (limited to 'fs/9p') diff --git a/fs/9p/fid.c b/fs/9p/fid.c index 3d681a2c2731..3304984c0fad 100644 --- a/fs/9p/fid.c +++ b/fs/9p/fid.c @@ -38,6 +38,33 @@ void v9fs_fid_add(struct dentry *dentry, struct p9_fid *fid) spin_unlock(&dentry->d_lock); } +/** + * v9fs_fid_find_inode - search for a fid off of the client list + * @inode: return a fid pointing to a specific inode + * @uid: return a fid belonging to the specified user + * + */ + +static struct p9_fid *v9fs_fid_find_inode(struct inode *inode, kuid_t uid) +{ + struct p9_client *clnt = v9fs_inode2v9ses(inode)->clnt; + struct p9_fid *fid, *fidptr, *ret = NULL; + unsigned long flags; + + p9_debug(P9_DEBUG_VFS, " inode: %p\n", inode); + + spin_lock_irqsave(&clnt->lock, flags); + list_for_each_entry_safe(fid, fidptr, &clnt->fidlist, flist) { + if (uid_eq(fid->uid, uid) && + (inode->i_ino == v9fs_qid2ino(&fid->qid))) { + ret = fid; + break; + } + } + spin_unlock_irqrestore(&clnt->lock, flags); + return ret; +} + /** * v9fs_fid_find - retrieve a fid that belongs to the specified uid * @dentry: dentry to look for fid in @@ -65,6 +92,9 @@ static struct p9_fid *v9fs_fid_find(struct dentry *dentry, kuid_t uid, int any) } } spin_unlock(&dentry->d_lock); + } else { + if (dentry->d_inode) + ret = v9fs_fid_find_inode(dentry->d_inode, uid); } return ret; diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c index ae0c38ad1fcb..31c2fddabb82 100644 --- a/fs/9p/vfs_inode.c +++ b/fs/9p/vfs_inode.c @@ -570,6 +570,10 @@ static int v9fs_remove(struct inode *dir, struct dentry *dentry, int flags) v9fs_invalidate_inode_attr(inode); v9fs_invalidate_inode_attr(dir); + + /* invalidate all fids associated with dentry */ + /* NOTE: This will not include open fids */ + dentry->d_op->d_release(dentry); } return retval; } -- cgit v1.2.3 From 987a64850996db22bbcf2c1d0a051446a343fa2c Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Wed, 23 Sep 2020 22:11:44 +0800 Subject: fs/9p: track open fids This patch adds accounting of open fids in a list hanging off the i_private field of the corresponding inode. This allows faster lookups compared to searching the full 9p client list. The lookup code is modified accordingly. Link: http://lkml.kernel.org/r/20200923141146.90046-3-jianyong.wu@arm.com Signed-off-by: Greg Kurz Signed-off-by: Jianyong Wu Signed-off-by: Dominique Martinet --- fs/9p/fid.c | 32 +++++++++++++++++++++++--------- fs/9p/fid.h | 1 + fs/9p/vfs_dir.c | 3 +++ fs/9p/vfs_file.c | 1 + fs/9p/vfs_inode.c | 6 +++++- fs/9p/vfs_inode_dotl.c | 1 + include/net/9p/client.h | 1 + 7 files changed, 35 insertions(+), 10 deletions(-) (limited to 'fs/9p') diff --git a/fs/9p/fid.c b/fs/9p/fid.c index 3304984c0fad..d11dd430590d 100644 --- a/fs/9p/fid.c +++ b/fs/9p/fid.c @@ -39,7 +39,7 @@ void v9fs_fid_add(struct dentry *dentry, struct p9_fid *fid) } /** - * v9fs_fid_find_inode - search for a fid off of the client list + * v9fs_fid_find_inode - search for an open fid off of the inode list * @inode: return a fid pointing to a specific inode * @uid: return a fid belonging to the specified user * @@ -47,24 +47,38 @@ void v9fs_fid_add(struct dentry *dentry, struct p9_fid *fid) static struct p9_fid *v9fs_fid_find_inode(struct inode *inode, kuid_t uid) { - struct p9_client *clnt = v9fs_inode2v9ses(inode)->clnt; - struct p9_fid *fid, *fidptr, *ret = NULL; - unsigned long flags; + struct hlist_head *h; + struct p9_fid *fid, *ret = NULL; p9_debug(P9_DEBUG_VFS, " inode: %p\n", inode); - spin_lock_irqsave(&clnt->lock, flags); - list_for_each_entry_safe(fid, fidptr, &clnt->fidlist, flist) { - if (uid_eq(fid->uid, uid) && - (inode->i_ino == v9fs_qid2ino(&fid->qid))) { + spin_lock(&inode->i_lock); + h = (struct hlist_head *)&inode->i_private; + hlist_for_each_entry(fid, h, ilist) { + if (uid_eq(fid->uid, uid)) { ret = fid; break; } } - spin_unlock_irqrestore(&clnt->lock, flags); + spin_unlock(&inode->i_lock); return ret; } +/** + * v9fs_open_fid_add - add an open fid to an inode + * @dentry: inode that the fid is being added to + * @fid: fid to add + * + */ + +void v9fs_open_fid_add(struct inode *inode, struct p9_fid *fid) +{ + spin_lock(&inode->i_lock); + hlist_add_head(&fid->ilist, (struct hlist_head *)&inode->i_private); + spin_unlock(&inode->i_lock); +} + + /** * v9fs_fid_find - retrieve a fid that belongs to the specified uid * @dentry: dentry to look for fid in diff --git a/fs/9p/fid.h b/fs/9p/fid.h index 928b1093f511..dfa11df02818 100644 --- a/fs/9p/fid.h +++ b/fs/9p/fid.h @@ -15,6 +15,7 @@ static inline struct p9_fid *v9fs_parent_fid(struct dentry *dentry) } void v9fs_fid_add(struct dentry *dentry, struct p9_fid *fid); struct p9_fid *v9fs_writeback_fid(struct dentry *dentry); +void v9fs_open_fid_add(struct inode *inode, struct p9_fid *fid); static inline struct p9_fid *clone_fid(struct p9_fid *fid) { return IS_ERR(fid) ? fid : p9_client_walk(fid, 0, NULL, 1); diff --git a/fs/9p/vfs_dir.c b/fs/9p/vfs_dir.c index 674d22bf4f6f..d82d8a346f86 100644 --- a/fs/9p/vfs_dir.c +++ b/fs/9p/vfs_dir.c @@ -210,6 +210,9 @@ int v9fs_dir_release(struct inode *inode, struct file *filp) fid = filp->private_data; p9_debug(P9_DEBUG_VFS, "inode: %p filp: %p fid: %d\n", inode, filp, fid ? fid->fid : -1); + spin_lock(&inode->i_lock); + hlist_del(&fid->ilist); + spin_unlock(&inode->i_lock); if (fid) p9_client_clunk(fid); return 0; diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c index b177fd3b1eb3..b0ef225cecd0 100644 --- a/fs/9p/vfs_file.c +++ b/fs/9p/vfs_file.c @@ -96,6 +96,7 @@ int v9fs_file_open(struct inode *inode, struct file *file) mutex_unlock(&v9inode->v_mutex); if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE) v9fs_cache_inode_set_cookie(inode, file); + v9fs_open_fid_add(inode, fid); return 0; out_error: p9_client_clunk(file->private_data); diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c index 31c2fddabb82..6b243ffcbcf0 100644 --- a/fs/9p/vfs_inode.c +++ b/fs/9p/vfs_inode.c @@ -256,6 +256,7 @@ int v9fs_init_inode(struct v9fs_session_info *v9ses, inode->i_rdev = rdev; inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode); inode->i_mapping->a_ops = &v9fs_addr_operations; + inode->i_private = NULL; switch (mode & S_IFMT) { case S_IFIFO: @@ -796,6 +797,7 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry, struct v9fs_session_info *v9ses; struct p9_fid *fid, *inode_fid; struct dentry *res = NULL; + struct inode *inode; if (d_in_lookup(dentry)) { res = v9fs_vfs_lookup(dir, dentry, 0); @@ -824,7 +826,8 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry, } v9fs_invalidate_inode_attr(dir); - v9inode = V9FS_I(d_inode(dentry)); + inode = d_inode(dentry); + v9inode = V9FS_I(inode); mutex_lock(&v9inode->v_mutex); if ((v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE) && !v9inode->writeback_fid && @@ -852,6 +855,7 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry, file->private_data = fid; if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE) v9fs_cache_inode_set_cookie(d_inode(dentry), file); + v9fs_open_fid_add(inode, fid); file->f_mode |= FMODE_CREATED; out: diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c index 0028eccb665a..08f2e089fb0e 100644 --- a/fs/9p/vfs_inode_dotl.c +++ b/fs/9p/vfs_inode_dotl.c @@ -342,6 +342,7 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry, file->private_data = ofid; if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE) v9fs_cache_inode_set_cookie(inode, file); + v9fs_open_fid_add(inode, ofid); file->f_mode |= FMODE_CREATED; out: v9fs_put_acl(dacl, pacl); diff --git a/include/net/9p/client.h b/include/net/9p/client.h index dd5b5bd781a4..ce7882da8e86 100644 --- a/include/net/9p/client.h +++ b/include/net/9p/client.h @@ -152,6 +152,7 @@ struct p9_fid { void *rdir; struct hlist_node dlist; /* list of all fids attached to a dentry */ + struct hlist_node ilist; }; /** -- cgit v1.2.3 From 478ba09edc1f2f2ee27180a06150cb2d1a686f9c Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Wed, 23 Sep 2020 22:11:45 +0800 Subject: fs/9p: search open fids first A previous patch fixed the "create-unlink-getattr" idiom: if getattr is called on an unlinked file, we try to find an open fid attached to the corresponding inode. We have a similar issue with file permissions and setattr: open("./test.txt", O_RDWR|O_CREAT, 0666) = 4 chmod("./test.txt", 0) = 0 truncate("./test.txt", 0) = -1 EACCES (Permission denied) ftruncate(4, 0) = -1 EACCES (Permission denied) The failure is expected with truncate() but not with ftruncate(). This happens because the lookup code does find a matching fid in the dentry list. Unfortunately, this is not an open fid and the server will be forced to rely on the path name, rather than on an open file descriptor. This is the case in QEMU: the setattr operation will use truncate() and fail because of bad write permissions. This patch changes the logic in the lookup code, so that we consider open fids first. It gives a chance to the server to match this open fid to an open file descriptor and use ftruncate() instead of truncate(). This does not change the current behaviour for truncate() and other path name based syscalls, since file permissions are checked earlier in the VFS layer. With this patch, we get: open("./test.txt", O_RDWR|O_CREAT, 0666) = 4 chmod("./test.txt", 0) = 0 truncate("./test.txt", 0) = -1 EACCES (Permission denied) ftruncate(4, 0) = 0 Link: http://lkml.kernel.org/r/20200923141146.90046-4-jianyong.wu@arm.com Signed-off-by: Greg Kurz Signed-off-by: Jianyong Wu Signed-off-by: Dominique Martinet --- fs/9p/fid.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'fs/9p') diff --git a/fs/9p/fid.c b/fs/9p/fid.c index d11dd430590d..0b23b0fe6c51 100644 --- a/fs/9p/fid.c +++ b/fs/9p/fid.c @@ -95,8 +95,12 @@ static struct p9_fid *v9fs_fid_find(struct dentry *dentry, kuid_t uid, int any) dentry, dentry, from_kuid(&init_user_ns, uid), any); ret = NULL; + + if (d_inode(dentry)) + ret = v9fs_fid_find_inode(d_inode(dentry), uid); + /* we'll recheck under lock if there's anything to look in */ - if (dentry->d_fsdata) { + if (!ret && dentry->d_fsdata) { struct hlist_head *h = (struct hlist_head *)&dentry->d_fsdata; spin_lock(&dentry->d_lock); hlist_for_each_entry(fid, h, dlist) { @@ -106,9 +110,6 @@ static struct p9_fid *v9fs_fid_find(struct dentry *dentry, kuid_t uid, int any) } } spin_unlock(&dentry->d_lock); - } else { - if (dentry->d_inode) - ret = v9fs_fid_find_inode(dentry->d_inode, uid); } return ret; -- cgit v1.2.3 From 6636b6dcc3db2258cd0585b8078c1c225c4b6dde Mon Sep 17 00:00:00 2001 From: Jianyong Wu Date: Wed, 23 Sep 2020 22:11:46 +0800 Subject: 9p: add refcount to p9_fid struct Fix race issue in fid contention. Eric's and Greg's patch offer a mechanism to fix open-unlink-f*syscall bug in 9p. But there is race issue in fid parallel accesses. As Greg's patch stores all of fids from opened files into according inode, so all the lookup fid ops can retrieve fid from inode preferentially. But there is no mechanism to handle the fid contention issue. For example, there are two threads get the same fid in the same time and one of them clunk the fid before the other thread ready to discard the fid. In this scenario, it will lead to some fatal problems, even kernel core dump. I introduce a mechanism to fix this race issue. A counter field introduced into p9_fid struct to store the reference counter to the fid. When a fid is allocated from the inode or dentry, the counter will increase, and will decrease at the end of its occupation. It is guaranteed that the fid won't be clunked before the reference counter go down to 0, then we can avoid the clunked fid to be used. tests: race issue test from the old test case: for file in {01..50}; do touch f.${file}; done seq 1 1000 | xargs -n 1 -P 50 -I{} cat f.* > /dev/null open-unlink-f*syscall test: I have tested for f*syscall include: ftruncate fstat fchown fchmod faccessat. Link: http://lkml.kernel.org/r/20200923141146.90046-5-jianyong.wu@arm.com Fixes: 478ba09edc1f ("fs/9p: search open fids first") Signed-off-by: Jianyong Wu Signed-off-by: Dominique Martinet --- fs/9p/fid.c | 22 ++++++++++++++++++---- fs/9p/fid.h | 10 +++++++++- fs/9p/vfs_dentry.c | 2 ++ fs/9p/vfs_dir.c | 9 +++++---- fs/9p/vfs_inode.c | 37 +++++++++++++++++++++++++++++-------- fs/9p/vfs_inode_dotl.c | 34 ++++++++++++++++++++++++++++------ fs/9p/vfs_super.c | 1 + fs/9p/xattr.c | 16 +++++++++++++--- include/net/9p/client.h | 6 ++++++ net/9p/client.c | 9 +++++---- 10 files changed, 116 insertions(+), 30 deletions(-) (limited to 'fs/9p') diff --git a/fs/9p/fid.c b/fs/9p/fid.c index 0b23b0fe6c51..89643dabcdae 100644 --- a/fs/9p/fid.c +++ b/fs/9p/fid.c @@ -28,6 +28,7 @@ static inline void __add_fid(struct dentry *dentry, struct p9_fid *fid) { + atomic_set(&fid->count, 1); hlist_add_head(&fid->dlist, (struct hlist_head *)&dentry->d_fsdata); } @@ -60,6 +61,8 @@ static struct p9_fid *v9fs_fid_find_inode(struct inode *inode, kuid_t uid) break; } } + if (ret && !IS_ERR(ret)) + atomic_inc(&ret->count); spin_unlock(&inode->i_lock); return ret; } @@ -74,6 +77,7 @@ static struct p9_fid *v9fs_fid_find_inode(struct inode *inode, kuid_t uid) void v9fs_open_fid_add(struct inode *inode, struct p9_fid *fid) { spin_lock(&inode->i_lock); + atomic_set(&fid->count, 1); hlist_add_head(&fid->ilist, (struct hlist_head *)&inode->i_private); spin_unlock(&inode->i_lock); } @@ -106,6 +110,7 @@ static struct p9_fid *v9fs_fid_find(struct dentry *dentry, kuid_t uid, int any) hlist_for_each_entry(fid, h, dlist) { if (any || uid_eq(fid->uid, uid)) { ret = fid; + atomic_inc(&ret->count); break; } } @@ -167,7 +172,10 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, fid = v9fs_fid_find(ds, uid, any); if (fid) { /* Found the parent fid do a lookup with that */ - fid = p9_client_walk(fid, 1, &dentry->d_name.name, 1); + struct p9_fid *ofid = fid; + + fid = p9_client_walk(ofid, 1, &dentry->d_name.name, 1); + p9_client_clunk(ofid); goto fid_out; } up_read(&v9ses->rename_sem); @@ -192,8 +200,10 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, v9fs_fid_add(dentry->d_sb->s_root, fid); } /* If we are root ourself just return that */ - if (dentry->d_sb->s_root == dentry) + if (dentry->d_sb->s_root == dentry) { + atomic_inc(&fid->count); return fid; + } /* * Do a multipath walk with attached root. * When walking parent we need to make sure we @@ -240,6 +250,7 @@ fid_out: fid = ERR_PTR(-ENOENT); } else { __add_fid(dentry, fid); + atomic_inc(&fid->count); spin_unlock(&dentry->d_lock); } } @@ -290,11 +301,14 @@ struct p9_fid *v9fs_fid_lookup(struct dentry *dentry) struct p9_fid *v9fs_writeback_fid(struct dentry *dentry) { int err; - struct p9_fid *fid; + struct p9_fid *fid, *ofid; - fid = clone_fid(v9fs_fid_lookup_with_uid(dentry, GLOBAL_ROOT_UID, 0)); + ofid = v9fs_fid_lookup_with_uid(dentry, GLOBAL_ROOT_UID, 0); + if (ofid && !IS_ERR(ofid)) + fid = clone_fid(ofid); if (IS_ERR(fid)) goto error_out; + p9_client_clunk(ofid); /* * writeback fid will only be used to write back the * dirty pages. We always request for the open fid in read-write diff --git a/fs/9p/fid.h b/fs/9p/fid.h index dfa11df02818..1fed96546728 100644 --- a/fs/9p/fid.h +++ b/fs/9p/fid.h @@ -22,6 +22,14 @@ static inline struct p9_fid *clone_fid(struct p9_fid *fid) } static inline struct p9_fid *v9fs_fid_clone(struct dentry *dentry) { - return clone_fid(v9fs_fid_lookup(dentry)); + struct p9_fid *fid, *nfid; + + fid = v9fs_fid_lookup(dentry); + if (!fid || IS_ERR(fid)) + return fid; + + nfid = p9_client_walk(fid, 0, NULL, 1); + p9_client_clunk(fid); + return nfid; } #endif diff --git a/fs/9p/vfs_dentry.c b/fs/9p/vfs_dentry.c index 7d6f69aefd45..4b4292123b3d 100644 --- a/fs/9p/vfs_dentry.c +++ b/fs/9p/vfs_dentry.c @@ -85,6 +85,8 @@ static int v9fs_lookup_revalidate(struct dentry *dentry, unsigned int flags) retval = v9fs_refresh_inode_dotl(fid, inode); else retval = v9fs_refresh_inode(fid, inode); + p9_client_clunk(fid); + if (retval == -ENOENT) return 0; if (retval < 0) diff --git a/fs/9p/vfs_dir.c b/fs/9p/vfs_dir.c index d82d8a346f86..b6a5a0be444d 100644 --- a/fs/9p/vfs_dir.c +++ b/fs/9p/vfs_dir.c @@ -210,11 +210,12 @@ int v9fs_dir_release(struct inode *inode, struct file *filp) fid = filp->private_data; p9_debug(P9_DEBUG_VFS, "inode: %p filp: %p fid: %d\n", inode, filp, fid ? fid->fid : -1); - spin_lock(&inode->i_lock); - hlist_del(&fid->ilist); - spin_unlock(&inode->i_lock); - if (fid) + if (fid) { + spin_lock(&inode->i_lock); + hlist_del(&fid->ilist); + spin_unlock(&inode->i_lock); p9_client_clunk(fid); + } return 0; } diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c index 6b243ffcbcf0..4a937fac1acb 100644 --- a/fs/9p/vfs_inode.c +++ b/fs/9p/vfs_inode.c @@ -551,6 +551,7 @@ static int v9fs_remove(struct inode *dir, struct dentry *dentry, int flags) if (v9fs_proto_dotl(v9ses)) retval = p9_client_unlinkat(dfid, dentry->d_name.name, v9fs_at_to_dotl_flags(flags)); + p9_client_clunk(dfid); if (retval == -EOPNOTSUPP) { /* Try the one based on path */ v9fid = v9fs_fid_clone(dentry); @@ -595,14 +596,12 @@ v9fs_create(struct v9fs_session_info *v9ses, struct inode *dir, { int err; const unsigned char *name; - struct p9_fid *dfid, *ofid, *fid; + struct p9_fid *dfid, *ofid = NULL, *fid = NULL; struct inode *inode; p9_debug(P9_DEBUG_VFS, "name %pd\n", dentry); err = 0; - ofid = NULL; - fid = NULL; name = dentry->d_name.name; dfid = v9fs_parent_fid(dentry); if (IS_ERR(dfid)) { @@ -616,12 +615,14 @@ v9fs_create(struct v9fs_session_info *v9ses, struct inode *dir, if (IS_ERR(ofid)) { err = PTR_ERR(ofid); p9_debug(P9_DEBUG_VFS, "p9_client_walk failed %d\n", err); + p9_client_clunk(dfid); return ERR_PTR(err); } err = p9_client_fcreate(ofid, name, perm, mode, extension); if (err < 0) { p9_debug(P9_DEBUG_VFS, "p9_client_fcreate failed %d\n", err); + p9_client_clunk(dfid); goto error; } @@ -633,6 +634,7 @@ v9fs_create(struct v9fs_session_info *v9ses, struct inode *dir, p9_debug(P9_DEBUG_VFS, "p9_client_walk failed %d\n", err); fid = NULL; + p9_client_clunk(dfid); goto error; } /* @@ -643,11 +645,13 @@ v9fs_create(struct v9fs_session_info *v9ses, struct inode *dir, err = PTR_ERR(inode); p9_debug(P9_DEBUG_VFS, "inode creation failed %d\n", err); + p9_client_clunk(dfid); goto error; } v9fs_fid_add(dentry, fid); d_instantiate(dentry, inode); } + p9_client_clunk(dfid); return ofid; error: if (ofid) @@ -760,6 +764,7 @@ struct dentry *v9fs_vfs_lookup(struct inode *dir, struct dentry *dentry, */ name = dentry->d_name.name; fid = p9_client_walk(dfid, 1, &name, 1); + p9_client_clunk(dfid); if (fid == ERR_PTR(-ENOENT)) inode = NULL; else if (IS_ERR(fid)) @@ -910,7 +915,7 @@ v9fs_vfs_rename(struct inode *old_dir, struct dentry *old_dentry, struct inode *old_inode; struct inode *new_inode; struct v9fs_session_info *v9ses; - struct p9_fid *oldfid; + struct p9_fid *oldfid, *dfid; struct p9_fid *olddirfid; struct p9_fid *newdirfid; struct p9_wstat wstat; @@ -927,13 +932,20 @@ v9fs_vfs_rename(struct inode *old_dir, struct dentry *old_dentry, if (IS_ERR(oldfid)) return PTR_ERR(oldfid); - olddirfid = clone_fid(v9fs_parent_fid(old_dentry)); + dfid = v9fs_parent_fid(old_dentry); + olddirfid = clone_fid(dfid); + if (dfid && !IS_ERR(dfid)) + p9_client_clunk(dfid); + if (IS_ERR(olddirfid)) { retval = PTR_ERR(olddirfid); goto done; } - newdirfid = clone_fid(v9fs_parent_fid(new_dentry)); + dfid = v9fs_parent_fid(new_dentry); + newdirfid = clone_fid(dfid); + p9_client_clunk(dfid); + if (IS_ERR(newdirfid)) { retval = PTR_ERR(newdirfid); goto clunk_olddir; @@ -990,6 +1002,7 @@ clunk_olddir: p9_client_clunk(olddirfid); done: + p9_client_clunk(oldfid); return retval; } @@ -1022,6 +1035,7 @@ v9fs_vfs_getattr(const struct path *path, struct kstat *stat, return PTR_ERR(fid); st = p9_client_stat(fid); + p9_client_clunk(fid); if (IS_ERR(st)) return PTR_ERR(st); @@ -1042,7 +1056,7 @@ v9fs_vfs_getattr(const struct path *path, struct kstat *stat, static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr) { - int retval; + int retval, use_dentry = 0; struct v9fs_session_info *v9ses; struct p9_fid *fid = NULL; struct p9_wstat wstat; @@ -1058,8 +1072,10 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr) fid = iattr->ia_file->private_data; WARN_ON(!fid); } - if (!fid) + if (!fid) { fid = v9fs_fid_lookup(dentry); + use_dentry = 1; + } if(IS_ERR(fid)) return PTR_ERR(fid); @@ -1089,6 +1105,10 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr) filemap_write_and_wait(d_inode(dentry)->i_mapping); retval = p9_client_wstat(fid, &wstat); + + if (use_dentry) + p9_client_clunk(fid); + if (retval < 0) return retval; @@ -1213,6 +1233,7 @@ static const char *v9fs_vfs_get_link(struct dentry *dentry, return ERR_PTR(-EBADF); st = p9_client_stat(fid); + p9_client_clunk(fid); if (IS_ERR(st)) return ERR_CAST(st); diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c index 08f2e089fb0e..823c2eb5f1bf 100644 --- a/fs/9p/vfs_inode_dotl.c +++ b/fs/9p/vfs_inode_dotl.c @@ -296,6 +296,7 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry, /* instantiate inode and assign the unopened fid to the dentry */ fid = p9_client_walk(dfid, 1, &name, 1); + p9_client_clunk(dfid); if (IS_ERR(fid)) { err = PTR_ERR(fid); p9_debug(P9_DEBUG_VFS, "p9_client_walk failed %d\n", err); @@ -408,7 +409,6 @@ static int v9fs_vfs_mkdir_dotl(struct inode *dir, err = p9_client_mkdir_dotl(dfid, name, mode, gid, &qid); if (err < 0) goto error; - fid = p9_client_walk(dfid, 1, &name, 1); if (IS_ERR(fid)) { err = PTR_ERR(fid); @@ -452,6 +452,7 @@ error: if (fid) p9_client_clunk(fid); v9fs_put_acl(dacl, pacl); + p9_client_clunk(dfid); return err; } @@ -479,6 +480,7 @@ v9fs_vfs_getattr_dotl(const struct path *path, struct kstat *stat, */ st = p9_client_getattr_dotl(fid, P9_STATS_ALL); + p9_client_clunk(fid); if (IS_ERR(st)) return PTR_ERR(st); @@ -540,7 +542,7 @@ static int v9fs_mapped_iattr_valid(int iattr_valid) int v9fs_vfs_setattr_dotl(struct dentry *dentry, struct iattr *iattr) { - int retval; + int retval, use_dentry = 0; struct p9_fid *fid = NULL; struct p9_iattr_dotl p9attr; struct inode *inode = d_inode(dentry); @@ -565,8 +567,10 @@ int v9fs_vfs_setattr_dotl(struct dentry *dentry, struct iattr *iattr) fid = iattr->ia_file->private_data; WARN_ON(!fid); } - if (!fid) + if (!fid) { fid = v9fs_fid_lookup(dentry); + use_dentry = 1; + } if (IS_ERR(fid)) return PTR_ERR(fid); @@ -575,8 +579,11 @@ int v9fs_vfs_setattr_dotl(struct dentry *dentry, struct iattr *iattr) filemap_write_and_wait(inode->i_mapping); retval = p9_client_setattr(fid, &p9attr); - if (retval < 0) + if (retval < 0) { + if (use_dentry) + p9_client_clunk(fid); return retval; + } if ((iattr->ia_valid & ATTR_SIZE) && iattr->ia_size != i_size_read(inode)) @@ -588,9 +595,15 @@ int v9fs_vfs_setattr_dotl(struct dentry *dentry, struct iattr *iattr) if (iattr->ia_valid & ATTR_MODE) { /* We also want to update ACL when we update mode bits */ retval = v9fs_acl_chmod(inode, fid); - if (retval < 0) + if (retval < 0) { + if (use_dentry) + p9_client_clunk(fid); return retval; + } } + if (use_dentry) + p9_client_clunk(fid); + return 0; } @@ -742,6 +755,7 @@ error: if (fid) p9_client_clunk(fid); + p9_client_clunk(dfid); return err; } @@ -770,11 +784,15 @@ v9fs_vfs_link_dotl(struct dentry *old_dentry, struct inode *dir, return PTR_ERR(dfid); oldfid = v9fs_fid_lookup(old_dentry); - if (IS_ERR(oldfid)) + if (IS_ERR(oldfid)) { + p9_client_clunk(dfid); return PTR_ERR(oldfid); + } err = p9_client_link(dfid, oldfid, dentry->d_name.name); + p9_client_clunk(dfid); + p9_client_clunk(oldfid); if (err < 0) { p9_debug(P9_DEBUG_VFS, "p9_client_link failed %d\n", err); return err; @@ -789,6 +807,7 @@ v9fs_vfs_link_dotl(struct dentry *old_dentry, struct inode *dir, return PTR_ERR(fid); v9fs_refresh_inode_dotl(fid, d_inode(old_dentry)); + p9_client_clunk(fid); } ihold(d_inode(old_dentry)); d_instantiate(dentry, d_inode(old_dentry)); @@ -887,6 +906,8 @@ error: if (fid) p9_client_clunk(fid); v9fs_put_acl(dacl, pacl); + p9_client_clunk(dfid); + return err; } @@ -915,6 +936,7 @@ v9fs_vfs_get_link_dotl(struct dentry *dentry, if (IS_ERR(fid)) return ERR_CAST(fid); retval = p9_client_readlink(fid, &target); + p9_client_clunk(fid); if (retval) return ERR_PTR(retval); set_delayed_call(done, kfree_link, target); diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c index 9a21269b7234..5fce6e30bc5a 100644 --- a/fs/9p/vfs_super.c +++ b/fs/9p/vfs_super.c @@ -268,6 +268,7 @@ static int v9fs_statfs(struct dentry *dentry, struct kstatfs *buf) } res = simple_statfs(dentry, buf); done: + p9_client_clunk(fid); return res; } diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c index ac8ff8ca4c11..87217dd0433e 100644 --- a/fs/9p/xattr.c +++ b/fs/9p/xattr.c @@ -71,14 +71,17 @@ ssize_t v9fs_xattr_get(struct dentry *dentry, const char *name, void *buffer, size_t buffer_size) { struct p9_fid *fid; + int ret; p9_debug(P9_DEBUG_VFS, "name = %s value_len = %zu\n", name, buffer_size); fid = v9fs_fid_lookup(dentry); if (IS_ERR(fid)) return PTR_ERR(fid); + ret = v9fs_fid_xattr_get(fid, name, buffer, buffer_size); + p9_client_clunk(fid); - return v9fs_fid_xattr_get(fid, name, buffer, buffer_size); + return ret; } /* @@ -96,8 +99,15 @@ ssize_t v9fs_xattr_get(struct dentry *dentry, const char *name, int v9fs_xattr_set(struct dentry *dentry, const char *name, const void *value, size_t value_len, int flags) { - struct p9_fid *fid = v9fs_fid_lookup(dentry); - return v9fs_fid_xattr_set(fid, name, value, value_len, flags); + int ret; + struct p9_fid *fid; + + fid = v9fs_fid_lookup(dentry); + if (IS_ERR(fid)) + return PTR_ERR(fid); + ret = v9fs_fid_xattr_set(fid, name, value, value_len, flags); + p9_client_clunk(fid); + return ret; } int v9fs_fid_xattr_set(struct p9_fid *fid, const char *name, diff --git a/include/net/9p/client.h b/include/net/9p/client.h index ce7882da8e86..58ed9bd306bd 100644 --- a/include/net/9p/client.h +++ b/include/net/9p/client.h @@ -140,10 +140,16 @@ struct p9_client { * * TODO: This needs lots of explanation. */ +enum fid_source { + FID_FROM_OTHER, + FID_FROM_INODE, + FID_FROM_DENTRY, +}; struct p9_fid { struct p9_client *clnt; u32 fid; + atomic_t count; int mode; struct p9_qid qid; u32 iounit; diff --git a/net/9p/client.c b/net/9p/client.c index 1a3f72bf45fc..a6c8a915e0d8 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -901,6 +901,7 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt) fid->clnt = clnt; fid->rdir = NULL; fid->fid = 0; + atomic_set(&fid->count, 1); idr_preload(GFP_KERNEL); spin_lock_irq(&clnt->lock); @@ -908,7 +909,6 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt) GFP_NOWAIT); spin_unlock_irq(&clnt->lock); idr_preload_end(); - if (!ret) return fid; @@ -1187,7 +1187,6 @@ struct p9_fid *p9_client_walk(struct p9_fid *oldfid, uint16_t nwname, p9_debug(P9_DEBUG_9P, ">>> TWALK fids %d,%d nwname %ud wname[0] %s\n", oldfid->fid, fid->fid, nwname, wnames ? wnames[0] : NULL); - req = p9_client_rpc(clnt, P9_TWALK, "ddT", oldfid->fid, fid->fid, nwname, wnames); if (IS_ERR(req)) { @@ -1461,12 +1460,14 @@ int p9_client_clunk(struct p9_fid *fid) struct p9_req_t *req; int retries = 0; - if (!fid) { - pr_warn("%s (%d): Trying to clunk with NULL fid\n", + if (!fid || IS_ERR(fid)) { + pr_warn("%s (%d): Trying to clunk with invalid fid\n", __func__, task_pid_nr(current)); dump_stack(); return 0; } + if (!atomic_dec_and_test(&fid->count)) + return 0; again: p9_debug(P9_DEBUG_9P, ">>> TCLUNK fid %d (try %d)\n", fid->fid, -- cgit v1.2.3 From ff5e72ebef41068789c93b0666cebde80cc8bd8c Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Tue, 3 Nov 2020 09:35:57 +0100 Subject: 9p: apply review requests for fid refcounting Fix style issues in parent commit ("apply review requests for fid refcounting"), no functional change. Link: http://lkml.kernel.org/r/1605802012-31133-2-git-send-email-asmadeus@codewreck.org Fixes: 6636b6dcc3db ("9p: add refcount to p9_fid struct") Signed-off-by: Dominique Martinet --- fs/9p/fid.c | 10 ++++------ fs/9p/fid.h | 2 +- include/net/9p/client.h | 2 +- net/9p/client.c | 4 ++-- 4 files changed, 8 insertions(+), 10 deletions(-) (limited to 'fs/9p') diff --git a/fs/9p/fid.c b/fs/9p/fid.c index 89643dabcdae..50118ec72a92 100644 --- a/fs/9p/fid.c +++ b/fs/9p/fid.c @@ -28,7 +28,6 @@ static inline void __add_fid(struct dentry *dentry, struct p9_fid *fid) { - atomic_set(&fid->count, 1); hlist_add_head(&fid->dlist, (struct hlist_head *)&dentry->d_fsdata); } @@ -62,7 +61,7 @@ static struct p9_fid *v9fs_fid_find_inode(struct inode *inode, kuid_t uid) } } if (ret && !IS_ERR(ret)) - atomic_inc(&ret->count); + refcount_inc(&ret->count); spin_unlock(&inode->i_lock); return ret; } @@ -77,7 +76,6 @@ static struct p9_fid *v9fs_fid_find_inode(struct inode *inode, kuid_t uid) void v9fs_open_fid_add(struct inode *inode, struct p9_fid *fid) { spin_lock(&inode->i_lock); - atomic_set(&fid->count, 1); hlist_add_head(&fid->ilist, (struct hlist_head *)&inode->i_private); spin_unlock(&inode->i_lock); } @@ -110,7 +108,7 @@ static struct p9_fid *v9fs_fid_find(struct dentry *dentry, kuid_t uid, int any) hlist_for_each_entry(fid, h, dlist) { if (any || uid_eq(fid->uid, uid)) { ret = fid; - atomic_inc(&ret->count); + refcount_inc(&ret->count); break; } } @@ -201,7 +199,7 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, } /* If we are root ourself just return that */ if (dentry->d_sb->s_root == dentry) { - atomic_inc(&fid->count); + refcount_inc(&fid->count); return fid; } /* @@ -250,7 +248,7 @@ fid_out: fid = ERR_PTR(-ENOENT); } else { __add_fid(dentry, fid); - atomic_inc(&fid->count); + refcount_inc(&fid->count); spin_unlock(&dentry->d_lock); } } diff --git a/fs/9p/fid.h b/fs/9p/fid.h index 1fed96546728..f7f33509e169 100644 --- a/fs/9p/fid.h +++ b/fs/9p/fid.h @@ -28,7 +28,7 @@ static inline struct p9_fid *v9fs_fid_clone(struct dentry *dentry) if (!fid || IS_ERR(fid)) return fid; - nfid = p9_client_walk(fid, 0, NULL, 1); + nfid = clone_fid(fid); p9_client_clunk(fid); return nfid; } diff --git a/include/net/9p/client.h b/include/net/9p/client.h index 58ed9bd306bd..e1c308d8d288 100644 --- a/include/net/9p/client.h +++ b/include/net/9p/client.h @@ -149,7 +149,7 @@ enum fid_source { struct p9_fid { struct p9_client *clnt; u32 fid; - atomic_t count; + refcount_t count; int mode; struct p9_qid qid; u32 iounit; diff --git a/net/9p/client.c b/net/9p/client.c index a6c8a915e0d8..ba4910138c5b 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -901,7 +901,7 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt) fid->clnt = clnt; fid->rdir = NULL; fid->fid = 0; - atomic_set(&fid->count, 1); + refcount_set(&fid->count, 1); idr_preload(GFP_KERNEL); spin_lock_irq(&clnt->lock); @@ -1466,7 +1466,7 @@ int p9_client_clunk(struct p9_fid *fid) dump_stack(); return 0; } - if (!atomic_dec_and_test(&fid->count)) + if (!refcount_dec_and_test(&fid->count)) return 0; again: -- cgit v1.2.3 From 5bfe97d7382b5c1ec351c59a878e742c9fd73d38 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Thu, 19 Nov 2020 15:11:04 +0100 Subject: 9p: Fix writeback fid incorrectly being attached to dentry v9fs_dir_release needs fid->ilist to have been initialized for filp's fid, not the inode's writeback fid's. With refcounting this can be improved on later but this appears to fix null deref issues. Link: http://lkml.kernel.org/r/1605802012-31133-3-git-send-email-asmadeus@codewreck.org Fixes: 6636b6dcc3db ("fs/9p: track open fids") Signed-off-by: Dominique Martinet --- fs/9p/vfs_file.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'fs/9p') diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c index b0ef225cecd0..c5e49c88688d 100644 --- a/fs/9p/vfs_file.c +++ b/fs/9p/vfs_file.c @@ -46,7 +46,7 @@ int v9fs_file_open(struct inode *inode, struct file *file) int err; struct v9fs_inode *v9inode; struct v9fs_session_info *v9ses; - struct p9_fid *fid; + struct p9_fid *fid, *writeback_fid; int omode; p9_debug(P9_DEBUG_VFS, "inode: %p file: %p\n", inode, file); @@ -85,13 +85,13 @@ int v9fs_file_open(struct inode *inode, struct file *file) * because we want write after unlink usecase * to work. */ - fid = v9fs_writeback_fid(file_dentry(file)); + writeback_fid = v9fs_writeback_fid(file_dentry(file)); if (IS_ERR(fid)) { err = PTR_ERR(fid); mutex_unlock(&v9inode->v_mutex); goto out_error; } - v9inode->writeback_fid = (void *) fid; + v9inode->writeback_fid = (void *) writeback_fid; } mutex_unlock(&v9inode->v_mutex); if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE) -- cgit v1.2.3 From dfd375864ac1e2ee60ed2d61820697ac01642f80 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Mon, 30 Nov 2020 23:04:12 -0800 Subject: 9p: Uninitialized variable in v9fs_writeback_fid() If v9fs_fid_lookup_with_uid() fails then "fid" is not initialized. The v9fs_fid_lookup_with_uid() can't return NULL. If it returns an error pointer then we can still pass that to clone_fid() and it will return the error pointer back again. Fixes: 6636b6dcc3db ("9p: add refcount to p9_fid struct") Signed-off-by: Dan Carpenter Signed-off-by: Dominique Martinet --- fs/9p/fid.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'fs/9p') diff --git a/fs/9p/fid.c b/fs/9p/fid.c index 50118ec72a92..79837f1b1c91 100644 --- a/fs/9p/fid.c +++ b/fs/9p/fid.c @@ -302,8 +302,7 @@ struct p9_fid *v9fs_writeback_fid(struct dentry *dentry) struct p9_fid *fid, *ofid; ofid = v9fs_fid_lookup_with_uid(dentry, GLOBAL_ROOT_UID, 0); - if (ofid && !IS_ERR(ofid)) - fid = clone_fid(ofid); + fid = clone_fid(ofid); if (IS_ERR(fid)) goto error_out; p9_client_clunk(ofid); -- cgit v1.2.3 From cfd1d0f524a87b7d6d14b41a14fa4cbe522cf8cc Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Tue, 1 Dec 2020 10:04:34 +0300 Subject: 9p: Remove unnecessary IS_ERR() check The "fid" variable can't be an error pointer so there is no need to check. The code is slightly cleaner if we move the increment before the break and remove the NULL check as well. Signed-off-by: Dan Carpenter Signed-off-by: Dominique Martinet --- fs/9p/fid.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'fs/9p') diff --git a/fs/9p/fid.c b/fs/9p/fid.c index 79837f1b1c91..9d9de62592be 100644 --- a/fs/9p/fid.c +++ b/fs/9p/fid.c @@ -56,12 +56,11 @@ static struct p9_fid *v9fs_fid_find_inode(struct inode *inode, kuid_t uid) h = (struct hlist_head *)&inode->i_private; hlist_for_each_entry(fid, h, ilist) { if (uid_eq(fid->uid, uid)) { + refcount_inc(&fid->count); ret = fid; break; } } - if (ret && !IS_ERR(ret)) - refcount_inc(&ret->count); spin_unlock(&inode->i_lock); return ret; } -- cgit v1.2.3