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

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Kenny Yu <kennyyu@fb.com>, Andrii Nakryiko <andrii@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>, bpf <bpf@vger.kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Gabriele <phoenix1987@gmail.com>, Yonghong Song <yhs@fb.com>
Subject: Re: [PATCH v5 bpf-next 1/3] bpf: Add bpf_access_process_vm() helper
Date: Fri, 21 Jan 2022 09:20:39 -0800	[thread overview]
Message-ID: <CAEf4Bzaen2f2njYOAJuyWot2YvXn0YV=2zBVyFZw=_CqJdggPw@mail.gmail.com> (raw)
In-Reply-To: <CAADnVQL-85q36gRvMGocXMLNk4WjDa_Xpi8Y9ZQS+qYLhF8E+A@mail.gmail.com>

On Thu, Jan 20, 2022 at 6:28 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Jan 20, 2022 at 2:46 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> a
> > > + *             wrapper of **access_process_vm**\ ().
> > > + *     Return
> > > + *             The number of bytes written to the buffer, or a negative error
> > > + *             in case of failure.
> >
> > wait, can it read less than *size* and return success?
> >
> > bpf_probe_read_kernel() returns:
> >
> > 0 on success, or a negative error in case of failure.
> >
> > Let's be consistent. Returning the number of read bytes makes more
> > sense in cases when we don't know the amount of bytes to be actually
> > read ahead of time (e.g., when reading zero-terminated strings).
> >
> > BTW, should we also add a C string reading helper as well, just like
> > there is bpf_probe_read_user_str() and bpf_probe_read_user()?
>
> 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.

>
> > Another thing, I think it's important to mention that this helper can
> > be used only from sleepable BPF programs.
> >
> > And not to start the bikeshedding session, but we have
> > bpf_copy_from_user(), wouldn't something like
> > bpf_copy_from_user_{vm,process,remote}() be more in line and less
> > surprising for BPF users. BTW, "access" implies writing just as much
> > as reading, so using "access" in the sense of "read" seems wrong and
> > confusing.
>
> How about bpf_copy_from_user_task() ?
> The task is the second to last argument, so the name fits ?

yeah, I like the name

> 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.