On Tue, Aug 17, 2010 at 7:41 PM, Serge E. Hallyn <serge.hallyn@xxxxxxxxxxxxx> wrote: > Quoting Chris Mason (chris.mason@xxxxxxxxxx): >> [ 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)) I disagree. I think the relevant capability test should be with respect to CAP_SETFCAP. Since this is the capability that allows you to put a capability on a file, it should be the one to retain it if the file is modified. >> > 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? > I think the test should be with respect to CAP_SETFCAP, but I agree with the rest of your comments. Lots of small writes to 'any' file also tends to bang on this code. I've been wondering if it might make sense to cache, in the inode, that a file does *not* have any capabilities associated with it. That way the kernel wouldn't need to look up the xattrs twice for the same incapable file - which is, by far, the common case. Cheers Andrew >> > 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
