linux-kernel - Re: [PATCH v10 6/7] apei/ghes: Use xchg_release() for updating new cache slot instead of cmpxchg()

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: <Y1OtRpLRwPPG/4Il@zn.tnic>
Date:   Sat, 22 Oct 2022 10:43:50 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Jia He <justin.he@....com>
Cc:     Ard Biesheuvel <ardb@...nel.org>, Len Brown <lenb@...nel.org>,
        Tony Luck <tony.luck@...el.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Robert Richter <rric@...nel.org>,
        Robert Moore <robert.moore@...el.com>,
        Qiuxu Zhuo <qiuxu.zhuo@...el.com>,
        Yazen Ghannam <yazen.ghannam@....com>,
        Jan Luebbe <jlu@...gutronix.de>,
        Khuong Dinh <khuong@...amperecomputing.com>,
        Kani Toshi <toshi.kani@....com>,
        James Morse <james.morse@....com>, linux-acpi@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-edac@...r.kernel.org,
        devel@...ica.org, "Rafael J . Wysocki" <rafael@...nel.org>,
        Shuai Xue <xueshuai@...ux.alibaba.com>,
        Jarkko Sakkinen <jarkko@...nel.org>, linux-efi@...r.kernel.org,
        nd@....com, Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v10 6/7] apei/ghes: Use xchg_release() for updating new
 cache slot instead of cmpxchg()

On Tue, Oct 18, 2022 at 08:22:13AM +0000, Jia He wrote:
> From: Ard Biesheuvel <ardb@...nel.org>
> 
> From: Ard Biesheuvel <ardb@...nel.org>
> 
> ghes_estatus_cache_add() selects a slot, and either succeeds in
> replacing its contents with a pointer to a new cached item, or it just
> gives up and frees the new item again, without attempting to select
> another slot even if one might be available.
> 
> Since only inserting new items is needed, the race can only cause a failure
> if the selected slot was updated with another new item concurrently,
> which means that it is arbitrary which of those two items gets
> dropped. This means the cmpxchg() and the special case are not necessary,

Hmm, are you sure about this?

Looking at this complex code, I *think* the intent of the cache is to
collect already reported errors - the ghes_estatus_cached() checks - and
the adding happens when you report a new one:

        if (!ghes_estatus_cached(estatus)) {
                if (ghes_print_estatus(NULL, ghes->generic, estatus))
                        ghes_estatus_cache_add(ghes->generic, estatus);

Now, the loop in ghes_estatus_cache_add() is trying to pick out the,
well, oldest element in there. Meaning, something which got reported
already but a long while ago. There's even a sentence trying to say what
this does:

/*
 * GHES error status reporting throttle, to report more kinds of
 * errors, instead of just most frequently occurred errors.
 */

And the cmpxchg() is there to make sure when that selected element
slot_cache is removed, it really *is* that element that gets removed and
not one which replaced it in the meantime.

So it is likely I'm missing something here but it sure looks like this
is some sort of a complex, lockless, LRU scheme...

Hmmm.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette