1. Patchew
  2. linux
  3. View series

[PATCH] KVM: x86: Enforce use of EXPORT_SYMBOL_FOR_KVM_INTERNAL

Sean Christopherson posted 1 patch 1 month, 1 week ago
There is a newer version of this series
arch/x86/kvm/Makefile | 56 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 56 insertions(+)
[PATCH] KVM: x86: Enforce use of EXPORT_SYMBOL_FOR_KVM_INTERNAL
Posted by Sean Christopherson 1 month, 1 week ago 
Add a (gnarly) inline "script" in the Makefile to fail the build if there
is EXPORT_SYMBOL_GPL or EXPORT_SYMBOL usage in virt/kvm or arch/x86/kvm
beyond the known-good/expected exports for other modules.  Remembering to
use EXPORT_SYMBOL_FOR_KVM_INTERNAL is surprisingly difficult, and hoping
to detect "bad" exports via code review is not a robust long-term strategy.

Jump through a pile of hoops to coerce make into printing a human-friendly
error message, with the offending files+lines cleanly separated.

E.g. where <srctree> is the resolution of $(srctree), i.e. '.' for in-tree
builds, and the absolute path for out-of-tree-builds:

  <srctree>/arch/x86/kvm/Makefile:97: *** ERROR ***
  found 2 unwanted occurrences of EXPORT_SYMBOL_GPL:
    <srctree>/arch/x86/kvm/x86.c:686:EXPORT_SYMBOL_GPL(__kvm_set_user_return_msr);
    <srctree>/arch/x86/kvm/x86.c:703:EXPORT_SYMBOL_GPL(kvm_set_user_return_msr);
  in directories:
    <srctree>/arch/x86/kvm
    <srctree>/virt/kvm
  Use EXPORT_SYMBOL_FOR_KVM_INTERNAL, not EXPORT_SYMBOL_GPL.  Stop.

and

  <srctree>/arch/x86/kvm/Makefile:98: *** ERROR ***
  found 1 unwanted occurrences of EXPORT_SYMBOL:
    <srctree>/arch/x86/kvm/x86.c:709:EXPORT_SYMBOL(kvm_get_user_return_msr);
  in directories:
    <srctree>/arch/x86/kvm
    <srctree>/virt/kvm
  Use EXPORT_SYMBOL_FOR_KVM_INTERNAL, not EXPORT_SYMBOL.  Stop.

Put the enforcement in x86's Makefile even though the rule itself applies
to virt/kvm, as putting the enforcement in virt/kvm/Makefile.kvm would
effectively require exempting every architecture except x86.  PPC is the
only other architecture with sub-modules, and PPC hasn't been switched to
use EXPORT_SYMBOL_FOR_KVM_INTERNAL (and given its nearly-orphaned state,
likely never will).  And for KVM architectures without sub-modules, that
means that, barring truly spurious exports, the exports are intended for
non-KVM usage and thus shouldn't be using EXPORT_SYMBOL_FOR_KVM_INTERNAL.

Cc: Chao Gao <chao.gao@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/Makefile | 56 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
index c4b8950c7abe..357138ac5cc6 100644
--- a/arch/x86/kvm/Makefile
+++ b/arch/x86/kvm/Makefile
@@ -47,3 +47,59 @@ $(obj)/kvm-asm-offsets.h: $(obj)/kvm-asm-offsets.s FORCE
 
 targets += kvm-asm-offsets.s
 clean-files += kvm-asm-offsets.h
+
+
+# Fail the build if there is unexpected EXPORT_SYMBOL_GPL (or EXPORT_SYMBOL)
+# usage.  All KVM-internal exports should use EXPORT_SYMBOL_FOR_KVM_INTERNAL.
+# Only a handful of exports intended for other modules (VFIO, KVMGT) should
+# use EXPORT_SYMBOL_GPL, and EXPORT_SYMBOL should never be used.
+ifdef CONFIG_KVM_X86
+define newline
+
+
+endef
+
+# Search recursively for whole words and print line numbers.  Filter out the
+# allowed set of exports, i.e. those that are intended for external usage.
+exports_grep_trailer := --include='*.[ch]' -nrw $(srctree)/virt/kvm $(srctree)/arch/x86/kvm | \
+			grep -v -e kvm_page_track_register_notifier \
+				-e kvm_page_track_unregister_notifier \
+				-e kvm_write_track_add_gfn \
+				-e kvm_write_track_remove_gfn \
+				-e kvm_get_kvm \
+				-e kvm_get_kvm_safe \
+				-e kvm_put_kvm
+
+# Force grep to emit a goofy group separator that can in turn be replaced with
+# the above newline macro (newlines in Make are a nightmare).  Note, grep only
+# prints the group separator when N lines of context are requested via -C,
+# a.k.a. --NUM.  Simply request zero lines.  Print the separator only after
+# filtering out expected exports to avoid extra newlines in the error message.
+define get_kvm_exports
+$(shell grep "$(1)" -C0 $(exports_grep_trailer) | grep "$(1)" -C0 --group-separator="AAAA")
+endef
+
+define check_kvm_exports
+nr_kvm_exports := $(shell grep "$(1)" $(exports_grep_trailer) | wc -l)
+
+ifneq (0,$$(nr_kvm_exports))
+$$(error ERROR ***\
+$$(newline)found $$(nr_kvm_exports) unwanted occurrences of $(1):\
+$$(newline)  $(subst AAAA,$$(newline) ,$(call get_kvm_exports,$(1)))\
+$$(newline)in directories:\
+$$(newline)  $(srctree)/arch/x86/kvm\
+$$(newline)  $(srctree)/virt/kvm\
+$$(newline)Use EXPORT_SYMBOL_FOR_KVM_INTERNAL, not $(1))
+endif # nr_kvm_exports != expected
+undefine exports_advice
+undefine nr_kvm_exports
+endef # check_kvm_exports
+
+$(eval $(call check_kvm_exports,EXPORT_SYMBOL_GPL))
+$(eval $(call check_kvm_exports,EXPORT_SYMBOL))
+
+undefine check_kvm_exports
+undefine get_kvm_exports
+undefine exports_grep_trailer
+undefine newline
+endif # CONFIG_KVM_X86

base-commit: a996dd2a5e1ec54dcf7d7b93915ea3f97e14e68a
-- 
2.51.2.1041.gc1ab5b90ca-goog
Re: [PATCH] KVM: x86: Enforce use of EXPORT_SYMBOL_FOR_KVM_INTERNAL
Posted by Paolo Bonzini 1 month, 1 week ago 
On 11/6/25 21:28, Sean Christopherson wrote:
> +# Fail the build if there is unexpected EXPORT_SYMBOL_GPL (or EXPORT_SYMBOL)
> +# usage.  All KVM-internal exports should use EXPORT_SYMBOL_FOR_KVM_INTERNAL.
> +# Only a handful of exports intended for other modules (VFIO, KVMGT) should
> +# use EXPORT_SYMBOL_GPL, and EXPORT_SYMBOL should never be used.
> +ifdef CONFIG_KVM_X86
> +define newline
> +
> +
> +endef

$(newline) is already defined in scripts/Kbuild.include, is it necessary 
here?

> +# Search recursively for whole words and print line numbers.  Filter out the
> +# allowed set of exports, i.e. those that are intended for external usage.
> +exports_grep_trailer := --include='*.[ch]' -nrw $(srctree)/virt/kvm $(srctree)/arch/x86/kvm | \
> +			grep -v -e kvm_page_track_register_notifier \
> +				-e kvm_page_track_unregister_notifier \
> +				-e kvm_write_track_add_gfn \
> +				-e kvm_write_track_remove_gfn \
> +				-e kvm_get_kvm \
> +				-e kvm_get_kvm_safe \
> +				-e kvm_put_kvm
> +
> +# Force grep to emit a goofy group separator that can in turn be replaced with
> +# the above newline macro (newlines in Make are a nightmare).  Note, grep only
> +# prints the group separator when N lines of context are requested via -C,
> +# a.k.a. --NUM.  Simply request zero lines.  Print the separator only after
> +# filtering out expected exports to avoid extra newlines in the error message.
> +define get_kvm_exports
> +$(shell grep "$(1)" -C0 $(exports_grep_trailer) | grep "$(1)" -C0 --group-separator="AAAA")

Maybe replace AAAA with something less goofy like !SEP! or similar?

