Re: [RFC PATCH v8 4/7] mm/mempolicy: Export memory policy symbols

[Vlastimil Babka <vbabka@suse.cz>]

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