backout the xattr override access checks flag

PATCH In review Christoph Hellwig 4 participants 6 messages Feb 20, 2003 – Feb 21, 2003

Subsystems (4)

VFS XFS filesystem ext2 file system JFS filesystem

lore

Share & tools

lore

Andreas Dilger

2003-02-20 20:20 UTC #1 lore raw

In reply to Christoph Hellwig

Review

Quoted 2 lines · Christoph Hellwig

(quoted diff) +1 -1

-	if (!((flags & XATTR_KERNEL_CONTEXT) || capable(CAP_SYS_ADMIN)))
+	if (!capable(CAP_SYS_ADMIN))

How about using CAP_DAC_OVERRIDE instead of CAP_SYS_ADMIN, since the latter is _way_ over used, and basically amounts to a uid=0 test these days.

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/

Christoph Hellwig

2003-02-20 20:26 UTC #2 lore raw

Quoted 7 lines · Andreas Dilger

Review
Quoted 2 lines · Christoph Hellwig
(quoted diff) +1 -1
-	if (!((flags & XATTR_KERNEL_CONTEXT) || capable(CAP_SYS_ADMIN)))
+	if (!capable(CAP_SYS_ADMIN))
How about using CAP_DAC_OVERRIDE instead of CAP_SYS_ADMIN, since the latter is _way_ over used, and basically amounts to a uid=0 test these days.

It's not a DAC override strictly spoken. But I agree with you that it shouldn't be CAP_SYS_ADMIN (see my previous discussion with Andreas).

Neverless this is purely a change to backout the flags change and I don't want to mix up too much.

Christoph Hellwig

author

2003-02-21 03:20 UTC #0 lore raw

PATCH 1/1 backout the xattr override access checks flag

This check backs out the XATTR_KERNEL_CONTEXT that tells filesystems to ignore the lack of capabilities of the caller that went in in the last week (through akpm..).

It's a cludgy interface design to have flags that change access checks, and there's a much easier way by raising the capabilities in the process that actually needs this in kernel mode (not that such code even exists yet).

Something even better would probably be to move out the xattr access checks to common code..

fs/xattr.c +3 -3

@@ -160,7 +160,7 @@
 		if (error)
 			goto out;
 		down(&d->d_inode->i_sem);
-		error = d->d_inode->i_op->getxattr(d, kname, kvalue, size, 0);
+		error = d->d_inode->i_op->getxattr(d, kname, kvalue, size);
 		up(&d->d_inode->i_sem);
 	}

@@ -233,7 +233,7 @@
 		if (error)
 			goto out;
 		down(&d->d_inode->i_sem);
-		error = d->d_inode->i_op->listxattr(d, klist, size, 0);
+		error = d->d_inode->i_op->listxattr(d, klist, size);
 		up(&d->d_inode->i_sem);
 	}

@@ -308,7 +308,7 @@
 		if (error)
 			goto out;
 		down(&d->d_inode->i_sem);
-		error = d->d_inode->i_op->removexattr(d, kname, 0);
+		error = d->d_inode->i_op->removexattr(d, kname);
 		up(&d->d_inode->i_sem);
 	}
 out:

fs/ext2/acl.c +5 -6

@@ -419,7 +419,7 @@
  */
 static size_t
 ext2_xattr_list_acl_access(char *list, struct inode *inode,
-			   const char *name, int name_len, int flags)
+			   const char *name, int name_len)
 {
 	const size_t size = sizeof(XATTR_NAME_ACL_ACCESS);

@@ -432,7 +432,7 @@
 
 static size_t
 ext2_xattr_list_acl_default(char *list, struct inode *inode,
-			    const char *name, int name_len, int flags)
+			    const char *name, int name_len)
 {
 	const size_t size = sizeof(XATTR_NAME_ACL_DEFAULT);

@@ -465,7 +465,7 @@
 
 static int
 ext2_xattr_get_acl_access(struct inode *inode, const char *name,
-			  void *buffer, size_t size, int flags)
+			  void *buffer, size_t size)
 {
 	if (strcmp(name, "") != 0)
 		return -EINVAL;

@@ -474,7 +474,7 @@
 
 static int
 ext2_xattr_get_acl_default(struct inode *inode, const char *name,
-			   void *buffer, size_t size, int flags)
+			   void *buffer, size_t size)
 {
 	if (strcmp(name, "") != 0)
 		return -EINVAL;

@@ -482,8 +482,7 @@
 }
 
 static int
