Re: 2.6.10-rc2-mm4

[Jeffrey Mahoney]

On Tue, Nov 30, 2004 at 07:43:28PM +0000, Christoph Hellwig wrote:
> On Tue, Nov 30, 2004 at 11:29:03AM -0800, Chris Wright wrote:
> > My concerns are that the check has to be duplicated in any module,
> > and that thus far we've tried to keep out fs -> module communication,
> > letting vfs do it.  This could at least be fs -> vfs communication,
> > and then either vfs or security framework could check flags and never
> > call into module on fs private objects.
> 
> (1) an inode beeing private could have much more uses even outside LSM
> (2) it's an awfull lot of code where having a flag is really little code
> (3) there 's lots of room in the inode flags
> 
> I can't find anything that speaks for the messy current implementation

I took some more time to find a more optimal solution. Since ReiserFS is
currently the only filesystem that cares about this, it's far easier to keep
the whole mess internal to ReiserFS. The issue isn't about the treating of
"private" files in reiserfs, but rather just to avoid the looping of xattr
calls that selinux would create.

Here's a new patch that replaces the ones being debated.

Since I'm not that familiar with the inner workings of selinux, might someone
more familiar verify that the behavior won't break anything? I've done some
preliminary testing (creating a filesystem, creating a directory tree,
verifying that the appropriate security attributes were created)

-Jeff

-----------------------------------------------------------------------------

Since selinux will call xattr operations on any file accessed, it will attempt
to call into the xattr subsystem while accessing the xattr files. This causes
a locking loop which will lock the filesystem.

As part of the reiserfs xattr subsystem initialization process, this patch
copies the existing inode_operations structs and NULLs out the xattr
operations.

When selinux tries to access an internal file, it will see the ->{get,set}xattr
operation is NULL and assign a default SID rather than calling into the
filesystem again.

reiserfs_mark_inode_private is responsible for the assignment. This happens
as part of reiserfs_new_inode, as part of reiserfs_inherit_default_acl or
explicitly. As such the assignments of the inode_operations for various types
have been moved into reiserfs_new_inode, rather than the create,mknod,mkdir,etc
functions. The effective assignment order is unchanged.

Signed-off-by: Jeffrey Mahoney <jeffm@xxxxxxxx>

diff -ruNpX dontdiff linux-2.6.9/fs/reiserfs/inode.c linux-2.6.9.noxattr/fs/reiserfs/inode.c
--- linux-2.6.9/fs/reiserfs/inode.c	2004-11-19 14:40:53.000000000 -0500
+++ linux-2.6.9.noxattr/fs/reiserfs/inode.c	2004-12-01 17:31:51.000000000 -0500
@@ -1792,6 +1792,21 @@ int reiserfs_new_inode (struct reiserfs_
 	goto out_inserted_sd;
     }
 
+    if (S_ISREG (inode->i_mode)) {
+	inode->i_op = &reiserfs_file_inode_operations;
+	inode->i_fop = &reiserfs_file_operations;
+	inode->i_mapping->a_ops = &reiserfs_address_space_operations ;
+    } else if (S_ISDIR (inode->i_mode)) {
+	inode->i_op = &reiserfs_dir_inode_operations;
+	inode->i_fop = &reiserfs_dir_operations;
+    } else if (S_ISLNK (inode->i_mode)) {
+	inode->i_op = &reiserfs_symlink_inode_operations;
+	inode->i_mapping->a_ops = &reiserfs_address_space_operations;
+    } else {
+	inode->i_blocks = 0;
+	inode->i_op = &reiserfs_special_inode_operations;
+    }
+
     /* XXX CHECK THIS */
     if (reiserfs_posixacl (inode->i_sb)) {
         retval = reiserfs_inherit_default_acl (dir, dentry, inode);
@@ -1801,9 +1816,13 @@ int reiserfs_new_inode (struct reiserfs_
             journal_end(th, th->t_super, th->t_blocks_allocated);
             goto out_inserted_sd;
         }
-    } else if (inode->i_sb->s_flags & MS_POSIXACL) {
-	reiserfs_warning (inode->i_sb, "ACLs aren't enabled in the fs, "
-			  "but vfs thinks they are!");
+    } else {
+        if (inode->i_sb->s_flags & MS_POSIXACL) {
+	    reiserfs_warning (inode->i_sb, "ACLs aren't enabled in the fs, "
+			      "but vfs thinks they are!");
+        }
+	if (is_reiserfs_priv_object (dir))
+	    reiserfs_mark_inode_private (inode);
     }
 
     insert_inode_hash (inode);
