linux-kernel - Re: [PATCH v10 09/12] x86/mm: enable broadcast TLB invalidation for multi-threaded processes

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: <CA+i-1C3-F-VAy_JccwKx_AcD1mXsVcGHGwUFvi_ruAiqusiXZQ@mail.gmail.com>
Date: Wed, 12 Feb 2025 14:28:49 +0100
From: Brendan Jackman <jackmanb@...gle.com>
To: Rik van Riel <riel@...riel.com>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org, bp@...en8.de, 
	peterz@...radead.org, dave.hansen@...ux.intel.com, zhengqi.arch@...edance.com, 
	nadav.amit@...il.com, thomas.lendacky@....com, kernel-team@...a.com, 
	linux-mm@...ck.org, akpm@...ux-foundation.org, jannh@...gle.com, 
	mhklinux@...look.com, andrew.cooper3@...rix.com, 
	Manali Shukla <Manali.Shukla@....com>
Subject: Re: [PATCH v10 09/12] x86/mm: enable broadcast TLB invalidation for
 multi-threaded processes

On Tue, 11 Feb 2025 at 22:09, Rik van Riel <riel@...riel.com> wrote:
> +       /* Transition from global->local ASID does not currently happen. */
> +       if (!global_asid && is_global_asid(prev_asid))
> +               return true;

What about a WARN_ON_ONCE? Then the code contains evidence that the
comment is true. IIUC this isn't just a "not implemented feature",
rather there is plenty of other code that would need to be updated to
make it safe.

> +static void use_global_asid(struct mm_struct *mm)
> +{
> +       u16 asid;
> +
> +       guard(raw_spinlock_irqsave)(&global_asid_lock);
> +
> +       /* This process is already using broadcast TLB invalidation. */
> +       if (READ_ONCE(mm->context.global_asid))
> +               return;
> +
> +       /* The last global ASID was consumed while waiting for the lock. */
> +       if (!READ_ONCE(global_asid_available)) {

I think the READ_ONCE is unnecessary while we have the spinlock, it's
fine if this read gets split or whatever?

Ditto for mm->context.global_asid, it's only modified with the lock held.