Message-ID: <20250314131419.GJZ9Qrq8scAtDyBUcg@fat_crate.local>
Date: Fri, 14 Mar 2025 14:14:19 +0100
From: Borislav Petkov <bp@...en8.de>
To: Brendan Jackman <jackmanb@...gle.com>
Cc: akpm@...ux-foundation.org, dave.hansen@...ux.intel.com,
	yosryahmed@...gle.com, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	peterz@...radead.org, seanjc@...gle.com, tglx@...utronix.de,
	x86@...nel.org
Subject: Re: [PATCH RFC v2 03/29] mm: asi: Introduce ASI core API

On Fri, Feb 28, 2025 at 08:43:55AM +0000, Brendan Jackman wrote:
> Yeah I see what you mean. I think the issues are:
> 
> 1. We're mixing up two different aspects in the API:
> 
>    a. Starting and finishing "critical sections" (i.e. the region
>       between asi_enter() and asi_relax())
> 
>    b. Actually triggering address space transitions.
> 
> 2. There is a fundamental asymmetry at play here: asi_enter() and
>    asi_exit() can both be NOPs (when we're already in the relevant
>    address space), and asi_enter() being a NOP is really the _whole
>    point of ASI_.

I'm guessing you mean this thing in __asi_enter():

+	if (!target || target == this_cpu_read(curr_asi))
+		return;

The assumption being that curr_asi will be the target most of the time after
having done the expensive switch once...

>    The ideal world is where asi_exit() is very very rare, so
>    asi_enter() is almost always a NOP.

... asi_exit() being the actual switch to the unrestricted CR3.

And asi_relax() being the switch of current task's asi target ptr to NULL.
Comment says

"Domain to enter when returning to process context."

but I'm none-the-wiser.

So, why are we doing that relaxing thing?

I'm guessing the relaxing is marking the end of the region where we're running
untrusted code. After asi_relax() we are still in the restricted CR3 but we're
not running untrusted code.

> So we could disentangle part 1 by just rejigging things as you suggest,
> and I think the naming would be like:
> 
> asi_enter
>   asi_start_critical
>   asi_end_critical
> asi_exit

Yap, that's what I was gonna suggest: asi_enter and asi_exit do the actual CR3
build and switch and start_critical and end_critical do the cheaper tracking
thing.

> But the issue with that is that asi_start_critical() _must_ imply
> asi_enter()

What does that mean exactly?

asi_start_critical() must never be called before asi_enter()?

If so, I'm sure there are ways to track and check that and warn if not, right?

> (otherwise if we get an NMI between asi_enter() and
> asi_start_critical(), and that causes a #PF, we will start the
> critical section in the wrong address space and ASI won't do its job).
> So, we are somewhat forced to mix up a. and b. from above.

I don't understand: asi_enter() can be interrupted by an NMI at any random
point. How is the current, imbalanced interface not vulnerable to this
scenario?

> BTW, there is another thing complicating this picture a little: ASI
> "clients" (really just meaning KVM code at this point) are not not
> really supposed to care at all about the actual address space, the fact
> that they currently have to call asi_exit() in part 4b is just a
> temporary thing to simplify the initial implementation. It has a
> performance cost (not enormous, serious KVM platforms try pretty hard

You mean the switch to the unrestricted_cr3? I can imagine...

> to avoid returning to user space, but it does still matter) so
> Google's internal version has already got rid of it and that's where I
> expect this thing to evolve too. But for now it just lets us keep
> things simple since e.g. we never have to think about context
> switching in the restricted address space.
> 
> With that in mind, what if it looked like this:
> 
> ioctl(KVM_RUN) {
>     enter_from_user_mode()
>     while !need_userspace_handling()
> 	// This implies asi_enter(), but this code "doesn't care"
> 	// about that.
>         asi_start_critical();
>         vmenter();
>         asi_end_critical();
>     }
>     // TODO: This is temporary, it should not be needed.
>     asi_exit();
>     exit_to_user_mode()
> }
> 
> Once the asi_exit() call disappears, it will be symmetrical from the
> "client API"'s point of view. And while we still mix up address space
> switching with critical section boundaries, the address space
> switching is "just an implementation detail" and not really visible as
> part of the API.

So I'm still unclear on that whole design here so I'm asking silly questions
but I know that:

1. you can do empty calls to keep the interface balanced and easy to use

2. once you can remove asi_exit(), you should be able to replace all in-tree
   users in one atomic change so that they're all switched to the new,
   simplified interface

But I still have the feeling that we could re-jig what asi_enter/relax/exit do
and thus have a balanced interface. We'll see...

> I have now setup Mutt.

I did that 20 years ago. Never looked back. I'd say you're on the right track
:-)

> But, for now I am replying with plan vim + git-send-email, because I also
> sent this RFC to a ridiculous CC list (I just blindly used the
> get_maintainers.pl output, I don't know why I thought that was a reasonable
> approach) and it turns out this is the easiest way to trim it in a reply!
> Hopefully I can get the headers right...

Yeah, works. On the next version, you could trim it to the couple relevant
lists and to whoever reviewed this. The others can always get the thread from
lore and there's really no need anymore to Cc the whole world :)

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Powered by blists - more mailing lists