Re: [PATCH RFC v3 bpf-next 2/4] bpf: Add bpf_copy_from_user() helper.

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	Daniel Borkmann <daniel@iogearbox.net>,
	"Paul E . McKenney" <paulmck@kernel.org>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH RFC v3 bpf-next 2/4] bpf: Add bpf_copy_from_user() helper.
Date: Tue, 30 Jun 2020 11:53:24 -0700	[thread overview]
Message-ID: <20200630185324.5elry5u3ctaohszx@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <CAEf4BzbfXajuL-1VLBUJsC3P796s2hk9oYGveYG5QnS2=YoN-A@mail.gmail.com>

On Tue, Jun 30, 2020 at 11:23:07AM -0700, Andrii Nakryiko wrote:
> On Mon, Jun 29, 2020 at 5:28 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Thu, Jun 18, 2020 at 3:33 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > > > + *
> > > > + * int bpf_copy_from_user(void *dst, u32 size, const void *user_ptr)
> > >
> > > Can we also add bpf_copy_str_from_user (or bpf_copy_from_user_str,
> > > whichever makes more sense) as well?
> >
> > Those would have to wait. I think strings need better long term design.
> > That would be separate patches.
> 
> I agree that it would be nice to have better support for strings, long
> term, but that's beside the point.
> 
> I think bpf_copy_from_user_str() is a must have right now as a
> sleepable counterpart to bpf_probe_read_user_str(), just like
> bpf_copy_from_user() is a sleepable variant of bpf_probe_read_user().
> Look at progs/strobemeta.h, it does bpf_probe_read_user_str() to get
> user-space zero-terminated strings. It's well defined interface and
> behavior. There is nothing extra needed beyond a sleepable variant of
> bpf_probe_read_user_str() to allow Strobemeta reliably fetch data from
> user-space from inside a sleepable BPF program.

short answer: may be.
long answer: I'm not sure that bpf_probe_read_user_str() is such a great interface.
Copy pasting something just because it exists and already known is imo not
the best approach.
I believe KP is thinking about string chunking logic to be able
to pass many null terminated strings from bpf prog to user space.
envvars is such example. It's one giant array of strings separated
by zeros. The semantics of bpf_probe_read_user_str() may or may not
be a good fit.
strobemeta is 'cheating' with strings by doing max bound on all of them.
For tracing applications like strobemeta it's ok, but for lsm it's not.
Hence I'd like to see an api that solves lsm case along with strobemeta case.