open-unlink-f*syscall bug is well-known in 9p. We try to fix the bug
in this patch set.
I take Eric's and Greg's patches which constiute the 1/4 - 3/4 of this patch
set as the main frame of the solution. In patch 4/4, I fix the fid race issue
exists in Greg's patch.
change log:
v1 to v2:
(1) in patch 4/4: do fid refcounter down in the clunk helper.
(2) int patch 4/4: remove the enum value denoting from which of the
inode or dentry fids are allcated.
Eric Van Hensbergen (1):
fs/9p: fix create-unlink-getattr idiom
Greg Kurz (1):
fs/9p: search open fids first
Jianyong Wu (2):
fs/9p: track open fids
9p: fix race issue in fid contention.
fs/9p/fid.c | 69 ++++++++++++++++++++++++++++++++++++++---
fs/9p/fid.h | 11 ++++++-
fs/9p/vfs_dentry.c | 2 ++
fs/9p/vfs_dir.c | 6 +++-
fs/9p/vfs_file.c | 1 +
fs/9p/vfs_inode.c | 47 ++++++++++++++++++++++------
fs/9p/vfs_inode_dotl.c | 35 +++++++++++++++++----
fs/9p/vfs_super.c | 1 +
fs/9p/xattr.c | 16 ++++++++--
include/net/9p/client.h | 7 +++++
net/9p/client.c | 14 ++++++---
11 files changed, 179 insertions(+), 30 deletions(-)
--
2.17.1
From: Greg Kurz <[email protected]>
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
Signed-off-by: Greg Kurz <[email protected]>
Signed-off-by: Jianyong Wu <[email protected]>
---
fs/9p/fid.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
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;
--
2.17.1
From: Greg Kurz <[email protected]>
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.
Signed-off-by: Greg Kurz <[email protected]>
Signed-off-by: Jianyong Wu <[email protected]>
---
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(-)
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 92cd1d80218d..b42cc1752cd1 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;
};
/**
--
2.17.1