diff -ruNpX dontdiff linux-2.6.9/fs/reiserfs/namei.c linux-2.6.9.noxattr/fs/reiserfs/namei.c
--- linux-2.6.9/fs/reiserfs/namei.c	2004-08-14 01:37:14.000000000 -0400
+++ linux-2.6.9.noxattr/fs/reiserfs/namei.c	2004-12-01 17:52:32.000000000 -0500
@@ -352,7 +352,7 @@ static struct dentry * reiserfs_lookup (
 
 	/* Propogate the priv_object flag so we know we're in the priv tree */
 	if (is_reiserfs_priv_object (dir))
-	    REISERFS_I(inode)->i_flags |= i_priv_object;
+	    reiserfs_mark_inode_private (inode);
     }
     reiserfs_write_unlock(dir->i_sb);
     if ( retval == IO_ERROR ) {
@@ -616,10 +616,6 @@ static int reiserfs_create (struct inode
         goto out_failed;
     }
 	
-    inode->i_op = &reiserfs_file_inode_operations;
-    inode->i_fop = &reiserfs_file_operations;
-    inode->i_mapping->a_ops = &reiserfs_address_space_operations ;
-
     retval = reiserfs_add_entry (&th, dir, dentry->d_name.name, dentry->d_name.len, 
 				inode, 1/*visible*/);
     if (retval) {
@@ -677,7 +673,6 @@ static int reiserfs_mknod (struct inode 
         goto out_failed;
     }
 
-    inode->i_op = &reiserfs_special_inode_operations;
     init_special_inode(inode, inode->i_mode, rdev) ;
 
     //FIXME: needed for block and char devices only
@@ -751,9 +746,6 @@ static int reiserfs_mkdir (struct inode 
     reiserfs_update_inode_transaction(inode) ;
     reiserfs_update_inode_transaction(dir) ;
 
-    inode->i_op = &reiserfs_dir_inode_operations;
-    inode->i_fop = &reiserfs_dir_operations;
-
     // note, _this_ add_entry will not update dir's stat data
     retval = reiserfs_add_entry (&th, dir, dentry->d_name.name, dentry->d_name.len, 
 				inode, 1/*visible*/);
@@ -1001,9 +993,6 @@ static int reiserfs_symlink (struct inod
     reiserfs_update_inode_transaction(inode) ;
     reiserfs_update_inode_transaction(parent_dir) ;
 
-    inode->i_op = &reiserfs_symlink_inode_operations;
-    inode->i_mapping->a_ops = &reiserfs_address_space_operations;
-
     // must be sure this inode is written with this transaction
     //
     //reiserfs_update_sd (&th, inode, READ_BLOCKS);
diff -ruNpX dontdiff linux-2.6.9/fs/reiserfs/xattr_acl.c linux-2.6.9.noxattr/fs/reiserfs/xattr_acl.c
--- linux-2.6.9/fs/reiserfs/xattr_acl.c	2004-11-19 14:40:53.000000000 -0500
+++ linux-2.6.9.noxattr/fs/reiserfs/xattr_acl.c	2004-12-01 17:03:55.000000000 -0500
@@ -337,7 +337,7 @@ reiserfs_inherit_default_acl (struct ino
      * would be useless since permissions are ignored, and a pain because
      * it introduces locking cycles */
     if (is_reiserfs_priv_object (dir)) {
-        REISERFS_I(inode)->i_flags |= i_priv_object;
+        reiserfs_mark_inode_private (inode);
         goto apply_umask;
     }
 
diff -ruNpX dontdiff linux-2.6.9/fs/reiserfs/xattr.c linux-2.6.9.noxattr/fs/reiserfs/xattr.c
--- linux-2.6.9/fs/reiserfs/xattr.c	2004-11-19 14:40:53.000000000 -0500
+++ linux-2.6.9.noxattr/fs/reiserfs/xattr.c	2004-12-01 17:11:35.000000000 -0500
@@ -181,8 +181,6 @@ open_xa_dir (const struct inode *inode, 
             dput (xadir);
             return ERR_PTR (-ENODATA);
         }
-        /* Newly created object.. Need to mark it private */
-        REISERFS_I(xadir->d_inode)->i_flags |= i_priv_object;
     }
 
     dput (xaroot);
@@ -230,8 +228,6 @@ get_xa_file_dentry (const struct inode *
             dput (xafile);
             goto out;
         }
-        /* Newly created object.. Need to mark it private */
-        REISERFS_I(xafile->d_inode)->i_flags |= i_priv_object;
     }
 
 out:
@@ -1261,6 +1257,32 @@ static struct dentry_operations xattr_lo
     .d_compare = xattr_lookup_poison,
 };
 
+struct inode_operations reiserfs_priv_file_inode_operations;
+struct inode_operations reiserfs_priv_dir_inode_operations;
+struct inode_operations reiserfs_priv_symlink_inode_operations;
+struct inode_operations reiserfs_priv_special_inode_operations;
+
+#define __init_priv_operation(op) 					\
+do {									\
+	memcpy (&reiserfs_priv_##op##_inode_operations,			\
+		&reiserfs_##op##_inode_operations,			\
+                sizeof (struct inode_operations));			\
+        reiserfs_priv_##op##_inode_operations.getxattr = NULL;		\
+        reiserfs_priv_##op##_inode_operations.setxattr = NULL;		\
+        reiserfs_priv_##op##_inode_operations.listxattr = NULL;		\
+        reiserfs_priv_##op##_inode_operations.removexattr = NULL;	\
+} while (0)
+
+static void
+init_priv_operations (void)
+{
+    __init_priv_operation(file);
+    __init_priv_operation(dir);
+
+    /* Technically these never get used, but no harm done */
+    __init_priv_operation(symlink);
+    __init_priv_operation(special);
+}
 
 /* We need to take a copy of the mount flags since things like
  * MS_RDONLY don't get set until *after* we're called.
@@ -1315,8 +1337,9 @@ reiserfs_xattr_init (struct super_block 
         err = PTR_ERR (dentry);
 
       if (!err && dentry) {
+          init_priv_operations();
           s->s_root->d_op = &xattr_lookup_poison_ops;
-          REISERFS_I(dentry->d_inode)->i_flags |= i_priv_object;
+          reiserfs_mark_inode_private (dentry->d_inode);
           REISERFS_SB(s)->priv_root = dentry;
       } else if (!(mount_flags & MS_RDONLY)) { /* xattrs are unavailable */
           /* If we're read-only it just means that the dir hasn't been
diff -ruNpX dontdiff linux-2.6.9/include/linux/reiserfs_xattr.h linux-2.6.9.noxattr/include/linux/reiserfs_xattr.h
--- linux-2.6.9/include/linux/reiserfs_xattr.h	2004-08-14 01:38:11.000000000 -0400
+++ linux-2.6.9.noxattr/include/linux/reiserfs_xattr.h	2004-12-01 16:37:06.000000000 -0500
@@ -103,9 +103,30 @@ reiserfs_read_unlock_xattr_i(struct inod
     up_read (&REISERFS_I(inode)->xattr_sem);
 }
 
+extern struct inode_operations reiserfs_priv_file_inode_operations;
+extern struct inode_operations reiserfs_priv_dir_inode_operations;
+extern struct inode_operations reiserfs_priv_symlink_inode_operations;
+extern struct inode_operations reiserfs_priv_special_inode_operations;
+
+static inline void
+reiserfs_mark_inode_private(struct inode *inode)
+{
+    if (S_ISREG (inode->i_mode))
+        inode->i_op = &reiserfs_priv_file_inode_operations;
+    else if (S_ISDIR (inode->i_mode))
+        inode->i_op = &reiserfs_priv_dir_inode_operations;
+    else if (S_ISLNK (inode->i_mode))
+        inode->i_op = &reiserfs_priv_symlink_inode_operations;
+    else
+        inode->i_op = &reiserfs_priv_special_inode_operations;
+
+    REISERFS_I(inode)->i_flags |= i_priv_object;
+}
+
 #else
 
 #define is_reiserfs_priv_object(inode) 0
+#define reiserfs_mark_inode_private(inode)
 #define reiserfs_getxattr NULL
 #define reiserfs_setxattr NULL
 #define reiserfs_listxattr NULL
-- 
Jeff Mahoney
SuSE Labs

Attachment: pgp00000.pgp
Description: PGP signature