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: <CAGudoHGJKUkYkrG514YzghV4mBMKGW6N4njdQViBnKiLwoN4SQ@mail.gmail.com>
Date: Thu, 22 Aug 2024 23:31:39 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: Paul Moore <paul@...l-moore.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 10:58 PM Paul Moore <paul@...l-moore.com> wrote:
>
> 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.
>

ye I remember trouble getting any response in the past.

fwiw I expect to write one more patch (maybe split into two) to
distribute the refcount into task_struct, then modulo a bugfix should
someone find a problem hopefully I wont be submitting any more cred
stuff :)

-- 
Mateusz Guzik <mjguzik gmail.com>