linux-kernel - Re: [RESEND PATCH] cred: separate the refcount from frequently read fields

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAHC9VhRNMMtZ-_-sON_jz5_pkyFTyH5VOOVmPem=AbLpA5Y6dg@mail.gmail.com>
Date: Thu, 22 Aug 2024 16:58:45 -0400
From: Paul Moore <paul@...l-moore.com>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: torvalds@...ux-foundation.org, linux-security-module@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [RESEND PATCH] cred: separate the refcount from frequently read fields

On Thu, Aug 22, 2024 at 9:15 AM Mateusz Guzik <mjguzik@...il.com> wrote:
>
> The refcount shares the cacheline with uid, gid and other frequently
> read fields.
>
> Said counter gest modified a lot (for example every time a file is
> closed or opened) in turn causing avoidable bounces when another thread
> tries to do permission checks. Said bouncing can be avoided without
> growing the struct by reordering the fields -- keyring (enabled by
> default) is comparatively rarely used and can suffer bouncing instead.
>
> An additional store is performed to clear the non_rcu flag. Since the
> flag is rarely set (a special case in the access(2) system call) and
> transitions at most once, it can get placed in a read-mostly area and is
> only conditionally written to.
>
> With this in place regular permission checks no longer bounce cachelines
> in face of refcount changes.
>
> Validated with a simple test where one thread opens and closes a file
> (dirtying creds twice), while another keeps re-reading *another* file in
> a loop (ops/s):
> before: 4353763
> after:  4742792 (+9%)
>
> Signed-off-by: Mateusz Guzik <mjguzik@...il.com>
> ---
>
> This is a resend with a reworded commit message and added Linus since he
> wrote the non_rcu thing.
>
> Note each process has its on creds, so this is not causing bounces
> globally.
>
> Even so, there is stuff I plan to do in the area and this patch can be
> considered prep (only one store to non_rcu).
>
> I'll also note I don't see a way to *whack* non_rcu either. :)
>
> 0 rush
>
>  fs/open.c            |  2 +-
>  include/linux/cred.h | 31 +++++++++++++++----------------
>  kernel/cred.c        |  6 +++---
>  3 files changed, 19 insertions(+), 20 deletions(-)

[NOTE: no comment on the patch context yet, just process comments below]

FWIW, I really haven't commented on these last couple of cred patches
mostly because I've been assuming someone else would emerge from the
shadows as the cred maintainer, or at least someone who felt strongly
enough about these changes would merge them.  Unfortunately, I worry
that isn't really going to happen and I'd hate for some of the cred
improvements to die on the lists.

If no one starts grabbing your cred patches I can start taking cred
patches as part of the LSM tree, I've done it a couple of times in the
past and Linus didn't seem to mind.

-- 
paul-moore.com