Message-ID: <Z9sRQ0cK0rupEiT-@google.com>
Date: Wed, 19 Mar 2025 18:47:31 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Borislav Petkov <bp@...en8.de>
Cc: Brendan Jackman <jackmanb@...gle.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	"H. Peter Anvin" <hpa@...or.com>, Andy Lutomirski <luto@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Josh Poimboeuf <jpoimboe@...nel.org>,
	Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>, x86@...nel.org,
	linux-kernel@...r.kernel.org, linux-alpha@...r.kernel.org,
	linux-snps-arc@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org, linux-csky@...r.kernel.org,
	linux-hexagon@...r.kernel.org, loongarch@...ts.linux.dev,
	linux-m68k@...ts.linux-m68k.org, linux-mips@...r.kernel.org,
	linux-openrisc@...r.kernel.org, linux-parisc@...r.kernel.org,
	linuxppc-dev@...ts.ozlabs.org, linux-riscv@...ts.infradead.org,
	linux-s390@...r.kernel.org, linux-sh@...r.kernel.org,
	sparclinux@...r.kernel.org, linux-um@...ts.infradead.org,
	linux-arch@...r.kernel.org, linux-mm@...ck.org,
	linux-trace-kernel@...r.kernel.org,
	linux-perf-users@...r.kernel.org, kvm@...r.kernel.org,
	linux-efi@...r.kernel.org, Junaid Shahid <junaids@...gle.com>
Subject: Re: [PATCH RFC v2 04/29] mm: asi: Add infrastructure for boot-time
 enablement

On Wed, Mar 19, 2025 at 06:29:35PM +0100, Borislav Petkov wrote:
> On Fri, Jan 10, 2025 at 06:40:30PM +0000, Brendan Jackman wrote:
> > Add a boot time parameter to control the newly added X86_FEATURE_ASI.
> > "asi=on" or "asi=off" can be used in the kernel command line to enable
> > or disable ASI at boot time. If not specified, ASI enablement depends
> > on CONFIG_ADDRESS_SPACE_ISOLATION_DEFAULT_ON, which is off by default.
> 
> I don't know yet why we need this default-on thing...

It's a convenience to avoid needing to set asi=on if you want ASI to be
on by default. It's similar to HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON
or ZSWAP_DEFAULT_ON.

[..]
> > @@ -175,7 +184,11 @@ static __always_inline bool asi_is_restricted(void)
> >  	return (bool)asi_get_current();
> >  }
> >  
> > -/* If we exit/have exited, can we stay that way until the next asi_enter? */
> > +/*
> > + * If we exit/have exited, can we stay that way until the next asi_enter?
> 
> What is that supposed to mean here?

asi_is_relaxed() checks if the thread is outside an ASI critical
section.

I say "the thread" because it will also return true if we are executing
an interrupt that arrived during the critical section, even though the
interrupt handler is not technically part of the critical section.

Now the reason it says "if we exit we stay that way" is probably
referring to the fact that an asi_exit() when interrupting a critical
section will be undone in the interrupt epilogue by re-entering ASI.

I agree the wording here is confusing. We should probably describe this
more explicitly and probably rename the function after the API
discussions you had in the previous patch.

> 
> > + *
> > + * When ASI is disabled, this returns true.
> > + */
> >  static __always_inline bool asi_is_relaxed(void)
> >  {
> >  	return !asi_get_target(current);
[..]
> > @@ -66,10 +73,36 @@ const char *asi_class_name(enum asi_class_id class_id)
> >  	return asi_class_names[class_id];
> >  }
> >  
> > +void __init asi_check_boottime_disable(void)
> > +{
> > +	bool enabled = IS_ENABLED(CONFIG_MITIGATION_ADDRESS_SPACE_ISOLATION_DEFAULT_ON);
> > +	char arg[4];
> > +	int ret;
> > +
> > +	ret = cmdline_find_option(boot_command_line, "asi", arg, sizeof(arg));
> > +	if (ret == 3 && !strncmp(arg, "off", 3)) {
> > +		enabled = false;
> > +		pr_info("ASI disabled through kernel command line.\n");
> > +	} else if (ret == 2 && !strncmp(arg, "on", 2)) {
> > +		enabled = true;
> > +		pr_info("Ignoring asi=on param while ASI implementation is incomplete.\n");
> > +	} else {
> > +		pr_info("ASI %s by default.\n",
> > +			enabled ? "enabled" : "disabled");
> > +	}
> > +
> > +	if (enabled)
> > +		pr_info("ASI enablement ignored due to incomplete implementation.\n");
> 
> Incomplete how?

This is referring to the fact that ASI is still not fully/correctly
functional, but it will be after the following patches.

I think it will be clearer if we just add the feature flag here so that
we have something to check for in the following patches, but add the
infrastructure for boot-time enablement at the end of the series when
the impelemntation is complete.

Basically start by a feature flag that has no way of being enabled, use
it in the implmentation, then add means of enabling it.

> 
> > +}
> > +
> >  static void __asi_destroy(struct asi *asi)
> >  {
> > -	lockdep_assert_held(&asi->mm->asi_init_lock);
> > +	WARN_ON_ONCE(asi->ref_count <= 0);
> > +	if (--(asi->ref_count) > 0)
> 
> Switch that to
> 
> include/linux/kref.h
> 
> It gives you a sanity-checking functionality too so you don't need the WARN...

I think we hve internal changes that completely get rid of this
ref_count and simplifies the lifetime handling that we can squash here.
We basically keep ASI objects around until the process is torn down,
which makes this simpler and avoids the need for complex synchronization
when we try to context switch or run userspace without exiting ASI
(spoiler alert :) ).

> 
> > +		return;
> >  
> > +	free_pages((ulong)asi->pgd, PGD_ALLOCATION_ORDER);
> > +	memset(asi, 0, sizeof(struct asi));
> 
> And then you can do:
> 
> 	if (kref_put())
> 		free_pages...
> 
> and so on.
> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
> 

Powered by blists - more mailing lists