Re: [PATCH] fs: call security_d_instantiate in d_obtain_alias

[Eric Paris <eparis@redhat.com>]

On Wed, 2010-11-17 at 15:26 -0500, J. Bruce Fields wrote:
> On Wed, Nov 17, 2010 at 02:28:22PM -0500, Josef Bacik wrote:
> > On Wed, Nov 17, 2010 at 02:18:17PM -0500, J. Bruce Fields wrote:
> > > On Wed, Nov 17, 2010 at 12:51:03PM -0500, Josef Bacik wrote:
> > > > While trying to track down some NFS problems with BTRFS, I kept noticing I was
> > > > getting -EACCESS for no apparent reason.  Eric Paris and printk() helped me
> > > > figure out that it was SELinux that was giving me grief, with the following
> > > > denial
> > > > 
> > > > type=AVC msg=audit(1290013638.413:95): avc:  denied  { 0x800000 } for  pid=1772
> > > > comm="nfsd" name="" dev=sda1 ino=256 scontext=system_u:system_r:kernel_t:s0
> > > > tcontext=system_u:object_r:unlabeled_t:s0 tclass=file
> > > > 
> > > > Turns out this is because in d_obtain_alias if we can't find an alias we create
> > > > one and do all the normal instantiation stuff, but we don't do the
> > > > security_d_instantiate.  With this patch I'm no longer seeing these errant
> > > > -EACCESS return values.  Thanks,
> > > 
> > > Possibly dumb question: Is there still a small race here?  Is it
> > > possible for another nfsd thread to find the new alias on the list while
> > > this thread is still:
> > > 
> > > > 
> > > > Signed-off-by: Josef Bacik <josef@redhat.com>
> > > > ---
> > > >  fs/dcache.c |    1 +
> > > >  1 files changed, 1 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/fs/dcache.c b/fs/dcache.c
> > > > index 23702a9..890a59e 100644
> > > > --- a/fs/dcache.c
> > > > +++ b/fs/dcache.c
> > > > @@ -1201,6 +1201,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
> > > >  	spin_unlock(&tmp->d_lock);
> > > >  
> > > >  	spin_unlock(&dcache_lock);
> > > 
> > > ... right here, so that that other nfsd thread still ends up trying to
> > > do something with a dentry that hasn't had security_d_instantiate called
> > > on it yet?
> > > 
> > > > +	security_d_instantiate(tmp, inode);
> > > >  	return tmp;
> > > >  
> > > >   out_iput:
> > > > -- 
> > > 
> > > Or does something else prevent that?
> > > 
> > 
> > That's a good question, I have no idea actually.  Every other consumer of
> > security_d_instantiate seems to hold the i_mutex of the parent directory inode,
> > tho I'm not sure if that is appropriate for d_obtain_alias, maybe somebody else
> > has an idea?  Thanks,
> 
> Actually, I don't get it:
> 
> 	- Why is selinux using a *dentry* operation to initialize an
> 	  *inode*?
> 	- Are security hooks necessarily prepared to handle a
> 	  disconnected dentry?  (Which has no real parent, name an empty
> 	  string, etc.)
> 	- What use is the dentry to the security module in this case
> 	  anyway?

I only know a bit from the SELinux world and can't speak at all for any
other LSMs.  SELinux however needs the dentry when an inode first enters
core for a couple of reasons.  (once the inode is in core it should have
already been initialized and we skip all this)

If you have persistent xattr support we need the dentry since the xattr
code requires a dentry.  I have no idea why but that's what
inode->i_op->getxattr() requires.

Then we come to procfs.  In that filesystem we actually label based on
the pathname.  oh no did I say SELinux uses pathnames?  yes, I did, but
we only use the part of the pathname relative to the procfs root, so
really it's a static identifier which is immutable.

>From what I can see SELinux doesn't really care about the dentry, we
don't care about IS_ROOT() or the name or any of that crap (in the
non-procfs case).  All we really about is that if something can find an
inode and use it that security_d_instantiate() was called....

Calling security_d_instantiate() extra times is very low overhead and
not harmful.  the dirtiest (but easiest I guess) fix would be to add it
into the out_iput path as well.  I feel like there has to be an easier
solution, I'm just not sure what it is...

-Eric