-ext2_xattr_set_acl(struct inode *inode, int type, const void *value,
-		   size_t size)
+ext2_xattr_set_acl(struct inode *inode, int type, const void *value, size_t size)
 {
 	struct posix_acl *acl;
 	int error;

===== fs/ext2/xattr.c 1.8 vs edited =====

fs/ext2/xattr.c +9 ~~-10~~

@@ -199,7 +199,7 @@
  */
 ssize_t
 ext2_getxattr(struct dentry *dentry, const char *name,
-	      void *buffer, size_t size, int flags)
+	      void *buffer, size_t size)
 {
 	struct ext2_xattr_handler *handler;
 	struct inode *inode = dentry->d_inode;

@@ -207,7 +207,7 @@
 	handler = ext2_xattr_resolve_name(&name);
 	if (!handler)
 		return -EOPNOTSUPP;
-	return handler->get(inode, name, buffer, size, flags);
+	return handler->get(inode, name, buffer, size);
 }
 
 /*

@@ -217,9 +217,9 @@
  * BKL held [before 2.5.x]
  */
 ssize_t
-ext2_listxattr(struct dentry *dentry, char *buffer, size_t size, int flags)
+ext2_listxattr(struct dentry *dentry, char *buffer, size_t size)
 {
-	return ext2_xattr_list(dentry->d_inode, buffer, size, flags);
+	return ext2_xattr_list(dentry->d_inode, buffer, size);
 }
 
 /*

@@ -250,7 +250,7 @@
  * BKL held [before 2.5.x]
  */
 int
-ext2_removexattr(struct dentry *dentry, const char *name, int flags)
+ext2_removexattr(struct dentry *dentry, const char *name)
 {
 	struct ext2_xattr_handler *handler;
 	struct inode *inode = dentry->d_inode;

@@ -258,7 +258,7 @@
 	handler = ext2_xattr_resolve_name(&name);
 	if (!handler)
 		return -EOPNOTSUPP;
-	return handler->set(inode, name, NULL, 0, flags | XATTR_REPLACE);
+	return handler->set(inode, name, NULL, 0, XATTR_REPLACE);
 }
 
 /*

@@ -371,8 +371,7 @@
  * used / required on success.
  */
 int
-ext2_xattr_list(struct inode *inode, char *buffer, size_t buffer_size,
-		int flags)
+ext2_xattr_list(struct inode *inode, char *buffer, size_t buffer_size)
 {
 	struct buffer_head *bh = NULL;
 	struct ext2_xattr_entry *entry;

@@ -412,7 +411,7 @@
 		handler = ext2_xattr_handler(entry->e_name_index);
 		if (handler)
 			size += handler->list(NULL, inode, entry->e_name,
-					      entry->e_name_len, flags);
+					      entry->e_name_len);
 	}
 
 	if (ext2_xattr_cache_insert(bh))

@@ -435,7 +434,7 @@
 		handler = ext2_xattr_handler(entry->e_name_index);
 		if (handler)
 			buf += handler->list(buf, inode, entry->e_name,
-					     entry->e_name_len, flags);
+					     entry->e_name_len);
 	}
 	error = size;

===== fs/ext2/xattr.h 1.4 vs edited =====

fs/ext2/xattr.h +6 -6

@@ -58,9 +58,9 @@
 struct ext2_xattr_handler {
 	char *prefix;
 	size_t (*list)(char *list, struct inode *inode, const char *name,
-		       int name_len, int flags);
+		       int name_len);
 	int (*get)(struct inode *inode, const char *name, void *buffer,
-		   size_t size, int flags);
+		   size_t size);
 	int (*set)(struct inode *inode, const char *name, const void *buffer,
 		   size_t size, int flags);
 };

