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

[Shakeel Butt <shakeel.butt@linux.dev>]

On Tue, Feb 11, 2025 at 02:07:31PM -0800, Andrii Nakryiko wrote:
> 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.

I think early return 0 on len == 0 should be fine.