Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1428747 > unrolled thread

[PATCH 0/3] fs/9p: fix setattr/getattr issues with open files

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] fs/9p: fix setattr/getattr issues with open files Greg Kurz <groug@kaod.org> - 2016-06-22 14:30 +0200
    [PATCH 3/3] fs/9p: search open fids first Greg Kurz <groug@kaod.org> - 2016-06-22 14:30 +0200

#1428747 — [PATCH 0/3] fs/9p: fix setattr/getattr issues with open files

The 9p filesystem has some serious issues with all syscalls that deal
with file attributes out of a file descriptor instead of a path name
(aka. fstat, ftruncate, fchmod and friends). If the file is opened and
then unlinked, any subsequent call to the above syscalls will fail with
EACCES. The same will happen to ftruncate() if the file is no longer
writable.

The root cause to this is that the VFS layer does not pass the struct
file to the filesystem when it is about file attributes. This is
legitimate: when we reach setattr/getattr, we don't need anything to
be dereferenced from the struct file. But the current lookup code is
based on the list of fids hanging off the dentry: it is okay for path
name based syscalls, but not relialable in the case of file descriptors
because we cannot find an open fid this way.

Eric sent a patch last year to address the case when the file gets
unlinked:

https://patchwork.kernel.org/patch/6252761/

The general idea is to try to find an open fid, starting from the inode.
The patch does this by browsing the 9p client list of all fids when we
know the file was unlinked: if we find a fid, it is necessarily an open
fid.

I could find a newer version of this patch with some minor changes, here:

https://github.com/ericvh/linux/commits/v9fs-devel

Since the patch also fixes a few other things, I re-post it first in
this series. Eric, I hope it is okay for you ?

As suggested in the changelog, the lookup of open fid can be optimized
if we link open fids to the inode. This is addressed by the second patch.

The third patch addresses a related issue with ftruncate() on unwriteable
files.

This series can be tested with a custom QEMU, available here:

https://github.com/gkurz/qemu/commits/9p-attr-fixes

I could have most f*() syscalls working in the guest, with the notable
exception of xattr related ones because more code is needed in QEMU.

Please comment. Test reports will also be appreciated :)

---

Eric Van Hensbergen (1):
      fs/9p: fix create-unlink-getattr idiom

Greg Kurz (2):
      fs/9p: track open fids
      fs/9p: search open fids first


 fs/9p/fid.c             |   46 +++++++++++++++++++++++++++++++++++++++++++++-
 fs/9p/fid.h             |    1 +
 fs/9p/vfs_dir.c         |    5 +++++
 fs/9p/vfs_file.c        |    1 +
 fs/9p/vfs_inode.c       |   10 +++++++++-
 fs/9p/vfs_inode_dotl.c  |    1 +
 include/net/9p/client.h |    3 +++
 net/9p/client.c         |    5 ++++-
 8 files changed, 69 insertions(+), 3 deletions(-)

--
Greg

[toc] | [next] | [standalone]

#1428748 — [PATCH 3/3] 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

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 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 6ac68df50dca..ffe945995378 100644
--- a/fs/9p/fid.c
+++ b/fs/9p/fid.c
@@ -109,8 +109,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) {
@@ -120,9 +124,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;

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web