Re: [PATCH v2 bpf-next 0/6] Dynptr convenience helpers

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Joanne Koong <joannelkoong@gmail.com>,
	bpf@vger.kernel.org, andrii@kernel.org, kernel-team@meta.com,
	ast@kernel.org, daniel@iogearbox.net, martin.lau@linux.dev,
	song@kernel.org
Subject: Re: [PATCH v2 bpf-next 0/6] Dynptr convenience helpers
Date: Thu, 8 Dec 2022 16:42:06 -0800	[thread overview]
Message-ID: <CAEf4BzYGUf=yMry5Ezen2PZqvkfS+o1jSF2e1Fpa+pgAmx+OcA@mail.gmail.com> (raw)
In-Reply-To: <20221208015434.ervz6q5j7bb4jt4a@macbook-pro-6.dhcp.thefacebook.com>

On Wed, Dec 7, 2022 at 5:54 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Dec 07, 2022 at 12:55:31PM -0800, Joanne Koong wrote:
> > This patchset is the 3rd in the dynptr series. The 1st can be found here [0]
> > and the 2nd can be found here [1].
> >
> > In this patchset, the following convenience helpers are added for interacting
> > with bpf dynamic pointers:
> >
> >     * bpf_dynptr_data_rdonly
> >     * bpf_dynptr_trim
> >     * bpf_dynptr_advance
> >     * bpf_dynptr_is_null
> >     * bpf_dynptr_is_rdonly
> >     * bpf_dynptr_get_size
> >     * bpf_dynptr_get_offset
> >     * bpf_dynptr_clone
> >     * bpf_dynptr_iterator
>
> This is great, but it really stretches uapi limits.

Stretches in what sense? They are simple and straightforward getters
and trim/advance/clone are fundamental modifiers to be able to work
with a subset of dynptr's overall memory area.

> Please convert the above and those in [1] to kfuncs.
> I know that there can be an argument made for consistency with existing dynptr uapi

yeah, given we have bpf_dynptr_{read,write} and bpf_dynptr_data() as
BPF helpers, it makes sense to have such basic things like is_null and
trim/advance/clone as BPF helpers as well. Both for consistency and
because there is nothing unstable about them. We are not going to
remove dynptr as a concept, it's pretty well defined.

Out of the above list perhaps only move bpf_dynptr_iterator() might be
a candidate for kfunc. Though, personally, it makes sense to me to
keep it as BPF helper without GPL restriction as well, given it is
meant for networking applications in the first place, and you don't
need to be GPL-compatible to write useful networking BPF program, from
what I understand. But all the other ones is something you'd need to
make actual use of dynptr concept in real-world BPF programs.

Can we please have those as BPF helpers, and we can decide to move
slightly fancier bpf_dynptr_iterator() (and future dynptr-related
extras) into kfunc?

> helpers, but we got burned on them once and scrambled to add 'flags' argument.
> kfuncs are unstable and can be adjusted/removed at any time later.

I don't see why we would remove any of the above list ever? They are
generic and fundamental to dynptr as a concept, they can't restrict
what dynptr can do in the future.

Also GPL restriction of kfuncs doesn't apply to these dynptr helpers
either, IMO.

> The verifier now supports dynptr in kfunc verification, so conversion should
> be straightforward.
> Thanks
>
> >
> > Please note that this patchset will be rebased on top of dynptr refactoring/fixes
> > once that is landed upstream.
> >
> > [0] https://lore.kernel.org/bpf/20220523210712.3641569-1-joannelkoong@gmail.com/
> > [1] https://lore.kernel.org/bpf/20221021011510.1890852-1-joannelkoong@gmail.com/
> >