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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wh84ATUBUZG4DtoY-=Jo-WKwDcfNUdOGw0_PzEr85rLqw@mail.gmail.com>
Date: Fri, 23 Aug 2024 23:25:54 +0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Mateusz Guzik <mjguzik@...il.com>
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, 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.

> 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.

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

> consumers which already had a ref don't suffer any extra misses

Consumers that already had a ref don't touch 'non_rcu' at all as far
as I can see, so they have no reason to have it next to those "most
commonly used fields".

See my argument? You seem to have pointlessly separated out the
'non_rcu' from being together with the rcu_head, and thus wasted
potentially useful space in the structure.

Your own email even had that:

>        bool                       non_rcu;              /*   100     1 */
>
>        /* XXX 3 bytes hole, try to pack */

which would have been a /* 4 byte hole, try to pack */

A 4-byte hole is more useful than a 3-byte one. A 3-byte one is much
harder to use.

In fact, even without the union, I think your current cacheline 0 ends
up having a 3-byte hole due to that

        unsigned char   jit_keyring;    /* default keyring to attach requested

with CONFIG_KEYS.

Without CONFIG_KEYS, you have something like a 40-byte hole there due to the

    kuid_t uid ____cacheline_aligned_in_smp;

which seems very wasteful, but I guess CONFIG_KEYS is the common case.

So I repeat: what did I miss?

       Linus