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

[Anastasios Papagiannis]

> 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()
include/linux/mm.h: declaration of copy_remote_mm_str() and  copy_remote_vm_str()
mm/util.c: shared implementation for copy_remote_mm_str() and copy_remote_vm_str()

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()?

--
Thanks,
-Anastasios