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