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

[Jordan Rome <linux@jordanrome.com>]

On Tue, Feb 11, 2025 at 9:19 PM Shakeel Butt <shakeel.butt@linux.dev> wrote:
>
> 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.

Ack.