> +endef
> +
> +define check_kvm_exports
> +nr_kvm_exports := $(shell grep "$(1)" $(exports_grep_trailer) | wc -l)
> +
> +ifneq (0,$$(nr_kvm_exports))
> +$$(error ERROR ***\
> +$$(newline)found $$(nr_kvm_exports) unwanted occurrences of $(1):\
> +$$(newline)  $(subst AAAA,$$(newline) ,$(call get_kvm_exports,$(1)))\
> +$$(newline)in directories:\
> +$$(newline)  $(srctree)/arch/x86/kvm\
> +$$(newline)  $(srctree)/virt/kvm\
> +$$(newline)Use EXPORT_SYMBOL_FOR_KVM_INTERNAL, not $(1))
> +endif # nr_kvm_exports != expected
> +undefine exports_advice
> +undefine nr_kvm_exports
> +endef # check_kvm_exports
> +
> +$(eval $(call check_kvm_exports,EXPORT_SYMBOL_GPL))
> +$(eval $(call check_kvm_exports,EXPORT_SYMBOL))
> +
> +undefine check_kvm_exports
> +undefine get_kvm_exports
> +undefine exports_grep_trailer
> +undefine newline

(if the definition is not needed above, remember to remove the 
"undefine" here too).

Thanks,

Paolo
Re: [PATCH] KVM: x86: Enforce use of EXPORT_SYMBOL_FOR_KVM_INTERNAL
Posted by Chao Gao 1 month, 1 week ago 
On Thu, Nov 06, 2025 at 12:28:11PM -0800, Sean Christopherson wrote:
>Add a (gnarly) inline "script" in the Makefile to fail the build if there
>is EXPORT_SYMBOL_GPL or EXPORT_SYMBOL usage in virt/kvm or arch/x86/kvm
>beyond the known-good/expected exports for other modules.  Remembering to
>use EXPORT_SYMBOL_FOR_KVM_INTERNAL is surprisingly difficult, and hoping
>to detect "bad" exports via code review is not a robust long-term strategy.
>
>Jump through a pile of hoops to coerce make into printing a human-friendly
>error message, with the offending files+lines cleanly separated.
>
>E.g. where <srctree> is the resolution of $(srctree), i.e. '.' for in-tree
>builds, and the absolute path for out-of-tree-builds:
>
>  <srctree>/arch/x86/kvm/Makefile:97: *** ERROR ***
>  found 2 unwanted occurrences of EXPORT_SYMBOL_GPL:
>    <srctree>/arch/x86/kvm/x86.c:686:EXPORT_SYMBOL_GPL(__kvm_set_user_return_msr);
>    <srctree>/arch/x86/kvm/x86.c:703:EXPORT_SYMBOL_GPL(kvm_set_user_return_msr);
>  in directories:
>    <srctree>/arch/x86/kvm
>    <srctree>/virt/kvm
>  Use EXPORT_SYMBOL_FOR_KVM_INTERNAL, not EXPORT_SYMBOL_GPL.  Stop.
>
>and
>
>  <srctree>/arch/x86/kvm/Makefile:98: *** ERROR ***
>  found 1 unwanted occurrences of EXPORT_SYMBOL:
>    <srctree>/arch/x86/kvm/x86.c:709:EXPORT_SYMBOL(kvm_get_user_return_msr);
>  in directories:
>    <srctree>/arch/x86/kvm
>    <srctree>/virt/kvm
>  Use EXPORT_SYMBOL_FOR_KVM_INTERNAL, not EXPORT_SYMBOL.  Stop.
>
>Put the enforcement in x86's Makefile even though the rule itself applies
>to virt/kvm, as putting the enforcement in virt/kvm/Makefile.kvm would
>effectively require exempting every architecture except x86.  PPC is the
>only other architecture with sub-modules, and PPC hasn't been switched to
>use EXPORT_SYMBOL_FOR_KVM_INTERNAL (and given its nearly-orphaned state,
>likely never will).  And for KVM architectures without sub-modules, that
>means that, barring truly spurious exports, the exports are intended for
>non-KVM usage and thus shouldn't be using EXPORT_SYMBOL_FOR_KVM_INTERNAL.
>
>Cc: Chao Gao <chao.gao@intel.com>
>Signed-off-by: Sean Christopherson <seanjc@google.com>

Tested-by: Chao Gao <chao.gao@intel.com>