@@ -69,12 +69,12 @@
 extern void ext2_xattr_unregister(int, struct ext2_xattr_handler *);
 
 extern int ext2_setxattr(struct dentry *, const char *, const void *, size_t, int);
-extern ssize_t ext2_getxattr(struct dentry *, const char *, void *, size_t, int);
-extern ssize_t ext2_listxattr(struct dentry *, char *, size_t, int);
-extern int ext2_removexattr(struct dentry *, const char *, int);
+extern ssize_t ext2_getxattr(struct dentry *, const char *, void *, size_t);
+extern ssize_t ext2_listxattr(struct dentry *, char *, size_t);
+extern int ext2_removexattr(struct dentry *, const char *);
 
 extern int ext2_xattr_get(struct inode *, int, const char *, void *, size_t);
-extern int ext2_xattr_list(struct inode *, char *, size_t, int flags);
+extern int ext2_xattr_list(struct inode *, char *, size_t);
 extern int ext2_xattr_set(struct inode *, int, const char *, const void *, size_t, int);
 
 extern void ext2_xattr_delete_inode(struct inode *);

===== fs/ext2/xattr_trusted.c 1.1 vs edited =====

fs/ext2/xattr_trusted.c +3 -3

@@ -20,7 +20,7 @@
 {
 	const int prefix_len = sizeof(XATTR_TRUSTED_PREFIX)-1;
 
-	if (!((flags & XATTR_KERNEL_CONTEXT) || capable(CAP_SYS_ADMIN)))
+	if (!capable(CAP_SYS_ADMIN))
 		return 0;
 
 	if (list) {

@@ -37,7 +37,7 @@
 {
 	if (strcmp(name, "") == 0)
 		return -EINVAL;
-	if (!((flags & XATTR_KERNEL_CONTEXT) || capable(CAP_SYS_ADMIN)))
+	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	return ext2_xattr_get(inode, EXT2_XATTR_INDEX_TRUSTED, name,
 			      buffer, size);

@@ -49,7 +49,7 @@
 {
 	if (strcmp(name, "") == 0)
 		return -EINVAL;
-	if (!((flags & XATTR_KERNEL_CONTEXT) || capable(CAP_SYS_ADMIN)))
+	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	return ext2_xattr_set(inode, EXT2_XATTR_INDEX_TRUSTED, name,
 			      value, size, flags);

===== fs/ext2/xattr_user.c 1.5 vs edited =====

fs/ext2/xattr_user.c +21 ~~-24~~

80 lines not on this page · raw message

===== fs/ext3/acl.c 1.5 vs edited =====

fs/ext3/acl.c +5 -6

@@ -431,7 +431,7 @@
  */
 static size_t
 ext3_xattr_list_acl_access(char *list, struct inode *inode,
-			   const char *name, int name_len, int flags)
+			   const char *name, int name_len)
 {
 	const size_t size = sizeof(XATTR_NAME_ACL_ACCESS);

@@ -444,7 +444,7 @@
 
 static size_t
 ext3_xattr_list_acl_default(char *list, struct inode *inode,
-			    const char *name, int name_len, int flags)
+			    const char *name, int name_len)
 {
 	const size_t size = sizeof(XATTR_NAME_ACL_DEFAULT);

@@ -477,7 +477,7 @@
 
 static int
 ext3_xattr_get_acl_access(struct inode *inode, const char *name,
-			  void *buffer, size_t size, int flags)
+			  void *buffer, size_t size)
 {
 	if (strcmp(name, "") != 0)
 		return -EINVAL;

@@ -486,7 +486,7 @@
 
 static int
 ext3_xattr_get_acl_default(struct inode *inode, const char *name,
-			   void *buffer, size_t size, int flags)
+			   void *buffer, size_t size)
 {
 	if (strcmp(name, "") != 0)
 		return -EINVAL;

@@ -494,8 +494,7 @@
 }
 
 static int
-ext3_xattr_set_acl(struct inode *inode, int type, const void *value,
-		   size_t size)
+ext3_xattr_set_acl(struct inode *inode, int type, const void *value, size_t size)
 {
 	handle_t *handle;
 	struct posix_acl *acl;

===== fs/ext3/xattr.c 1.9 vs edited =====

fs/ext3/xattr.c +9 ~~-10~~

@@ -195,7 +195,7 @@
  */
 ssize_t
 ext3_getxattr(struct dentry *dentry, const char *name,
-	      void *buffer, size_t size, int flags)
+	      void *buffer, size_t size)
 {
 	struct ext3_xattr_handler *handler;
 	struct inode *inode = dentry->d_inode;

@@ -203,7 +203,7 @@
 	handler = ext3_xattr_resolve_name(&name);
 	if (!handler)
 		return -EOPNOTSUPP;
-	return handler->get(inode, name, buffer, size, flags);
+	return handler->get(inode, name, buffer, size);
 }
 
 /*

@@ -212,9 +212,9 @@
  * dentry->d_inode->i_sem down
  */
 ssize_t
-ext3_listxattr(struct dentry *dentry, char *buffer, size_t size, int flags)
+ext3_listxattr(struct dentry *dentry, char *buffer, size_t size)
 {
-	return ext3_xattr_list(dentry->d_inode, buffer, size, flags);
+	return ext3_xattr_list(dentry->d_inode, buffer, size);
 }
 
 /*

@@ -243,7 +243,7 @@
  * dentry->d_inode->i_sem down
  */
 int
-ext3_removexattr(struct dentry *dentry, const char *name, int flags)
+ext3_removexattr(struct dentry *dentry, const char *name)
 {
 	struct ext3_xattr_handler *handler;
 	struct inode *inode = dentry->d_inode;

@@ -251,7 +251,7 @@
 	handler = ext3_xattr_resolve_name(&name);
 	if (!handler)
 		return -EOPNOTSUPP;
-	return handler->set(inode, name, NULL, 0, flags | XATTR_REPLACE);
+	return handler->set(inode, name, NULL, 0, XATTR_REPLACE);
 }
 
 /*

@@ -364,8 +364,7 @@
  * used / required on success.
  */
 int
-ext3_xattr_list(struct inode *inode, char *buffer, size_t buffer_size,
-		int flags)
+ext3_xattr_list(struct inode *inode, char *buffer, size_t buffer_size)
 {
 	struct buffer_head *bh = NULL;
 	struct ext3_xattr_entry *entry;

@@ -405,7 +404,7 @@
 		handler = ext3_xattr_handler(entry->e_name_index);
 		if (handler)
 			size += handler->list(NULL, inode, entry->e_name,
-					      entry->e_name_len, flags);
+					      entry->e_name_len);
 	}
 
 	if (ext3_xattr_cache_insert(bh))

@@ -428,7 +427,7 @@
 		handler = ext3_xattr_handler(entry->e_name_index);
 		if (handler)
 			buf += handler->list(buf, inode, entry->e_name,
-					     entry->e_name_len, flags);
+					     entry->e_name_len);
 	}
 	error = size;

