On Wed, Jan 04, 2023 at 10:44:07AM -0800, Andrii Nakryiko wrote:
> >
> > Agree that any hard policy like 'only kfuncs from now on' gotta have its limits.
> > Maybe there will be a strong reason to add a new helper one day,
> > so we can keep the door open a tiny bit for an exception,
> > but for dynptr...
> > There are kfuncs with dynptr already (bpf_verify_pkcs7_signature)
> > So precedent is already made.
>
> bpf_verify_pkcs7_signature() is using dynptr as a pointer to memory.
> It's a totally valid and intended use case, to pass memory area of
> statically unknown size, yes.
>
> But that's very different from having basic dynptr helpers like
> is_null() and trim/advance as kfunc. Such helpers are stable, they
> manipulate generic attributes of dynptr: size, offset, underlying
> memory pointer. There is nothing unstable and potentially changing
> about them.
dynptr is defined in uapi as:
struct bpf_dynptr {
__u64 :64;
__u64 :64;
} __attribute__((aligned(8)));
So sizes, offset and memory pointer are not stable today and
there is no need to stabilize this part of it.
> From original exchange:
>
> > > > So just because there is no perfect way to
> > > > handle all the SKB/XDP physical non-contiguity, doesn't mean that the
> > > > dynptr concept itself is flawed or not well thought out. It's just
> > >
> > > I think that's exactly what it means. dynptr concept is flawed.
>
> Must be a lot of typos in here ;) because as written it clearly states
> that the whole concept of dynptr is flawed.
Maybe will we realize a year from now that it is?
We have some uapi exposure of dynptr in uapi. I think it's a safer bet
to keep it to the minimum.
