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/[email protected]
but, modules notwithstanding, simply checking if the patched instruction is
in .noinstr was a lot neater.
