Re: remove_suid bangs on xattrs

["Serge E. Hallyn" <serge.hallyn@canonical.com>]

Quoting Chris Mason (chris.mason@oracle.com):
> [ sorry, corrected cc list ]

Thanks - sorry for the inconvenience.  I'm also cc:ing Andrew Morgan
for another opinion.

> On Mon, Aug 16, 2010 at 03:38:12PM -0400, Chris Mason wrote:
> > Hi everyone,
> > 
> > I'm looking into a 2.6.35 btrfs performance regression, and perf tells
> > me that I'm spending a lot of time hammering on xattrs inside
> > remove_suid.  This is pretty surprising because I'm running as root, and
> > my files are not suid.  Looking back to this commit:
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=b53767719b6cd8789392ea3e7e2eb7b8906898f0
> > 
> > We've changed remove_suid's semantics from
> > 
> > if (file_is_suid)
> >     try to remove it

(but only if not capable(CAP_FSETID))

> > To something that always checks to see if we have removal permissions.

(not really - security_inode_need_killpriv() shoudl return <0 only if
there was an actual error, and the write needs to be cancelled altogether.
It returns 0 if privs don't need to be removed, and >0 if they do.

> > Was this intentional?  It didn't cause my 2.6.35 regression (that's all
> > my fault) but it does look wrong to me:

If I'm thinking right, I think the key change we should make is to have
CAP_FSETID be honored for maintaining file capabilities.

That would have two (good) results:

1. we should be able to re-arrange the code to check for CAP_FSETID
before bothering to check for file capabilities, so we can save the
getxattrs which I assume were what you were finding?  Even if it
wasn't the cause of your performance regression, it should be an
improvement.

2. I think it can be seen as a semantic fix.  We mostly try to
respect suid behavior for file caps, so it will be more consistent
to honor CAP_FSETID for file capabilities.

Andrew, what do you think?

> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 4fb1546..79f24a9 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -1627,12 +1627,18 @@ int __remove_suid(struct dentry *dentry, int kill)
> >  
> >  int remove_suid(struct dentry *dentry)
> >  {
> > -       int kill = should_remove_suid(dentry);
> > +       int killsuid = should_remove_suid(dentry);
> > +       int killpriv = security_inode_need_killpriv(dentry);
> > +       int error = 0;
> >  
> > -       if (unlikely(kill))
> > -               return __remove_suid(dentry, kill);
> > +       if (killpriv < 0)
> > +               return killpriv;
> > +       if (killpriv)
> > +               error = security_inode_killpriv(dentry);
> > +       if (!error && killsuid)
> > +               error = __remove_suid(dentry, killsuid);
> >  
> > -       return 0;
> > +       return error;
> >  }
> >  EXPORT_SYMBOL(remove_suid);
> > 
> > -chris

thanks,
-serge