linux-kernel - Re: acl_permission_check: disgusting performance

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 12 May 2011 21:50:13 -0500
From:	"Serge E. Hallyn" <serge@...lyn.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	"Serge E. Hallyn" <serge.hallyn@...onical.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Daniel Lezcano <daniel.lezcano@...e.fr>,
	David Howells <dhowells@...hat.com>,
	James Morris <jmorris@...ei.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	containers@...ts.linux-foundation.org,
	Al Viro <viro@...iv.linux.org.uk>
Subject: Re: acl_permission_check: disgusting performance

Quoting Linus Torvalds (torvalds@...ux-foundation.org):
> Those four instructions are about two thirds of the cost of the
> function. The last two are about 50% of the cost.
> 
> They are the accesses to "current", "->cred", "->user" and "->user_ns"
> respectively (the cmp with the big constant is that compare against
> "init_ns").
> 
> Now, if we got rid of them, we wouldn't improve performance by 2/3rds
> on that function, because we do need the two first accesses for
> "fsuid" (which is the next check), and the third one (which is
> currently "cred->user" ends up doing the cache miss that we'd take for
> "cred->fsuid" anyway. So the first three costs are fairly inescapable.
> 
> They are also cheaper, probably because those fields tend to be more
> often in the cache. So it really is that fourth one that hurts the
> most, as shown by it taking almost a third of the cycles of that
> function.
> 
> And it all comes from that annoying commit e795b71799ff0 ("userns:
> userns: check user namespace for task->file uid equivalence checks"),
> and I bet nobody involved thought about how expensive that was.
> 
> That "user_ns" is _really_ expensive to load. And the fact that it's
> after a chain of three other loads makes it all totally serialized,
> and makes things much more expensive.
> 
> Could we perhaps have "user_ns" directly in the "struct cred"? Or

The only reason not to put it into struct cred would be to avoid growing
the struct cred.  For that matter, esp since you can't unshare the user_ns,
it could also go right into the task_struct.

(Eric's sys_setns patchset will eventually complicate that, but I don't
think it'll be a problem)

> could we avoid or short-circuit this check entirely somehow, since it
> always checks against "init_ns"?

Of course I'm hoping that before fall the check won't be against
init_ns any more :)  I was actually hoping to get back to that next
week, so I can start by testing the caching you suggest.

thanks,
-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/