>---
> arch/x86/kvm/Makefile | 56 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 56 insertions(+)
>
>diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
>index c4b8950c7abe..357138ac5cc6 100644
>--- a/arch/x86/kvm/Makefile
>+++ b/arch/x86/kvm/Makefile
>@@ -47,3 +47,59 @@ $(obj)/kvm-asm-offsets.h: $(obj)/kvm-asm-offsets.s FORCE
> 
> targets += kvm-asm-offsets.s
> clean-files += kvm-asm-offsets.h
>+
>+
>+# Fail the build if there is unexpected EXPORT_SYMBOL_GPL (or EXPORT_SYMBOL)
>+# usage.  All KVM-internal exports should use EXPORT_SYMBOL_FOR_KVM_INTERNAL.
>+# Only a handful of exports intended for other modules (VFIO, KVMGT) should
>+# use EXPORT_SYMBOL_GPL, and EXPORT_SYMBOL should never be used.
>+ifdef CONFIG_KVM_X86
>+define newline
>+
>+
>+endef
>+
>+# Search recursively for whole words and print line numbers.  Filter out the
>+# allowed set of exports, i.e. those that are intended for external usage.
>+exports_grep_trailer := --include='*.[ch]' -nrw $(srctree)/virt/kvm $(srctree)/arch/x86/kvm | \
>+			grep -v -e kvm_page_track_register_notifier \
>+				-e kvm_page_track_unregister_notifier \
>+				-e kvm_write_track_add_gfn \
>+				-e kvm_write_track_remove_gfn \
>+				-e kvm_get_kvm \
>+				-e kvm_get_kvm_safe \
>+				-e kvm_put_kvm
>+
>+# Force grep to emit a goofy group separator that can in turn be replaced with
>+# the above newline macro (newlines in Make are a nightmare).  Note, grep only
>+# prints the group separator when N lines of context are requested via -C,
>+# a.k.a. --NUM.  Simply request zero lines.  Print the separator only after
>+# filtering out expected exports to avoid extra newlines in the error message.
>+define get_kvm_exports
>+$(shell grep "$(1)" -C0 $(exports_grep_trailer) | grep "$(1)" -C0 --group-separator="AAAA")
>+endef
>+
>+define check_kvm_exports
>+nr_kvm_exports := $(shell grep "$(1)" $(exports_grep_trailer) | wc -l)
>+
>+ifneq (0,$$(nr_kvm_exports))
>+$$(error ERROR ***\
>+$$(newline)found $$(nr_kvm_exports) unwanted occurrences of $(1):\
>+$$(newline)  $(subst AAAA,$$(newline) ,$(call get_kvm_exports,$(1)))\
>+$$(newline)in directories:\
>+$$(newline)  $(srctree)/arch/x86/kvm\
>+$$(newline)  $(srctree)/virt/kvm\

any reason to print directories here? the error message already has the file
name and the line number.

>+$$(newline)Use EXPORT_SYMBOL_FOR_KVM_INTERNAL, not $(1))
>+endif # nr_kvm_exports != expected

nit: "expected" in the tailing comment is not defined anywhere; maybe use 0 instead.

>+undefine exports_advice

exports_advice is not defined.

>+undefine nr_kvm_exports
>+endef # check_kvm_exports
>+
>+$(eval $(call check_kvm_exports,EXPORT_SYMBOL_GPL))
>+$(eval $(call check_kvm_exports,EXPORT_SYMBOL))
>+
>+undefine check_kvm_exports
>+undefine get_kvm_exports
>+undefine exports_grep_trailer
>+undefine newline
>+endif # CONFIG_KVM_X86
>
>base-commit: a996dd2a5e1ec54dcf7d7b93915ea3f97e14e68a
>-- 
>2.51.2.1041.gc1ab5b90ca-goog
>
Re: [PATCH] KVM: x86: Enforce use of EXPORT_SYMBOL_FOR_KVM_INTERNAL
Posted by Sean Christopherson 1 month ago 
On Mon, Nov 10, 2025, Chao Gao wrote:
> On Thu, Nov 06, 2025 at 12:28:11PM -0800, Sean Christopherson wrote:
> >+ifneq (0,$$(nr_kvm_exports))
> >+$$(error ERROR ***\
> >+$$(newline)found $$(nr_kvm_exports) unwanted occurrences of $(1):\
> >+$$(newline)  $(subst AAAA,$$(newline) ,$(call get_kvm_exports,$(1)))\
> >+$$(newline)in directories:\
> >+$$(newline)  $(srctree)/arch/x86/kvm\
> >+$$(newline)  $(srctree)/virt/kvm\
> 
> any reason to print directories here? the error message already has the file
> name and the line number.

I want to fully disambiguate the file, e.g. KVM has multiple versions of pmu.c.
And on the off chance someone unfamiliar with KVM breaks things, to provide a
more verbose hint on how to fix the issue.  We have a similar "rule" internally
related to mmu_lock, and the initial implementation of the rule simply failed
the build with no diagnostic information.  It was incredibly painful to debug,
and I want to avoid foisting that experience on others. :-)

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.