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@redhat.com
but, modules notwithstanding, simply checking if the patched instruction is in .noinstr was a lot neater.