===== fs/ext3/xattr.h 1.5 vs edited =====

fs/ext3/xattr.h +6 -6

@@ -57,9 +57,9 @@
 struct ext3_xattr_handler {
 	char *prefix;
 	size_t (*list)(char *list, struct inode *inode, const char *name,
-		       int name_len, int flags);
+		       int name_len);
 	int (*get)(struct inode *inode, const char *name, void *buffer,
-		   size_t size, int flags);
+		   size_t size);
 	int (*set)(struct inode *inode, const char *name, const void *buffer,
 		   size_t size, int flags);
 };

@@ -68,12 +68,12 @@
 extern void ext3_xattr_unregister(int, struct ext3_xattr_handler *);
 
 extern int ext3_setxattr(struct dentry *, const char *, const void *, size_t, int);
-extern ssize_t ext3_getxattr(struct dentry *, const char *, void *, size_t, int);
-extern ssize_t ext3_listxattr(struct dentry *, char *, size_t, int);
-extern int ext3_removexattr(struct dentry *, const char *, int);
+extern ssize_t ext3_getxattr(struct dentry *, const char *, void *, size_t);
+extern ssize_t ext3_listxattr(struct dentry *, char *, size_t);
+extern int ext3_removexattr(struct dentry *, const char *);
 
 extern int ext3_xattr_get(struct inode *, int, const char *, void *, size_t);
