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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAGudoHGBuSL2p_mWOcaY+T9TV_2zRrPjk+E3BFJu6o4iHcBL4w@mail.gmail.com>
Date: Fri, 23 Aug 2024 17:42:11 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: paul@...l-moore.com, linux-security-module@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [RESEND PATCH] cred: separate the refcount from frequently read fields

On Fri, Aug 23, 2024 at 5:26 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Fri, 23 Aug 2024 at 20:33, Mateusz Guzik <mjguzik@...il.com> wrote:
> >
> > On Fri, Aug 23, 2024 at 2:06 AM Linus Torvalds
> > <torvalds@...ux-foundation.org> wrote:
> > >
> > >
> > > Yes, it is rarely actually written to and as such can be "mostly
> > > read-only", but since it is both read and written next to refcounts,
> > > why do that?
> > >
> > > Did I miss some common use?
> > >
> >
> > It gets looked at every time you grab a ref.
>
> Mateusz - read my email. That's what I daid.
>
> But the *ref* is already in cacheline 0. With your change it looked like this:
>
>    struct cred {
>         atomic_long_t   usage;
>         struct rcu_head rcu;            /* RCU deletion hook */
>
> and if you had kept the union with that 'struct rcu_head', then
> 'non_rcu' would be RIGHT THERE.
>

Huh, it's been a while since I wrote this patch. the rcu thing used to
be at the end I forgot I moved *that* var as well (apart from just
keys stuff), but I still support non_rcu being elsewhere (see below)

> > Thus consumers which grab the ref and then look at the most commonly
> > used fields get the non_rcu + rest combo "for free".
>
> They'd get it for free JUST BECAUSE IT'S NEXT TO THE REF. In cacheline
> 0 - that is already dirtied by the reference count. Which makes a
> *store* cheaper.
>

The store to the non_rcu var happens at most once.

Suppose multiple threads open and close file objs, this results in a
stream of atomics on ->usage. With the proposed layout only accesses
there are rmw within the atomic.

If non_rcu hangs around in the same cacheline there is additional
ping-pong to check the field.

> And you also wouldn't waste separate space for it.
>

I agree space is getting wasted (or rather a hole is there when it
does not have to be), I just don't think in the current layout this
matters.

I am not going to die on this hill though, if you insist on non_rcu
going back to the union I send a v2, but as is this would come with
*some* loss.
-- 
Mateusz Guzik <mjguzik gmail.com>