Re: [PATCH v5 bpf-next 1/3] bpf: Add bpf_access_process_vm() helper

From: Kenny Yu <kennyyu@fb.com>
To: <andrii.nakryiko@gmail.com>
Cc: <alexei.starovoitov@gmail.com>, <andrii@kernel.org>,
	<ast@kernel.org>, <bpf@vger.kernel.org>, <daniel@iogearbox.net>,
	<kennyyu@fb.com>, <phoenix1987@gmail.com>, <yhs@fb.com>
Subject: Re: [PATCH v5 bpf-next 1/3] bpf: Add bpf_access_process_vm() helper
Date: Fri, 21 Jan 2022 09:41:45 -0800	[thread overview]
Message-ID: <20220121174145.3433628-1-kennyyu@fb.com> (raw)
In-Reply-To: <CAEf4Bzaen2f2njYOAJuyWot2YvXn0YV=2zBVyFZw=_CqJdggPw@mail.gmail.com>

> > How about bpf_copy_from_user_task() ?
> > The task is the second to last argument, so the name fits ?
> 
> yeah, I like the name

I'll change the name to `bpf_copy_from_user_task`.

> > Especially if we call it this way it would be best to align
> > return codes with bpf_copy_from_user.
> > Adding memset() in case of failure is mandatory too.
> > I've missed this bit earlier.
>
> Yep, good catch! Seems like copy_from_user() currently returns amount
> of bytes *not* read and memsets those unread bytes to zero. So for
> efficiency we could probably memset only those that were read.
>
> > The question is to decide what to do with
> > ret > 0 && ret < size condition.
> > Is it a failure and we should memset() the whole buffer and
> > return -EFAULT or memset only the leftover bytes and return 0?
> > I think the former is best to align with bpf_copy_from_user.
>
> Yeah, I think all or nothing approach (either complete success and
> zero return, or memset and error return) is best and most in line with
> other similar helpers.

Thanks for the suggestions! I'll go with the all-or-nothing approach to
be consistent with `bpf_copy_from_user` and will make the following changes:

* Return value: returns 0 on success, or negative error on failure.
* If we had a partial read, we will memset the read bytes to 0 and return
  -EFAULT

> Another thing, I think it's important to mention that this helper can
> be used only from sleepable BPF programs.

Will add that to the docs.

> > That would be difficult. There is no suitable kernel api for that.
>
> Ok, but maybe we can add it later. Otherwise it will be hard to
> profiler Python processes and such, because you most certainly will
> need to read zero-terminated strings there.

I will NOT add a C string helper in this patch series, and I'll explore
how to add this in the future once this patch series is merged.

> > +       skel = bpf_iter_task__open_and_load();
> > +       if (CHECK(!skel, "bpf_iter_task__open_and_load",
> > +                 "skeleton open_and_load failed\n"))
>
> Please use ASSERT_OK_PTR() instead.

Will fix.

> > +       numread = bpf_access_process_vm(&user_data,
> > +                                       sizeof(uint32_t),
> > +                                       ptr,
> > +                                       task,
> > +                                       0);
>
> nit: keep it on one line (up to 100 characters is ok)

Will fix.

Thanks for the suggestions everyone!

Kenny