Re: [PATCH bpf-next v4 1/7] mm: Add copy_remote_mm_str()

[David Hildenbrand (Arm)]

On 9/7/26 15:41, Anastasios Papagiannis wrote:
>> We have this check in copy_remote_vm_str(). Why are we performing the check now
>> twice?
> 
>> It should either go only into __copy_remote_mm_str(), or if there a reason to
>> have it before get_task_mm(), it should go into copy_remote_mm_str(). Same
>> applies to the memory.c case.
> 
> Yes, this makes sense. I will fix that.
> 
>> What's more annoying is that both implementations of copy_remote_vm_str() are
>> identical, and both implementations of copy_remote_mm_str() are nearly identical
>> (just dropping the gup_flags for nommu). I'd like to avoid duplicating code for
>> nommu.
> 
>> If we could export __copy_remote_vm_str(mm, addr, buf, len, gup_flags) for both
>> cases, we could instead provide a single implementation for copy_remote_vm_str()
>> and copy_remote_mm_str() e.g., in mm.h? (I'd prefer somewhere else, but we don't
>> seem to have a good git for memory.c + nommu.c shared stuff)
> 
> Another idea can be:
> mm/memory.c: MMU implementation of __copy_remote_mm_str()
> mm/nommu.c: NOMMU implementation of __copy_remote_mm_str()
> mm/internal.h: declaration of __copy_remote_mm_str()

Ack

> include/linux/mm.h: declaration of copy_remote_mm_str() and  copy_remote_vm_str()

Ack.

> mm/util.c: shared implementation for copy_remote_mm_str() and copy_remote_vm_str()

Ah, yes, util.c is the good fit I was missing :)

> 
> This allows us to remove the duplicate code. Does this sound reasonable?
> 
>> Now, that's also not completely nice, as I don't want us to EXPORT
>> __copy_remote_vm_str() ... given that these functions are "#ifdef
>> CONFIG_BPF_SYSCALL" could we EXPORT_SYMBOL_FOR_MODULES?
> 
> Now copy_remote_vm_str() is EXPORT_SYMBOL_GPL. In this series, we use
> copy_remote_mm_str() without the need to export that. Why do we need to
> consider exporting __copy_remote_vm_str()?

If the implementation lives in mm/util.c (instead of a header), there is no need.

-- 
Cheers,

David