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

[Valentin Schneider <vschneid@xxxxxxxxxx>]

On 14/01/25 13:48, Sean Christopherson wrote:
> On Tue, Jan 14, 2025, 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.
>>
>> I don't expect this to ever cause problems in practice, because patching 
>> code in
>> KVM's VM-Enter/VM-Exit path that has *functional* implications, while CPUs 
>> are
>> actively running guest code, would be all kinds of crazy.  But I do think we
>> should plug the hole.
>>
>> If this issue is unique to KVM, i.e. is not a generic problem for all 
>> modules (I
>> assume module code generally isn't allowed in the entry path, even via 
>> NMI?), one
>> idea would be to let KVM register its noinstr section for text poking.
>
> Another idea would be to track which keys/branches are tagged noinstr, i.e. 
> generate
> the information at compile time instead of doing lookups at runtime.  The 
> biggest
> downside I can think of is that it would require plumbing in the information 
> to
> text_poke_bp_batch().

IIUC that's what I went for in v3:

https://lore.kernel.org/lkml/20241119153502.41361-11-vschneid@xxxxxxxxxx

but, modules notwithstanding, simply checking if the patched instruction is
in .noinstr was a lot neater.