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: <CAMj1kXFu36faTPoGSGPs9KhcKsoh_DE9X2rmwdenxaJwa3P_yw@mail.gmail.com>
Date:   Sat, 22 Oct 2022 11:01:01 +0200
From:   Ard Biesheuvel <ardb@...nel.org>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Jia He <justin.he@....com>, 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 Sat, 22 Oct 2022 at 10:44, Borislav Petkov <bp@...en8.de> wrote:
>
> 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...
>

You are correct.

But the point is that the new element we are adding has the same
properties as the one we want to avoid replacing inadvertently, and if
the cmpxchg() failed, we just drop it on the floor.

So instead of dropping 'our' new element, we now drop 'the other' new element.

The correct approach here would be to rerun the selection loop on
failure, but I doubt whether it is worth it. This is just a fancy rate
limiter.