[V2] orangefs: posix open permission checking

Commit Message

[e-mail] Nov. 25, 2019, 3:12 p.m. UTC

From: Mike Marshall <[e-mail]>


Here's another approach that might not be as broken...

Orangefs has no open. Orangefs does permission
checking each time a file is accessed. The Posix
model is for permission checks to be done at open.

This patch brings orangefs-through-the-kernel closer
to Posix by marking inodes "opened" when they pass
through file_operations->open and using a UID whose
capabilities include PINT_CAP_WRITE and PINT_CAP_READ
for IO associated with open files.

I have another proof-of-concept version of this patch which sends a
message to the userspace server to add PINT_CAP_WRITE and PINT_CAP_READ
to whatever arbitrary UID is doing IO, in case using the
root UID for the entire IO is not conservative enough... this
version also requires a patch to the userspace part of Orangefs.

  "root has every possible capability - PINT_get_capabilities"

Signed-off-by: Mike Marshall <[e-mail]>
---
 fs/orangefs/file.c            | 4 ++++
 fs/orangefs/inode.c           | 1 +
 fs/orangefs/orangefs-kernel.h | 1 +
 3 files changed, 6 insertions(+)

Comments

Linus Torvalds Nov. 25, 2019, 4:55 p.m. UTC | #1

On Mon, Nov 25, 2019 at 7:12 AM <[e-mail]> wrote:
>
> @@ -90,6 +90,8 @@ ssize_t wait_for_direct_io(enum ORANGEFS_io_type type, struct inode *inode,
>                 new_op->upcall.uid = from_kuid(&init_user_ns, wr->uid);
>                 new_op->upcall.gid = from_kgid(&init_user_ns, wr->gid);
>         }
> +       if (new_op->upcall.uid && (ORANGEFS_I(inode)->opened))
> +               new_op->upcall.uid = 0;

You still can't do this.

You can't make it part of the inode state, because the inode is shared
across different file descriptors. So you are giving a potentially
different file descriptor (that really was opened just for reading)
the magical override.

What you *should* do is to always use the credentials at open time,
and the "can I read or write this" from open time.

And regardless of whether you have your own open routine or not, those
are always available as "file->f_mode & FMODE_WRITE" and
"file->f_cred".

If you use those - and pretty much *ONLY* if you use those - you will
get things right.

             Linus

Patch

diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
index a5612abc0936..a6de17889682 100644
--- a/fs/orangefs/file.c
+++ b/fs/orangefs/file.c
@@ -90,6 +90,8 @@  ssize_t wait_for_direct_io(enum ORANGEFS_io_type type, struct inode *inode,
 		new_op->upcall.uid = from_kuid(&init_user_ns, wr->uid);
 		new_op->upcall.gid = from_kgid(&init_user_ns, wr->gid);
 	}
+	if (new_op->upcall.uid && (ORANGEFS_I(inode)->opened))
+		new_op->upcall.uid = 0;
 
 	gossip_debug(GOSSIP_FILE_DEBUG,
 		     "%s(%pU): offset: %llu total_size: %zd\n",
@@ -495,6 +497,7 @@  static int orangefs_file_release(struct inode *inode, struct file *file)
 		     "orangefs_file_release: called on %pD\n",
 		     file);
 
+	ORANGEFS_I(inode)->opened = 0;
 	/*
 	 * remove all associated inode pages from the page cache and
 	 * readahead cache (if any); this forces an expensive refresh of
@@ -618,6 +621,7 @@  static int orangefs_lock(struct file *filp, int cmd, struct file_lock *fl)
 static int orangefs_file_open(struct inode * inode, struct file *file)
 {
 	file->private_data = NULL;
+	ORANGEFS_I(inode)->opened = 1;
 	return generic_file_open(inode, file);
 }
 
diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
index efb12197da18..dc6ced95e888 100644
--- a/fs/orangefs/inode.c
+++ b/fs/orangefs/inode.c
@@ -1060,6 +1060,7 @@  static int orangefs_set_inode(struct inode *inode, void *data)
 	hash_init(ORANGEFS_I(inode)->xattr_cache);
 	ORANGEFS_I(inode)->mapping_time = jiffies - 1;
 	ORANGEFS_I(inode)->bitlock = 0;
+	ORANGEFS_I(inode)->opened = 0;
 	return 0;
 }
 
diff --git a/fs/orangefs/orangefs-kernel.h b/fs/orangefs/orangefs-kernel.h
index 34a6c99fa29b..0ce4a6af716d 100644
--- a/fs/orangefs/orangefs-kernel.h
+++ b/fs/orangefs/orangefs-kernel.h
@@ -198,6 +198,7 @@  struct orangefs_inode_s {
 	kuid_t attr_uid;
 	kgid_t attr_gid;
 	unsigned long bitlock;
+	int opened;
 
 	DECLARE_HASHTABLE(xattr_cache, 4);
 };