Re: [PATCH v4 25/30] context_tracking,x86: Defer kernel text patching IPIs

[Valentin Schneider <vschneid@xxxxxxxxxx>]

On 17/01/25 09:15, Sean Christopherson wrote:
> On Fri, Jan 17, 2025, Valentin Schneider wrote:
>> On 14/01/25 13:13, Sean Christopherson wrote:
>> > On Tue, Jan 14, 2025, Valentin Schneider wrote:
>> >> +/**
>> >> + * is_kernel_noinstr_text - checks if the pointer address is located in 
>> >> the
>> >> + *                    .noinstr section
>> >> + *
>> >> + * @addr: address to check
>> >> + *
>> >> + * Returns: true if the address is located in .noinstr, false otherwise.
>> >> + */
>> >> +static inline bool is_kernel_noinstr_text(unsigned long addr)
>> >> +{
>> >> + return addr >= (unsigned long)__noinstr_text_start &&
>> >> +        addr < (unsigned long)__noinstr_text_end;
>> >> +}
>> >
>> > This doesn't do the right thing for modules, which matters because KVM can 
>> > be
>> > built as a module on x86, and because context tracking understands 
>> > transitions
>> > to GUEST mode, i.e. CPUs that are running in a KVM guest will be treated 
>> > as not
>> > being in the kernel, and thus will have IPIs deferred.  If KVM uses a 
>> > static key
>> > or branch between guest_state_enter_irqoff() and 
>> > guest_state_exit_irqoff(), the
>> > patching code won't wait for CPUs to exit guest mode, i.e. KVM could 
>> > theoretically
>> > use the wrong static path.
>>>
>> AFAICT guest_state_{enter,exit}_irqoff() are only used in noinstr functions
>> and thus such a static key usage should at the very least be caught and
>> warned about by objtool - when this isn't built as a module.
>
> That doesn't magically do the right thing though.  If KVM is built as a 
> module,
> is_kernel_noinstr_text() will get false negatives even for static 
> keys/branches
> that are annotaed as NOINSTR.

Quite so.

I've been looking at mod_mem_type & friends, I'm thinking adding a
MOD_NOINSTR_TEXT type might be overkill considering modules really
shouldn't be involved with early entry, KVM being the one exception.

Your suggestion to have a KVM-module-specific noinstr section sounds good
to me, I'll have a look at that.