-extern int ext3_xattr_list(struct inode *, char *, size_t, int flags);
+extern int ext3_xattr_list(struct inode *, char *, size_t);
 extern int ext3_xattr_set(struct inode *, int, const char *, const void *, size_t, int);
 extern int ext3_xattr_set_handle(handle_t *, struct inode *, int, const char *, const void *, size_t, int);

===== fs/ext3/xattr_trusted.c 1.1 vs edited =====

fs/ext3/xattr_trusted.c +3 -3

@@ -21,7 +21,7 @@
 {
 	const int prefix_len = sizeof(XATTR_TRUSTED_PREFIX)-1;
 
-	if (!((flags & XATTR_KERNEL_CONTEXT) || capable(CAP_SYS_ADMIN)))
+	if (!capable(CAP_SYS_ADMIN))
 		return 0;
 
 	if (list) {

@@ -38,7 +38,7 @@
 {
 	if (strcmp(name, "") == 0)
 		return -EINVAL;
-	if (!((flags & XATTR_KERNEL_CONTEXT) || capable(CAP_SYS_ADMIN)))
+	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	return ext3_xattr_get(inode, EXT3_XATTR_INDEX_TRUSTED, name,
 			      buffer, size);

@@ -50,7 +50,7 @@
 {
 	if (strcmp(name, "") == 0)
 		return -EINVAL;
-	if (!((flags & XATTR_KERNEL_CONTEXT) || capable(CAP_SYS_ADMIN)))
+	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	return ext3_xattr_set(inode, EXT3_XATTR_INDEX_TRUSTED, name,
 			      value, size, flags);

===== fs/ext3/xattr_user.c 1.6 vs edited =====

fs/ext3/xattr_user.c +22 ~~-24~~

83 lines not on this page · raw message

===== fs/jfs/jfs_xattr.h 1.4 vs edited =====

fs/jfs/jfs_xattr.h +3 -3

@@ -57,8 +57,8 @@
 extern int jfs_setxattr(struct dentry *, const char *, const void *, size_t,
 			int);
 extern ssize_t __jfs_getxattr(struct inode *, const char *, void *, size_t);
-extern ssize_t jfs_getxattr(struct dentry *, const char *, void *, size_t, int);
-extern ssize_t jfs_listxattr(struct dentry *, char *, size_t, int);
-extern int jfs_removexattr(struct dentry *, const char *, int);
+extern ssize_t jfs_getxattr(struct dentry *, const char *, void *, size_t);
+extern ssize_t jfs_listxattr(struct dentry *, char *, size_t);
+extern int jfs_removexattr(struct dentry *, const char *);
 
 #endif	/* H_JFS_XATTR */

===== fs/jfs/xattr.c 1.11 vs edited =====

fs/jfs/xattr.c +3 -4

@@ -962,13 +962,12 @@
 }
 
 ssize_t jfs_getxattr(struct dentry *dentry, const char *name, void *data,
-		     size_t buf_size, int flags)
+		     size_t buf_size)
 {
 	return __jfs_getxattr(dentry->d_inode, name, data, buf_size);
 }
 
-ssize_t jfs_listxattr(struct dentry * dentry, char *data, size_t buf_size,
-		      int flags)
+ssize_t jfs_listxattr(struct dentry * dentry, char *data, size_t buf_size)
 {
 	struct inode *inode = dentry->d_inode;
 	char *buffer;

@@ -1014,7 +1013,7 @@
 	return size;
 }
 
-int jfs_removexattr(struct dentry *dentry, const char *name, int flags)
+int jfs_removexattr(struct dentry *dentry, const char *name)
 {
 	return __jfs_setxattr(dentry->d_inode, name, 0, 0, XATTR_REPLACE);
 }

===== fs/xfs/linux/xfs_iops.c 1.15 vs edited =====

fs/xfs/linux/xfs_iops.c +3 -6

@@ -640,8 +640,7 @@
 	struct dentry	*dentry,
 	const char	*name,
 	void		*data,
-	size_t		size,
-	int		flags)
+	size_t		size)
 {
 	ssize_t		error;
 	int		xflags = 0;

@@ -698,8 +697,7 @@
 linvfs_listxattr(
 	struct dentry		*dentry,
 	char			*data,
-	size_t			size,
-	int			flags)
+	size_t			size)
 {
 	ssize_t			error;
 	int			result = 0;

@@ -743,8 +741,7 @@
 STATIC int
 linvfs_removexattr(
 	struct dentry	*dentry,
-	const char	*name,
-	int		flags)
+	const char	*name)
 {
 	int		error;
 	int		xflags = 0;

===== include/linux/fs.h 1.219 vs edited =====

include/linux/fs.h +3 -3

@@ -743,9 +743,9 @@
 	int (*setattr) (struct dentry *, struct iattr *);
 	int (*getattr) (struct vfsmount *mnt, struct dentry *, struct kstat *);
 	int (*setxattr) (struct dentry *, const char *,const void *,size_t,int);
-	ssize_t (*getxattr) (struct dentry *, const char *, void *, size_t,int);
-	ssize_t (*listxattr) (struct dentry *, char *, size_t, int);
-	int (*removexattr) (struct dentry *, const char *, int);
+	ssize_t (*getxattr) (struct dentry *, const char *, void *, size_t);
+	ssize_t (*listxattr) (struct dentry *, char *, size_t);
+	int (*removexattr) (struct dentry *, const char *);
 };
 
 struct seq_file;

