From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Serge E. Hallyn" Subject: Re: remove_suid bangs on xattrs Date: Fri, 20 Aug 2010 07:25:15 -0500 Message-ID: <20100820122515.GA4704@hallyn.com> References: <20100816193812.GF993@think> <20100816194439.GG993@think> <20100818024139.GA16578@hallyn.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: "Serge E. Hallyn" , Chris Mason , linux-fsdevel@vger.kernel.org To: "Andrew G. Morgan" Return-path: Received: from adelie.canonical.com ([91.189.90.139]:34530 "EHLO adelie.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750857Ab0HTMZX convert rfc822-to-8bit (ORCPT ); Fri, 20 Aug 2010 08:25:23 -0400 Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Quoting Andrew G. Morgan (morgan@kernel.org): > On Tue, Aug 17, 2010 at 7:41 PM, Serge E. Hallyn > wrote: > > Quoting Chris Mason (chris.mason@oracle.com): > >> [ sorry, corrected cc list ] > > > > Thanks - sorry for the inconvenience. =A0I'm also cc:ing Andrew Mor= gan > > 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. =A0This is pretty surprising because I'm running as= root, and > >> > my files are not suid. =A0Looking back to this commit: > >> > http://git.kernel.org/?p=3Dlinux/kernel/git/torvalds/linux-2.6.g= it;a=3Dcommit;h=3Db53767719b6cd8789392ea3e7e2eb7b8906898f0 > >> > > >> > We've changed remove_suid's semantics from > >> > > >> > if (file_is_suid) > >> > =A0 =A0 try to remove it > > > > (but only if not capable(CAP_FSETID)) >=20 > I disagree. I think the relevant capability test should be with > respect to CAP_SETFCAP. >=20 > 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. I'm ok with that. > >> > To something that always checks to see if we have removal permis= sions. > > > > (not really - security_inode_need_killpriv() shoudl return <0 only = if > > there was an actual error, and the write needs to be cancelled alto= gether. > > It returns 0 if privs don't need to be removed, and >0 if they do. > > > >> > Was this intentional? =A0It didn't cause my 2.6.35 regression (t= hat'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? =A0Even 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. =A0We 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? > > >=20 > I think the test should be with respect to CAP_SETFCAP, but I agree > with the rest of your comments. I also point out, with some shame, that on first reading Chris' email, I had to look at that code more than I should have needed to to recall the details. So I think the function could stand some cleaning up/ simplifying. I'll try to take a look at this next week. > 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. That could also be shared with a new (old) optional xattr-free file-backed-filecaps mount option :) thanks, -serge -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel= " in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html