Re: Observed unexpected behavior of BTRFS in d_instantiate
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. > There's no reason we can't set i_op sooner in btrfs, I'll patch this in. -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
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
Re: Observed unexpected behavior of BTRFS in d_instantiate
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. -- Stephen Smalley National Security Agency -- 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 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. -- Stephen Smalley National Security Agency -- 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 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)? reproducer? -- Stephen Smalley National Security Agency -- 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
