Re: [bpf-next v7 1/3] mm: add copy_remote_vm_str

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Mon, Feb 10, 2025 at 2:23 PM Jordan Rome <linux@jordanrome.com> wrote:
>
> Similar to `access_process_vm` but specific to strings.
> Also chunks reads by page and utilizes `strscpy`
> for handling null termination.
>
> Signed-off-by: Jordan Rome <linux@jordanrome.com>
> ---
>  include/linux/mm.h |   3 ++
>  mm/memory.c        | 119 +++++++++++++++++++++++++++++++++++++++++++++
>  mm/nommu.c         |  73 +++++++++++++++++++++++++++
>  3 files changed, 195 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 7b1068ddcbb7..aee23d84ce01 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2486,6 +2486,9 @@ extern int access_process_vm(struct task_struct *tsk, unsigned long addr,
>  extern int access_remote_vm(struct mm_struct *mm, unsigned long addr,
>                 void *buf, int len, unsigned int gup_flags);
>
> +extern int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
> +               void *buf, int len, unsigned int gup_flags);
> +
>  long get_user_pages_remote(struct mm_struct *mm,
>                            unsigned long start, unsigned long nr_pages,
>                            unsigned int gup_flags, struct page **pages,
> diff --git a/mm/memory.c b/mm/memory.c
> index 539c0f7c6d54..e9d8584a7f56 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -6803,6 +6803,125 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr,
>  }
>  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,
> +                             void *buf, int len, unsigned int gup_flags)
> +{
> +       void *old_buf = buf;
> +       int err = 0;
> +
> +       *(char *)buf = '\0';

LGTM overall:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

But note that all this unconditional buf access will be incorrect if
len == 0. So either all of that has to be guarded with `if (len)`,
just dropped, or declared unsupported, depending on what mm folks
think. BPF helper won't ever call with len == 0, so that's why my ack.

(And yes, it would be nice to hear from someone from the MM side at
this point, thank you!).

> +
> +       if (mmap_read_lock_killable(mm))
> +               return -EFAULT;
> +
> +       /* Untag the address before looking up the VMA */
> +       addr = untagged_addr_remote(mm, addr);
> +
> +       /* Avoid triggering the temporary warning in __get_user_pages */
> +       if (!vma_lookup(mm, addr)) {
> +               err = -EFAULT;
> +               goto out;
> +       }
> +

[...]