Re: [RFC PATCH 04/47] mm: asi: ASI support in interrupts/exceptions

From: Junaid Shahid <junaids@google.com>
To: Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, jmattson@google.com,
	pjt@google.com, oweisse@google.com, alexandre.chartre@oracle.com,
	rppt@linux.ibm.com, dave.hansen@linux.intel.com,
	peterz@infradead.org, luto@kernel.org, linux-mm@kvack.org
Subject: Re: [RFC PATCH 04/47] mm: asi: ASI support in interrupts/exceptions
Date: Tue, 15 Mar 2022 15:41:23 -0700	[thread overview]
Message-ID: <87db9dbe-c7f1-deec-9c24-7d4bda406f2c@google.com> (raw)
In-Reply-To: <877d8v74tw.ffs@tglx>

Hi Thomas,

On 3/15/22 05:55, Thomas Gleixner wrote:
>>>
>>> This is wrong. You cannot invoke arbitrary code within a noinstr
>>> section.
>>>
>>> Please enable CONFIG_VMLINUX_VALIDATION and watch the build result with
>>> and without your patches.
>>>
>> Thank you for the pointer. It seems that marking asi_intr_enter/exit
>> and asi_enter/exit, and the few functions that they in turn call, as
>> noinstr would fix this, correct? (Along with removing the VM_BUG_ONs
>> from those functions and using notrace/nodebug variants of a couple of
>> functions).
> 
> you can keep the BUG_ON()s. If such a bug happens the noinstr
> correctness is the least of your worries, but it's important to get the
> information out, right?

Yes, that makes sense :)

> 
> Vs. adding noinstr. Yes, making the full callchain noinstr is going to
> cure it, but you really want to think hard whether these calls need to
> be in this section of the exception handlers.
> 
> These code sections have other constraints aside of being excluded from
> instrumentation, the main one being that you cannot use RCU there.

Neither of these functions need to use RCU, so that should be ok. Are there any other constraints that could matter here?

> 
> I'm not yet convinced that asi_intr_enter()/exit() need to be invoked in
> exactly the places you put it. The changelog does not give any clue
> about the why...

I had to place these calls early in the exception/interrupt handlers and specifically before the point where things like tracing and lockdep etc. can be used (and after the point where they can no longer be used, for the asi_intr_exit() calls). Otherwise, we would need to map all data structures touched by the tracing/lockdep infrastructure into the ASI restricted address spaces.

Basically, in general, if while running in a restricted address space, some kernel code touches some memory which is not mapped in the restricted address space, it will take an implicit ASI Exit via the page fault handler and continue running, so it would just be a small performance hit, but not a fatal issue. But there are 3 critical code regions where this implicit ASI Exit mechanism doesn't apply. The first is the region between an asi_enter() call and the asi_set_target_unrestricted() call. The second is the region between the start of an interrupt/exception handler and the asi_intr_enter() call, and the third is the region between the asi_intr_exit() call and the IRET. So any memory that is accessed by the code in these regions has to be mapped in the restricted address space, which is why I tried to place the asi_intr_enter/exit calls fairly early/late in the handlers. It is possible to move them further in, but if we accidentally miss annotating some data needed in that region, then it could potentially be fatal in some situations.

Thanks,
Junaid

> 
> Thanks,
> 
>          tglx
> 
> 
> 