linux-kernel - Re: [RFC PATCH 7/9] 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: <03E5F4D7-3E3F-4809-87FE-6BD0B792E90F@gmail.com>
Date: Tue, 6 May 2025 09:59:52 +0300
From: Nadav Amit <nadav.amit@...il.com>
To: Rik van Riel <riel@...riel.com>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
 "open list:MEMORY MANAGEMENT" <linux-mm@...ck.org>,
 the arch/x86 maintainers <x86@...nel.org>,
 kernel-team@...a.com,
 Dave Hansen <dave.hansen@...ux.intel.com>,
 luto@...nel.org,
 peterz@...radead.org,
 Thomas Gleixner <tglx@...utronix.de>,
 Ingo Molnar <mingo@...hat.com>,
 Borislav Petkov <bp@...en8.de>,
 "H. Peter Anvin" <hpa@...or.com>,
 Yu-cheng Yu <yu-cheng.yu@...el.com>
Subject: Re: [RFC PATCH 7/9] x86/mm: Introduce Remote Action Request

> On 6 May 2025, at 3:37, Rik van Riel <riel@...riel.com> wrote:
> 
> +void smp_call_rar_many(const struct cpumask *mask, u16 pcid,
> +		       unsigned long start, unsigned long end)
> +{
> +	unsigned long pages = (end - start + PAGE_SIZE) / PAGE_SIZE;
> +	int cpu, next_cpu, this_cpu = smp_processor_id();
> +	cpumask_t *dest_mask;
> +	unsigned long idx;
> +
> +	if (pages > RAR_INVLPG_MAX_PAGES || end == TLB_FLUSH_ALL)
> +		pages = RAR_INVLPG_MAX_PAGES;
> +
> +	/*
> +	 * Can deadlock when called with interrupts disabled.
> +	 * We allow cpu's that are not yet online though, as no one else can
> +	 * send smp call function interrupt to this cpu and as such deadlocks
> +	 * can't happen.
> +	 */
> +	WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
> +		     && !oops_in_progress && !early_boot_irqs_disabled);

To ease it for the reader, consider using the updated version from smp.c
(or - even better - refactor into common inline function):

	if (cpu_online(this_cpu) && !oops_in_progress &&
	    !early_boot_irqs_disabled)
		lockdep_assert_irqs_enabled();

> +
> +	/* Try to fastpath.  So, what's a CPU they want?  Ignoring this one. */
> +	cpu = cpumask_first_and(mask, cpu_online_mask);
> +	if (cpu == this_cpu)
> +		cpu = cpumask_next_and(cpu, mask, cpu_online_mask);
> +
> +	/* No online cpus?  We're done. */
> +	if (cpu >= nr_cpu_ids)
> +		return;
> +
> +	/* Do we have another CPU which isn't us? */
> +	next_cpu = cpumask_next_and(cpu, mask, cpu_online_mask);
> +	if (next_cpu == this_cpu)
> +		next_cpu = cpumask_next_and(next_cpu, mask, cpu_online_mask);
> +
> +	/* Fastpath: do that cpu by itself. */

If you follow my comment (suggestion) about the concurrent flushes, then 
this part should be moved to be in the same was as done in the updated
smp_call_function_many_cond().

IOW, the main difference between this path and the “slow path” is 
arch_send_rar_ipi_mask() vs arch_send_rar_single_ipi() (and maybe
“and” with cpu_online_mask).

> +	if (next_cpu >= nr_cpu_ids) {
> +		lock(this_cpu_ptr(&rar_lock));
> +		idx = get_payload();
> +		set_payload(idx, pcid, start, pages);
> +		set_action_entry(idx, cpu);
> +		arch_send_rar_single_ipi(cpu);
> +		wait_for_done(idx, cpu);
> +		free_payload(idx);
> +		unlock(this_cpu_ptr(&rar_lock));
> +		return;
> +	}
> +
> +	dest_mask = this_cpu_ptr(&rar_cpu_mask);
> +	cpumask_and(dest_mask, mask, cpu_online_mask);
> +	cpumask_clear_cpu(this_cpu, dest_mask);
> +
> +	/* Some callers race with other cpus changing the passed mask */
> +	if (unlikely(!cpumask_weight(dest_mask)))
> +		return;
> +
> +	lock(this_cpu_ptr(&rar_lock));
> +	idx = get_payload();
> +	set_payload(idx, pcid, start, pages);
> +
> +	for_each_cpu(cpu, dest_mask)
> +		set_action_entry(idx, cpu);
> +
> +	/* Send a message to all CPUs in the map */
> +	arch_send_rar_ipi_mask(dest_mask);

Since 2019 we have move into “multi” TLB flush instead of “many”.

This means that try to take advantage of the time between sending the IPI
and the indication it was completed to do the local TLB flush. For both
consistency and performance, I recommend you’d follow this approach and
do the local TLB flush (if needed) here, instead of doing it in the
caller. 

> +
> +	for_each_cpu(cpu, dest_mask)
> +		wait_for_done(idx, cpu);
> +
> +	free_payload(idx);
> +	unlock(this_cpu_ptr(&rar_lock));

We don’t do lock/unlock on kernel/smp.c . So I would expect at least a
comment as for why it is required.

> +}
> +EXPORT_SYMBOL(smp_call_rar_many);