linux-kernel - Re: [RFC PATCH v3 5/7] x86/mm: Introduce Remote Action Request

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: <2666b165d262a3bad73a3a4d886fa6d16bced03c.camel@surriel.com>
Date: Thu, 05 Jun 2025 15:40:27 -0400
From: Rik van Riel <riel@...riel.com>
To: Nadav Amit <nadav.amit@...il.com>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, 
	kernel-team@...a.com, Dave Hansen <dave.hansen@...ux.intel.com>,
 luto@...nel.org, 	peterz@...radead.org, Borislav Petkov <bp@...en8.de>, the
 arch/x86 maintainers	 <x86@...nel.org>, Sean Christopherson
 <seanjc@...gle.com>, Thomas Gleixner	 <tglx@...utronix.de>, Yu-cheng Yu
 <yu-cheng.yu@...el.com>
Subject: Re: [RFC PATCH v3 5/7] x86/mm: Introduce Remote Action Request

On Thu, 2025-06-05 at 21:54 +0300, Nadav Amit wrote:
> Just few small things that jump out…
> 
> > On 5 Jun 2025, at 19:35, Rik van Riel <riel@...riel.com> wrote:
> > 
> > +void rar_cpu_init(void)
> > +{
> > +	u64 r;
> > +	u8 *bitmap;
> > +	int max_payloads;
> > +	int this_cpu = smp_processor_id();
> > +
> > +	cpumask_clear(&per_cpu(rar_cpu_mask, this_cpu));
> > +
> > +	/* The MSR contains N defining the max [0-N] rar payload
> > slots. */
> > +	rdmsrl(MSR_IA32_RAR_INFO, r);
> > +	max_payloads = (r >> 32) + 1;
> > +
> > +	/* If this CPU supports less than RAR_MAX_PAYLOADS, lower
> > our limit. */
> > +	if (max_payloads < rar_max_payloads)
> > +		rar_max_payloads = max_payloads;
> 
> Unless I am missing something, this looks very racy.
> 
All the CPUs in the system should support the same
number rar_max_payloads, since they share the same
rar_action table.

> BTW: should rar_max_payloads be ro_after_init?
> 
> > +	pr_info_once("RAR: support %d payloads\n", max_payloads);
> > +
> > +	bitmap = (u8 *)per_cpu(rar_action, this_cpu);
> 
> this_cpu_ptr() would be cleaner (here and when using rar_cpu_mask).

A CPU cannot start using the rar_action table until
the wrmsl below.

That should ensure there is no race here.

Thank you for the cleanup ideas. I'll apply those for v4.

> > +	/*
> > +	 * Allow RAR events to be processed while interrupts are
> > disabled on
> > +	 * a target CPU. This prevents "pileups" where many CPUs
> > are waiting
> > +	 * on one CPU that has IRQs blocked for too long, and
> > should reduce
> > +	 * contention on the rar_payload table.
> > +	 */
> > +	r = RAR_CTRL_ENABLE | RAR_CTRL_IGNORE_IF;
> 
> Do we really need r ?

I suppose not. The original code added another
value into r, but that hangs hard with today's
microcode :)

> 
> > +	wrmsrl(MSR_IA32_RAR_CTRL, r);
> > +}
> 
> 

-- 
All Rights Reversed.