Observed unexpected behavior of BTRFS in d_instantiate

Observed unexpected behavior of BTRFS in d_instantiate

[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.

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.

Re: Observed unexpected behavior of BTRFS in d_instantiate

[Stephen Smalley]

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

Re: Observed unexpected behavior of BTRFS in d_instantiate

[Casey Schaufler]

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.

Re: Observed unexpected behavior of BTRFS in d_instantiate

[Stephen Smalley]

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

Re: Observed unexpected behavior of BTRFS in d_instantiate

[Stephen Smalley]

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

Re: Observed unexpected behavior of BTRFS in d_instantiate

[Chris Mason]

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

Re: Observed unexpected behavior of BTRFS in d_instantiate

[Casey Schaufler]

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