[PATCH] BTRFS: Establish i_ops before calling d_instantiate
From: Casey Schaufler ca...@schaufler-ca.com
The Smack LSM hook for security_d_instantiate checks
the inode's i_op-getxattr value to determine if the
containing filesystem supports extended attributes.
The BTRFS filesystem sets the inode's i_op value only
after it has instantiated the inode. This results in
Smack incorrectly giving new BTRFS inodes attributes
from the filesystem defaults on the assumption that
values can't be stored on the filesystem. This patch
moves the assignment of inode operation vectors ahead
of the calls to d_instantiate, letting Smack know that
the filesystem supports extended attributes. There
should be no impact on the performance or behavior of
BTRFS.
Signed-off-by: Casey Schaufler ca...@schaufler-ca.com
---
fs/btrfs/inode.c | 31 ++-
1 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 2c984f7..df0efca 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4555,11 +4555,18 @@ static int btrfs_mknod(struct inode *dir, struct dentry
*dentry,
goto out_unlock;
}
+ /*
+* If the active LSM wants to access the inode during
+* d_instantiate it needs these. Smack checks to see
+* if the filesystem supports xattrs by looking at the
+* ops vector.
+*/
+ inode-i_op = btrfs_special_inode_operations;
+
err = btrfs_add_nondir(trans, dir, dentry, inode, 0, index);
if (err)
drop_inode = 1;
else {
- inode-i_op = btrfs_special_inode_operations;
init_special_inode(inode, inode-i_mode, rdev);
btrfs_update_inode(trans, root, inode);
}
@@ -4613,14 +4620,21 @@ static int btrfs_create(struct inode *dir, struct
dentry *dentry,
goto out_unlock;
}
+ /*
+* If the active LSM wants to access the inode during
+* d_instantiate it needs these. Smack checks to see
+* if the filesystem supports xattrs by looking at the
+* ops vector.
+*/
+ inode-i_fop = btrfs_file_operations;
+ inode-i_op = btrfs_file_inode_operations;
+
err = btrfs_add_nondir(trans, dir, dentry, inode, 0, index);
if (err)
drop_inode = 1;
else {
inode-i_mapping-a_ops = btrfs_aops;
inode-i_mapping-backing_dev_info = root-fs_info-bdi;
- inode-i_fop = btrfs_file_operations;
- inode-i_op = btrfs_file_inode_operations;
BTRFS_I(inode)-io_tree.ops = btrfs_extent_io_ops;
}
out_unlock:
@@ -7076,14 +7090,21 @@ static int btrfs_symlink(struct inode *dir, struct
dentry *dentry,
goto out_unlock;
}
+ /*
+* If the active LSM wants to access the inode during
+* d_instantiate it needs these. Smack checks to see
+* if the filesystem supports xattrs by looking at the
+* ops vector.
+*/
+ inode-i_fop = btrfs_file_operations;
+ inode-i_op = btrfs_file_inode_operations;
+
err = btrfs_add_nondir(trans, dir, dentry, inode, 0, index);
if (err)
drop_inode = 1;
else {
inode-i_mapping-a_ops = btrfs_aops;
inode-i_mapping-backing_dev_info = root-fs_info-bdi;
- inode-i_fop = btrfs_file_operations;
- inode-i_op = btrfs_file_inode_operations;
BTRFS_I(inode)-io_tree.ops = btrfs_extent_io_ops;
}
if (drop_inode)
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Observed unexpected behavior of BTRFS in d_instantiate
On 4/28/2011 6:30 AM, Stephen Smalley wrote: On Tue, 2011-04-26 at 20:15 -0700, Casey Schaufler wrote: I have been tracking down an problem that we've been seeing with Smack on top of btrfs and have narrowed it down to a check in smack_d_instantiate() that checks to see if the underlying filesystem supports extended attributes by looking at inode-i_op-getxattr If the filesystem has no entry for getxattr it is assumed that it does not support extended attributes. The Smack code clearly finds this value to be NULL for btrfs and uses a fallback value. Clearly something is amiss, as other code paths clearly find the i_op-getxattr function and use it to effect. The btrfs code quite obviously includes getxattr functions. So, what is btrfs up to such that the inode ops does not include getxattr when security_d_instantiate is called? I am led to understand that SELinux has worked around this, but looking at the SELinux code I expect that there is a problem there as well. Thank you. kernel version(s)? 2.6.37 2.6.39rc4 reproducer? The MeeGo team saw the behavior first. I have been instrumenting the Smack code to track down what is happening. I am in the process of developing a Smack workaround for the btrfs behavior. -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Observed unexpected behavior of BTRFS in d_instantiate
On 4/28/2011 10:27 AM, Chris Mason wrote: Excerpts from Stephen Smalley's message of 2011-04-28 13:23:59 -0400: On Thu, 2011-04-28 at 13:13 -0400, Stephen Smalley wrote: On Thu, 2011-04-28 at 10:03 -0700, Casey Schaufler wrote: On 4/28/2011 6:30 AM, Stephen Smalley wrote: On Tue, 2011-04-26 at 20:15 -0700, Casey Schaufler wrote: I have been tracking down an problem that we've been seeing with Smack on top of btrfs and have narrowed it down to a check in smack_d_instantiate() that checks to see if the underlying filesystem supports extended attributes by looking at inode-i_op-getxattr If the filesystem has no entry for getxattr it is assumed that it does not support extended attributes. The Smack code clearly finds this value to be NULL for btrfs and uses a fallback value. Clearly something is amiss, as other code paths clearly find the i_op-getxattr function and use it to effect. The btrfs code quite obviously includes getxattr functions. So, what is btrfs up to such that the inode ops does not include getxattr when security_d_instantiate is called? I am led to understand that SELinux has worked around this, but looking at the SELinux code I expect that there is a problem there as well. Thank you. kernel version(s)? 2.6.37 2.6.39rc4 reproducer? The MeeGo team saw the behavior first. I have been instrumenting the Smack code to track down what is happening. I am in the process of developing a Smack workaround for the btrfs behavior. If this is for newly created files, then we initialize the in-core security label for the inode as part of the inode_init_security hook in SELinux and thus don't even try to call -getxattr at d_instantiate time. Not sure though why it wouldn't already be set. Actually, a quick look at the code makes it clear. btrfs_create() and friends call d_instantiate() before setting inode-i_op() for new inodes. In contrast, ext[234] set the i_op before calling d_instantiate(). In any event, you don't really need to go through the slow path of calling -getxattr for new inodes as you already know the label that is being set. I prefer having a single code path that performs this critical bit of security functionality. There's no reason we can't set i_op sooner in btrfs, I'll patch this in. Thank you very much. I will be happy to test the patch. -chris -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Observed unexpected behavior of BTRFS in d_instantiate
I have been tracking down an problem that we've been seeing with Smack on top of btrfs and have narrowed it down to a check in smack_d_instantiate() that checks to see if the underlying filesystem supports extended attributes by looking at inode-i_op-getxattr If the filesystem has no entry for getxattr it is assumed that it does not support extended attributes. The Smack code clearly finds this value to be NULL for btrfs and uses a fallback value. Clearly something is amiss, as other code paths clearly find the i_op-getxattr function and use it to effect. The btrfs code quite obviously includes getxattr functions. So, what is btrfs up to such that the inode ops does not include getxattr when security_d_instantiate is called? I am led to understand that SELinux has worked around this, but looking at the SELinux code I expect that there is a problem there as well. Thank you. -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
