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

["David Hildenbrand (Arm)" <david@kernel.org>]

> diff --git a/mm/nommu.c b/mm/nommu.c
> index ed3934bc2de4..94e3709e95fd 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1716,13 +1716,16 @@ EXPORT_SYMBOL_GPL(access_process_vm);
>   * Copy a string from another process's address space as given in mm.
>   * If there is any error return -EFAULT.
>   */
> -static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
> +static int __copy_remote_mm_str(struct mm_struct *mm, unsigned long addr,
>  				void *buf, int len)
>  {
>  	unsigned long addr_end;
>  	struct vm_area_struct *vma;
>  	int ret = -EFAULT;
>  
> +	if (unlikely(len == 0))
> +		return 0;

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.

> +
>  	*(char *)buf = '\0';
>  
>  	if (mmap_read_lock_killable(mm))
> @@ -1752,6 +1755,27 @@ static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
>  	return ret;
>  }
>  
> +/**
> + * copy_remote_mm_str - copy a string from a remote address space.
> + * @mm:         the remote address space
> + * @addr:       start address to read from
> + * @buf:        destination buffer
> + * @len:        number of bytes to copy
> + * @gup_flags:  flags modifying lookup behaviour (unused)
> + *
> + * The caller must hold a reference on @mm.
> + *
> + * Return: number of bytes copied from @addr (source) to @buf (destination),
> + * not including the trailing NUL. If @len is zero, return 0 without accessing
> + * @buf. Otherwise, @buf is always NUL-terminated. On any error, return
> + * -EFAULT.
> + */
> +int copy_remote_mm_str(struct mm_struct *mm, unsigned long addr,
> +		       void *buf, int len, unsigned int gup_flags)
> +{
> +	return __copy_remote_mm_str(mm, addr, buf, len);
> +}
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)

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?

Just a thought. CCing Lorenzo.

-- 
Cheers,

David