===== include/linux/i2c-proc.h 1.4 vs edited =====

include/linux/i2c-proc.h +1 -2

@@ -69,8 +69,7 @@
    these functions must be updated! */
 extern int i2c_register_entry(struct i2c_client *client,
 				  const char *prefix,
-				  ctl_table * ctl_template,
-				  struct module *controlling_mod);
+				  ctl_table * ctl_template);
 
 extern void i2c_deregister_entry(int id);

===== include/linux/xattr.h 1.3 vs edited =====

include/linux/xattr.h +2 -3

@@ -9,8 +9,7 @@
 #ifndef _LINUX_XATTR_H
 #define _LINUX_XATTR_H
 
-#define XATTR_CREATE		0x1	/* fail if attr already exists */
-#define XATTR_REPLACE		0x2	/* fail if attr does not exist */
-#define XATTR_KERNEL_CONTEXT	0x4	/* called from kernel context */
+#define XATTR_CREATE	0x1	/* set value, fail if attr already exists */
+#define XATTR_REPLACE	0x2	/* set value, fail if attr does not exist */
 
 #endif	/* _LINUX_XATTR_H */

Andreas Gruenbacher

2003-02-21 10:20 UTC #3 lore raw

Quoted 7 lines · you

This check backs out the XATTR_KERNEL_CONTEXT that tells filesystems to ignore the lack of capabilities of the caller that went in in the last week (through akpm..).
It's a cludgy interface design to have flags that change access checks, and there's a much easier way by raising the capabilities in the process that actually needs this in kernel mode

Thanks, I'm not fully convinced that raising capabilities temporarily in the kernel is a good thing to do, but it surely obsoletes the XATTR_KERNEL_CONTEXT flag.

Quoted 1 line

(not that such code even exists yet).

