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

[Sean Christopherson <seanjc@xxxxxxxxxx>]

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().