linux-kernel - Re: [GIT PULL] overlayfs updates for 6.13

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-=wg_Hbtk1oeghodpDMc5Pq24x=aaihBHedfubyCXbntEMw@mail.gmail.com>
Date: Fri, 22 Nov 2024 21:21:58 -0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Amir Goldstein <amir73il@...il.com>
Cc: Miklos Szeredi <miklos@...redi.hu>, Christian Brauner <brauner@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, linux-unionfs@...r.kernel.org
Subject: Re: [GIT PULL] overlayfs updates for 6.13

On Fri, 22 Nov 2024 at 01:57, Amir Goldstein <amir73il@...il.com> wrote:
>
> - Introduction and use of revert/override_creds_light() helpers, that were
>   suggested by Christian as a mitigation to cache line bouncing and false
>   sharing of fields in overlayfs creator_cred long lived struct cred copy.

So I don't actively hate this, but I do wonder if this shouldn't have
been done differently.

In particular, I suspect *most* users of override_creds() actually
wants this "light" version, because they all already hold a ref to the
cred that they want to use as the override.

We did it that safe way with the extra refcount not because most
people would need it, but it was expected to not be a big deal.

Now you found that it *is* a big deal, and instead of just fixing the
old interface, you create a whole new interface and the mental burden
of having to know the difference between the two.

So may I ask that you look at perhaps just converting the (not very
many) users of the non-light cred override to the "light" version?

Because I suspect you will find that they basically *all* convert. I
wouldn't be surprised if some of them could convert automatically with
a coccinelle script.

Because we actually have several users that have a pattern line

        old_cred = override_creds(override_cred);

        /* override_cred() gets its own ref */
        put_cred(override_cred);

because it *didn't* want the new cred, because it's literally a
temporary cred that already had the single ref it needed, and the code
actually it wants it to go away when it does

        revert_creds(old_cred);

End result: I suspect what it *really* would have wanted is basically
to have 'override_creds()' not do the refcount at all, and at revert
time, it would want "revert_creds()" to return the creds that got
reverted, and then it would just do

        old_cred = override_creds(override_cred);
        ...
        put_cred(revert_creds(old_cred));

instead - which would not change the refcount on 'old_cred' at all at
any time (and does it for the override case only at the end when it
actually wants it free'd).

And the above is very annoyingly *almost* exactly what your "light"
interface does, except your interface is bad too: it doesn't return
the reverted creds.

So then users have to remember the override_creds *and* the old creds,
just to do their own cred refcounting outside of this all.

In other words, what I really dislike about this all is that

 (a) we had a flawed interface

 (b) you added *another* flawed interface for one special case you cared about

 (c) now we have *two* flawed interfaces instead of one better one

Hmm?

                  Linus