There is an HSM project for which this feature has been added. I think they are using a loadable module.

Quoted 2 lines

Something even better would probably be to move out the xattr access checks to common code.

There are two problems with that, so this doesn't seem any better to me, either:

(a) We would have to decode attribute names twice, once for checking permissions, and a second time for determining how to store them.

(b) Different file systems may implement different features with different, file system specific limitations. The VFS layer tests would have to accept all potentially useful things. The file system would have to re-check.

Cheers,
Andreas.

Christoph Hellwig

2003-02-21 15:54 UTC #4 lore raw

Quoted 4 lines · Andreas Gruenbacher

Quoted 1 line
(not that such code even exists yet).
There is an HSM project for which this feature has been added. I think they are using a loadable module.

URL?

Quoted 9 lines

Quoted 3 lines
Something even better would probably be to move out the xattr access checks to common code.
There are two problems with that, so this doesn't seem any better to me, either:
(a) We would have to decode attribute names twice, once for checking permissions, and a second time for determining how to store them.

Doing it in the VFS would probably mean a rather large interface change so it should be decoded only once. I.e. moving your current ext2/ext3-specific handler abstraction to the VFS instead.

Quoted 3 lines

(b) Different file systems may implement different features with different, file system specific limitations. The VFS layer tests would have to accept all potentially useful things. The file system would have to re-check.

What types of EAs do we have?

(1) user attributes - the only access checks needed are the normal DAC ones
(2) system/trusted - only privilegued access

I think that's doable.

Andreas Gruenbacher

2003-02-21 16:32 UTC #5 lore raw

Quoted 26 lines · Christoph Hellwig

Quoted 4 lines · Andreas Gruenbacher
Quoted 1 line
(not that such code even exists yet).
There is an HSM project for which this feature has been added. I think they are using a loadable module.
URL?
Quoted 8 lines
Quoted 2 lines
Something even better would probably be to move out the xattr access checks to common code.
There are two problems with that, so this doesn't seem any better to me, either:
(a) We would have to decode attribute names twice, once for checking permissions, and a second time for determining how to store them.
Doing it in the VFS would probably mean a rather large interface change so it should be decoded only once. I.e. moving your current ext2/ext3-specific handler abstraction to the VFS instead.
Quoted 4 lines
(b) Different file systems may implement different features with different, file system specific limitations. The VFS layer tests would have to accept all potentially useful things. The file system would have to re-check.

Quoted 6 lines

What types of EAs do we have?
(1) user attributes - the only access checks needed are the normal DAC ones
(2) system/trusted - only privilegued access
I think that's doable.

User and Trusted extended attributes have permission checks per namespace (i.e., the rules for all user.* attributes are identical, and the rules for all trusted.* attributes are identical). System extended attribute access rules are per attribute: ACLs, Capabilities, MAC all have different policies.

Also, different file systems have different mechanisms for storing EAs, so that need to decode the attribute name, no matter what the VFS does.

I was thinking of at least abstracting the the permission checks from the file systems, so the file system layers can all use the same VFS helper function. But this is no real improvement, either:

	enum xattr_which { XATTR_USER, XATTR_TRUSTED,
				XATTR_POSIX_ACL, XATTR_CAP, ... };

	int xattr_permission(struct inode *inode, enum xattr_which which, int mask)
	{
		switch(which) {
			case XATTR_USER:
				return permission(inode, mask);

			case XATTR_TRUSTED:
				return capable(CAP_WHATEVER);

			case XATTR_POSIX_ACL:
				if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
					return -EPERM;
				return 0;
			default:
				return -EOPNOTSUPP;
		}
	}

This pseudo code is actually broken because it needs to be called from within the xattr inode operations where the inode lock is held, so the VFS permission() function cannot even be called because iops->permission() might grab the inode lock as well (and needs to on ACL aware systems).

So I agree with you that the checks could be moved to the VFS, but I don't see how this can be done in a way that is clearly better that keeping the checks in the FS.

Cheers,
Andreas.