Re: [PATCH] generic acl support for ->permission

[Andreas Gruenbacher <agruen@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 417 bytes --]

Hello Christoph,

as far as I can see what you propose is very similar to the approach taken in 
the attached patch, with one more indirection. This looks nicer to me, at the 
cost of "polluting" iops instead of vfs_permission. JFS will still need 
Dave's patch, obviously.

The attached patch only covers ext3; not sure if it even compiles.

Cheers,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX AG

[-- Attachment #2: other.diff --]
[-- Type: text/x-diff, Size: 6044 bytes --]

Index: linux-2.6.9-rc1-bk8/fs/ext3/file.c
===================================================================
--- linux-2.6.9-rc1-bk8.orig/fs/ext3/file.c
+++ linux-2.6.9-rc1-bk8/fs/ext3/file.c
@@ -136,6 +136,6 @@ struct inode_operations ext3_file_inode_
 	.getxattr	= ext3_getxattr,
 	.listxattr	= ext3_listxattr,
 	.removexattr	= ext3_removexattr,
-	.permission	= ext3_permission,
+	.check_posix_acl = ext3_check_posix_acl,
 };
 
Index: linux-2.6.9-rc1-bk8/fs/ext3/acl.h
===================================================================
--- linux-2.6.9-rc1-bk8.orig/fs/ext3/acl.h
+++ linux-2.6.9-rc1-bk8/fs/ext3/acl.h
@@ -59,7 +59,7 @@ static inline int ext3_acl_count(size_t 
 #define EXT3_ACL_NOT_CACHED ((void *)-1)
 
 /* acl.c */
-extern int ext3_permission (struct inode *, int, struct nameidata *);
+extern int ext3_check_posix_acl (struct inode *, int);
 extern int ext3_acl_chmod (struct inode *);
 extern int ext3_init_acl (handle_t *, struct inode *, struct inode *);
 
@@ -68,7 +68,7 @@ extern void exit_ext3_acl(void);
 
 #else  /* CONFIG_EXT3_FS_POSIX_ACL */
 #include <linux/sched.h>
-#define ext3_permission NULL
+#define ext3_check_posix_acl NULL
 
 static inline int
 ext3_acl_chmod(struct inode *inode)
Index: linux-2.6.9-rc1-bk8/fs/namei.c
===================================================================
--- linux-2.6.9-rc1-bk8.orig/fs/namei.c
+++ linux-2.6.9-rc1-bk8/fs/namei.c
@@ -180,8 +180,19 @@ int vfs_permission(struct inode * inode,
 
 	if (current->fsuid == inode->i_uid)
 		mode >>= 6;
-	else if (in_group_p(inode->i_gid))
-		mode >>= 3;
+	else {
+		if (IS_POSIXACL(inode) && (mode & S_IRWXG) &&
+		    inode->i_op->check_posix_acl) {
+			int error = inode->i_op->check_posix_acl(inode, mask);
+			if (error == -EACCES)
+				goto check_capabilities;
+			else if (error != -EAGAIN)
+				return error;
+		}
+		
+		if (in_group_p(inode->i_gid))
+			mode >>= 3;
+	}
 
 	/*
 	 * If the DACs are ok we don't need any capability check.
@@ -189,6 +200,7 @@ int vfs_permission(struct inode * inode,
 	if (((mode & mask & (MAY_READ|MAY_WRITE|MAY_EXEC)) == mask))
 		return 0;
 
+check_capabilities:
 	/*
 	 * Read/write DACs are always overridable.
 	 * Executable DACs are overridable if at least one exec bit is set.
@@ -208,7 +220,7 @@ int vfs_permission(struct inode * inode,
 	return -EACCES;
 }
 
-int permission(struct inode * inode,int mask, struct nameidata *nd)
+int permission(struct inode * inode, int mask, struct nameidata *nd)
 {
 	int retval;
 	int submask;
Index: linux-2.6.9-rc1-bk8/include/linux/fs.h
===================================================================
--- linux-2.6.9-rc1-bk8.orig/include/linux/fs.h
+++ linux-2.6.9-rc1-bk8/include/linux/fs.h
@@ -930,6 +930,7 @@ struct inode_operations {
 	void (*put_link) (struct dentry *, struct nameidata *);
 	void (*truncate) (struct inode *);
 	int (*permission) (struct inode *, int, struct nameidata *);
+	int (*check_posix_acl) (struct inode *, int);
 	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);
Index: linux-2.6.9-rc1-bk8/fs/ext3/acl.c
===================================================================
--- linux-2.6.9-rc1-bk8.orig/fs/ext3/acl.c
+++ linux-2.6.9-rc1-bk8/fs/ext3/acl.c
@@ -285,60 +285,17 @@ ext3_set_acl(handle_t *handle, struct in
 	return error;
 }
 
-/*
- * Inode operation permission().
- *
- * inode->i_sem: don't care
- */
 int
-ext3_permission(struct inode *inode, int mask, struct nameidata *nd)
+ext3_check_posix_acl(struct inode *inode, int mask)
 {
-	int mode = inode->i_mode;
+	struct posix_acl *acl = ext3_get_acl(inode, ACL_TYPE_ACCESS);
+	int error = -EAGAIN;
 
-	/* Nobody gets write access to a read-only fs */
-	if ((mask & MAY_WRITE) && IS_RDONLY(inode) &&
-	    (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode)))
-		return -EROFS;
-	/* Nobody gets write access to an immutable file */
-	if ((mask & MAY_WRITE) && IS_IMMUTABLE(inode))
-	    return -EACCES;
-	if (current->fsuid == inode->i_uid) {
-		mode >>= 6;
-	} else if (test_opt(inode->i_sb, POSIX_ACL)) {
-		struct posix_acl *acl;
-
-		/* The access ACL cannot grant access if the group class
-		   permission bits don't contain all requested permissions. */
-		if (((mode >> 3) & mask & S_IRWXO) != mask)
-			goto check_groups;
-		acl = ext3_get_acl(inode, ACL_TYPE_ACCESS);
-		if (acl) {
-			int error = posix_acl_permission(inode, acl, mask);
-			posix_acl_release(acl);
-			if (error == -EACCES)
-				goto check_capabilities;
-			return error;
-		} else
-			goto check_groups;
-	} else {
-check_groups:
-		if (in_group_p(inode->i_gid))
-			mode >>= 3;
+	if (acl) {
+		error = posix_acl_permission(inode, acl, mask);
+		posix_acl_release(acl);
 	}
-	if ((mode & mask & S_IRWXO) == mask)
-		return 0;
-
-check_capabilities:
-	/* Allowed to override Discretionary Access Control? */
-	if (!(mask & MAY_EXEC) ||
-	    (inode->i_mode & S_IXUGO) || S_ISDIR(inode->i_mode))
-		if (capable(CAP_DAC_OVERRIDE))
-			return 0;
-	/* Read and search granted if capable(CAP_DAC_READ_SEARCH) */
-	if (capable(CAP_DAC_READ_SEARCH) && ((mask == MAY_READ) ||
-	    (S_ISDIR(inode->i_mode) && !(mask & MAY_WRITE))))
-		return 0;
-	return -EACCES;
+	return error;
 }
 
 /*
Index: linux-2.6.9-rc1-bk8/fs/ext3/namei.c
===================================================================
--- linux-2.6.9-rc1-bk8.orig/fs/ext3/namei.c
+++ linux-2.6.9-rc1-bk8/fs/ext3/namei.c
@@ -2359,7 +2359,7 @@ struct inode_operations ext3_dir_inode_o
 	.getxattr	= ext3_getxattr,
 	.listxattr	= ext3_listxattr,
 	.removexattr	= ext3_removexattr,
-	.permission	= ext3_permission,
+	.check_posix_acl = ext3_check_posix_acl,
 };
 
 struct inode_operations ext3_special_inode_operations = {
@@ -2368,5 +2368,5 @@ struct inode_operations ext3_special_ino
 	.getxattr	= ext3_getxattr,
 	.listxattr	= ext3_listxattr,
 	.removexattr	= ext3_removexattr,
-	.permission	= ext3_permission,
+	.check_posix_acl = ext3_check_posix_acl,
 };