Observed unexpected behavior of BTRFS in d_instantiate

DISCUSSION Casey Schaufler 3 participants 7 messages Apr 27, 2011 – Apr 28, 2011 Btrfs file system

Share & tools

Casey Schaufler

author

2011-04-27 03:15 UTC #0 lore raw

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.

Stephen Smalley

2011-04-28 13:30 UTC #1 lore raw

Quoted 15 lines · Casey Schaufler

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.

Quoted 6 lines

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)?
reproducer?

Signature · 2 lines

Stephen Smalley
National Security Agency

Casey Schaufler

author

2011-04-28 17:03 UTC #2 lore raw

Quoted 15 lines · Stephen Smalley

Quoted 14 lines · Casey Schaufler
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.

Quoted 7 lines

Quoted 6 lines
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

Quoted 1 line

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.

Stephen Smalley

2011-04-28 17:13 UTC #3 lore raw

Quoted 24 lines · Casey Schaufler

Quoted 15 lines · Stephen Smalley
Quoted 14 lines · Casey Schaufler
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.
Quoted 7 lines
Quoted 6 lines
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)?

Quoted 8 lines

2.6.37
2.6.39rc4
Quoted 1 line
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.

Signature · 2 lines

Stephen Smalley
National Security Agency

Stephen Smalley

2011-04-28 17:23 UTC #4 lore raw

Quoted 30 lines · Stephen Smalley

Quoted 29 lines · Casey Schaufler
Quoted 15 lines · Stephen Smalley
Quoted 14 lines · Casey Schaufler
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.
Quoted 7 lines
Quoted 6 lines
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
Quoted 1 line
reproducer?

Quoted 8 lines

Quoted 3 lines
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.

Signature · 2 lines

Stephen Smalley
National Security Agency

Chris Mason

2011-04-28 17:27 UTC #5 lore raw

Excerpts from Stephen Smalley's message of 2011-04-28 13:23:59 -0400:

Quoted 40 lines · Stephen Smalley

Quoted 30 lines · Casey Schaufler
Quoted 29 lines · Stephen Smalley
Quoted 15 lines · Casey Schaufler
Quoted 14 lines
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.
Quoted 7 lines
Quoted 6 lines
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
Quoted 1 line
reproducer?
Quoted 8 lines
Quoted 3 lines
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.

Quoted 9 lines

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.

There's no reason we can't set i_op sooner in btrfs, I'll patch this in.

-chris

Casey Schaufler

author

2011-04-28 17:52 UTC #6 lore raw

Quoted 45 lines · Chris Mason

Excerpts from Stephen Smalley's message of 2011-04-28 13:23:59 -0400:
Quoted 44 lines · Stephen Smalley
Quoted 27 lines · Casey Schaufler
Quoted 26 lines · Stephen Smalley
Quoted 15 lines · Casey Schaufler
Quoted 14 lines
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.
Quoted 7 lines
Quoted 6 lines
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
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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.

Quoted 1 line

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.

Quoted 1 line

-chris