Re: [PATCH] fs: call security_d_instantiate in d_obtain_alias

[Josef Bacik <josef@redhat.com>]

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,
> 

I'd like some input from the SELinux guys on this, talking with Bruce we can't
really figure out a good way to deal with this.  If we're using the parent
inode->i_mutex to guard the security initalization stuff we're screwed when it
comes to this case, since we have no way to know who the parent is at this point
in time.  Any suggestions?  Thanks,

Josef