Re: [PATCH RFC v2 04/29] mm: asi: Add infrastructure for boot-time enablement
On Fri, Jan 10, 2025 at 06:40:30PM +, 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...
> asi_check_boottime_disable() is modeled after
> pti_check_boottime_disable().
>
> The boot parameter is currently ignored until ASI is fully functional.
>
> Once we have a set of ASI features checked in that we have actually
> tested, we will stop ignoring the flag. But for now let's just add the
> infrastructure so we can implement the usage code.
>
> Ignoring checkpatch.pl CONFIG_DESCRIPTION because the _DEFAULT_ON
> Kconfig is trivial to explain.
Those last two paragraphs go...
> Checkpatch-args: --ignore CONFIG_DESCRIPTION
> Co-developed-by: Junaid Shahid
> Signed-off-by: Junaid Shahid
> Co-developed-by: Yosry Ahmed
> Signed-off-by: Yosry Ahmed
> Signed-off-by: Brendan Jackman
> ---
... here as that's text not really pertaining to the contents of the patch.
> arch/x86/Kconfig | 9 +
> arch/x86/include/asm/asi.h | 19 --
> arch/x86/include/asm/cpufeatures.h | 1 +
> arch/x86/include/asm/disabled-features.h | 8 -
> arch/x86/mm/asi.c| 61
> +++-
> arch/x86/mm/init.c | 4 ++-
> include/asm-generic/asi.h| 4 +++
> 7 files changed, 92 insertions(+), 14 deletions(-)
...
> * the N ASI classes.
> */
>
> +#define static_asi_enabled() cpu_feature_enabled(X86_FEATURE_ASI)
Yeah, as already mentioned somewhere else, whack that thing pls.
> +
> /*
> * ASI uses a per-CPU tainting model to track what mitigation actions are
> * required on domain transitions. Taints exist along two dimensions:
> @@ -131,6 +134,8 @@ struct asi {
>
> DECLARE_PER_CPU_ALIGNED(struct asi *, curr_asi);
>
> +void asi_check_boottime_disable(void);
> +
> void asi_init_mm_state(struct mm_struct *mm);
>
> int asi_init_class(enum asi_class_id class_id, struct asi_taint_policy
> *taint_policy);
> @@ -155,7 +160,9 @@ void asi_exit(void);
> /* The target is the domain we'll enter when returning to process context. */
> static __always_inline struct asi *asi_get_target(struct task_struct *p)
> {
> - return p->thread.asi_state.target;
> + return static_asi_enabled()
> +? p->thread.asi_state.target
> +: NULL;
Waaay too fancy for old people:
if ()
return...
else
return NULL;
:-)
The others too pls.
> static __always_inline void asi_set_target(struct task_struct *p,
> @@ -166,7 +173,9 @@ static __always_inline void asi_set_target(struct
> task_struct *p,
>
> static __always_inline struct asi *asi_get_current(void)
> {
> - return this_cpu_read(curr_asi);
> + return static_asi_enabled()
> +? this_cpu_read(curr_asi)
> +: NULL;
> }
>
> /* Are we currently in a restricted address space? */
> @@ -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?
> + *
> + * When ASI is disabled, this returns true.
> + */
> static __always_inline bool asi_is_relaxed(void)
> {
> return !asi_get_target(current);
> diff --git a/arch/x86/include/asm/cpufeatures.h
> b/arch/x86/include/asm/cpufeatures.h
> index
> 913fd3a7bac6506141de65f33b9ee61c615c7d7d..d6a808d10c3b8900d190ea01c66fc248863f05e2
> 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -474,6 +474,7 @@
> #define X86_FEATURE_CLEAR_BHB_HW (21*32+ 3) /* BHI_DIS_S HW control
> enabled */
> #define X86_FEATURE_CLEAR_BHB_LOOP_ON_VMEXIT (21*32+ 4) /* Clear branch
> history at vmexit using SW loop */
> #define X86_FEATURE_FAST_CPPC(21*32 + 5) /* AMD Fast CPPC */
> +#define X86_FEATURE_ASI (21*32+6) /* Kernel Address
> Space Isolation */
>
> /*
> * BUG word(s)
> diff --git a/arch/x86/include/asm/disabled-features.h
> b/arch/x86/include/asm/disabled-features.h
> index
> c492bdc97b0595ec77f89dc9b0cefe5e3e64be41..c7964ed4fef8b9441e1c0453da587787d8008d9d
> 100644
> --- a/arch/x86/include/asm/disabled-features.h
> +++ b/arch/x86/include/asm/disabled-features.h
> @@ -50,6 +50,12 @@
> # define DISABLE_PTI (1 << (X86_FEATURE_PTI & 31))
> #endif
>
> +#ifdef CONFIG_MITIGATION_ADDRESS_SPACE_ISOLATION
> +# define DISABLE_ASI 0
> +#else
> +# define DISABLE_ASI (1 << (X86_FEATURE_ASI & 31))
> +#endif
> +
> #ifdef CONFIG_
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 +, 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
>
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
