| Message ID | 20250618112935.7629-5-shivankg@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add NUMA mempolicy support for KVM guest-memfd | expand |
On 6/19/25 13:13, Shivank Garg wrote: > > > On 6/18/2025 8:42 PM, Gregory Price wrote: >> On Wed, Jun 18, 2025 at 11:29:32AM +0000, Shivank Garg wrote: >>> KVM guest_memfd wants to implement support for NUMA policies just like >>> shmem already does using the shared policy infrastructure. As >>> guest_memfd currently resides in KVM module code, we have to export the >>> relevant symbols. >>> >>> In the future, guest_memfd might be moved to core-mm, at which point the >>> symbols no longer would have to be exported. When/if that happens is >>> still unclear. >>> >>> Acked-by: David Hildenbrand <david@redhat.com> >>> Acked-by: Vlastimil Babka <vbabka@suse.cz> >>> Signed-off-by: Shivank Garg <shivankg@amd.com> >>> --- >>> mm/mempolicy.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c >>> index 3b1dfd08338b..d98243cdf090 100644 >>> --- a/mm/mempolicy.c >>> +++ b/mm/mempolicy.c >>> @@ -354,6 +354,7 @@ struct mempolicy *get_task_policy(struct task_struct *p) >>> >>> return &default_policy; >>> } >>> +EXPORT_SYMBOL_GPL(get_task_policy); >>> >>> static const struct mempolicy_operations { >>> int (*create)(struct mempolicy *pol, const nodemask_t *nodes); >>> @@ -487,6 +488,7 @@ void __mpol_put(struct mempolicy *pol) >>> return; >>> kmem_cache_free(policy_cache, pol); >>> } >>> +EXPORT_SYMBOL_GPL(__mpol_put); >>> >> >> I'm concerned that get_task_policy doesn't actually increment the policy Hm it might be a bit misnomer. But fixing that would be out of scope here. >> refcount - and mpol_cond_put only decrements the refcount for shared >> policies (vma policies) - while __mpol_put decrements it unconditionally. >> >> If you look at how get_task_policy is used internally to mempolicy, >> you'll find that it either completes the operation in the context of the >> task lock (allocation time) or it calls mpol_get afterwards. > > I agree. But the semantics of my usage isn't new. shmem use this in same way. Yeah it's only used in the context of the allocation or the get_mempolicy() syscall and the pointer is not retained somewhere indefinitely. In case of task's mempolicy, the protection comes from only accessing current task's policy, and also only the current task can replace it with the sys_mempolicy() syscall. > I think the alloc_frozen_pages_noprof(), alloc_pages_bulk_mempolicy_noprof() > calls get_task_policy without task_lock or calling mpol_get. Yes. >> >> Exporting this as-is creates a triping hazard, if only because get/put >> naming implies reference counting. I don't think we in general consider the act of export a larger hazard for misuse than misuse by internal code. For e.g. __mpol_put() we have to export it due to combination of inlined and non-inlined code, but nobody would really call it directly, but use mpol_put() and mpol_cond_put(). We'd need to be able to "un-declare" it after the usage in the two inline wrappers to prevent direct (mis)use by both modules and non-modules. > Since KVM is the only user, we could consider newly added EXPORT_SYMBOL_GPL_FOR_MODULES(..., "kvm") > to avoid wider exposure. Yes that would be preferred now for all the guest_memfd related series in flight adding exports anywhere. > Does this solve your concern? > Or should we rename these functions. > What should be the preferred approach? > > Thanks, > Shivank
diff --git a/mm/mempolicy.c b/mm/mempolicy.c index 3b1dfd08338b..d98243cdf090 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -354,6 +354,7 @@ struct mempolicy *get_task_policy(struct task_struct *p) return &default_policy; } +EXPORT_SYMBOL_GPL(get_task_policy); static const struct mempolicy_operations { int (*create)(struct mempolicy *pol, const nodemask_t *nodes); @@ -487,6 +488,7 @@ void __mpol_put(struct mempolicy *pol) return; kmem_cache_free(policy_cache, pol); } +EXPORT_SYMBOL_GPL(__mpol_put); static void mpol_rebind_default(struct mempolicy *pol, const nodemask_t *nodes) { @@ -2888,6 +2890,7 @@ struct mempolicy *mpol_shared_policy_lookup(struct shared_policy *sp, read_unlock(&sp->lock); return pol; } +EXPORT_SYMBOL_GPL(mpol_shared_policy_lookup); static void sp_free(struct sp_node *n) { @@ -3173,6 +3176,7 @@ void mpol_shared_policy_init(struct shared_policy *sp, struct mempolicy *mpol) mpol_put(mpol); /* drop our incoming ref on sb mpol */ } } +EXPORT_SYMBOL_GPL(mpol_shared_policy_init); int mpol_set_shared_policy(struct shared_policy *sp, struct vm_area_struct *vma, struct mempolicy *pol) @@ -3191,6 +3195,7 @@ int mpol_set_shared_policy(struct shared_policy *sp, sp_free(new); return err; } +EXPORT_SYMBOL_GPL(mpol_set_shared_policy); /* Free a backing policy store on inode delete. */ void mpol_free_shared_policy(struct shared_policy *sp) @@ -3209,6 +3214,7 @@ void mpol_free_shared_policy(struct shared_policy *sp) } write_unlock(&sp->lock); } +EXPORT_SYMBOL_GPL(mpol_free_shared_policy); #ifdef CONFIG_NUMA_BALANCING static int __initdata numabalancing_override;