Re: [PATCH bpf-next v4 1/3] bpf: Add skb dynptrs

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Sun, 28 Aug 2022 at 01:03, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Sat, Aug 27, 2022 at 11:33 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Sat, 27 Aug 2022 at 19:22, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Sat, Aug 27, 2022 at 12:12 AM Kumar Kartikeya Dwivedi
> > > <memxor@gmail.com> wrote:
> > > > [...]
> > > > >
> > > > > I think the right answer here is to not make bpf_dynptr_data() return
> > > > > direct pointer of changing read-only-ness. Maybe the right answer here
> > > > > is another helper, bpf_dynptr_data_rdonly(), that will return NULL for
> > > > > non-read-only dynptr and PTR_TO_MEM | MEM_RDONLY if dynptr is indeed
> > > > > read-only?
> > > >
> > > > Shouldn't it be the other way around? bpf_dynptr_data_rdonly() should
> > > > work for both ro and rw dynptrs, and bpf_dynptr_data() only for rw
> > > > dynptr?
> > >
> > > Right, that's what I proposed:
> > >
> > >   "bpf_dynptr_data_rdonly(), that will return NULL for non-read-only dynptr"
> > >
> > > so if you pass read-write dynptr, it will return NULL (because it's
> > > unsafe to take writable direct pointer).
> > >
> > > bpf_dynptr_data_rdonly() should still work fine with both rdonly and
> > > read-write dynptr.
> > > bpf_dynptr_data() only works (in the sense returns non-NULL) for
> > > read-write dynptr only.
> > >
> > >
> > > >
> > > > And yes, you're kind of painting yourself in a corner if you allow
> > > > dynptr_data to work with statically ro dynptrs now, ascertaining the
> > > > ro bit for the returned slice, and then later you plan to add dynptrs
> > > > whose ro vs rw is not known to the verifier statically. Then it works
> > > > well for statically known ones (returning both ro and rw slices), but
> > > > has to return NULL at runtime for statically unknown read only ones,
> > > > and always rw slice in verifier state for them.
> > >
> > > Right, will be both inconsistent and puzzling.
> > >
> > > >
> > > > >
> > > > > By saying that read-only-ness of dynptr should be statically known and
> > > > > rejecting some dynptr functions at load time places us at the mercy of
> > > > > verifier's complete knowledge of application logic, which is exactly
> > > > > against the spirit of dynptr.
> > > > >
> > > >
> > > > Right, that might be too restrictive if we require them to be
> > > > statically read only.
> > > >
> > > > But it's not about forcing it to be statically ro, it is more about
> > > > rejecting load when we know the program is incorrect (e.g. the types
> > > > are incorrect when passed to helpers), otherwise we throw the error at
> > > > runtime anyway, which seems to be the convention afaicu. But maybe I
> > > > missed the memo and we gradually want to move away from such strict
> > > > static checks.
> > > >
> > > > I view the situation here similar to if we were rejecting direct
> > > > writes to PTR_TO_MEM | MEM_RDONLY at load time, but offloading as
> > > > runtime check in the helper writing to it as rw memory arg. It's as if
> > > > we pretend it's part of the 'type' of the register when doing direct
> > > > writes, but then ignore it while matching it against the said helper's
> > > > argument type.
> > >
> > > I disagree, it's not the same. bpf_dynptr_data/bpf_dynptr_data_rdonly
> > > turns completely dynamic dynptr into static slice of memory. Only
> > > after that point it makes sense for BPF verifier to reject something.
> > > Until then it's not incorrect. BPF program will always have to deal
> > > with some dynptr operations potentially failing. dynptr can always be
> > > NULL internally, you can still call bpf_dynptr_xxx() operations on it,
> > > they will just do nothing and return error. That doesn't make BPF
> > > program incorrect.
> >
> > Let me just explain one last time why I'm unable to swallow this
> > 'completely dynamic dynptr' explanation, because it is not treated as
> > completely dynamic by all dynptr helpers.
> >
> > No pushback, but it would be great if you could further help me wrap
> > my head around this, so that we're in sync for future discussions.
> >
> > So you say you may not know the type of dynptr (read-only, rw, local,
> > ringbuf, etc.). Hence you want to treat every dynptr as 'dynamic
>
> No, that's not what I said. I didn't talk about dynptr type (ringbuf
> vs skb vs local). Type we have to track statically precisely to limit
> what kind of helpers can be used on dynptrs of specific type.
>
> But note that there are helpers that don't care about dynptr and, in
> theory, should work with any dynptr. Because dynptr is an abstraction
> of "logically continuous range of memory". So in some cases even types
> don't matter.
>
> But regardless, I just don't think about read-only bit as part of
> dynptr types. I don't see why it has to be statically known and even
> why it has to stay as read-only or read-write for the entire duration
> of dynptr lifetime. Why can't we allow flipping dynptr from read-write
> to read-only before passing it to some custom BPF subprog, potentially
> provided from some BPF library which you don't control; just to make
> sure that it cannot really modify underlying memory (even if it is
> fundamentally modifiable, like with ringbuf)?
>
> So the point I'm trying to communicate is that things that don't
> **need** to be knowable statically to BPF verifier - **should not** be
> knowable and should be checked at runtime only. With any dynptr helper
> that is interfacing its runtime nature into static thing that verifier
> enforces (which is what bpf_dynptr_data/bpf_dynptr_data_rdonly are),
> it will always be "fallible" and users will have to expect that they
> might return NULL or do nothing, or whatever way to deal with failure
> case.
>
>
> And I believe dynptr read-onliness is not something that BPF verifier
> should be tracking statically. PTR_TO_MEM -- yes, dynptr -- no. That's
> it. Dynptr type/kind -- yes, track statically, it's way more important
> to determine what you can at all do with dynptr.
>

Understood, it's more clear now how you're thinking about this :),
especially that you don't consider read-only bit to be a property of
dynptr type. I think that was the main point I was not following.

And yes, I guess it's about different tradeoffs. Taking your BPF
library example, my idea would be that we will anyway be verifying the
global or static subprog in the library by the BTF of its function
prototype. The verifier has to verify safety of the subprog by setting
up argument types if it is global.

There, I find it more appropriate for global ones to declare const
bpf_dynptr * as the argument type if the intent is to not write to the
dynptr. Then user can safely pass both rw and ro dynptrs to it,
without having to flip it each time at runtime just to ensure safety.
If it is static it will also be able to catch incorrect writes for
callers.

But indeed there may be cases where you want to control this at
runtime, IMPO that should be orthogonal to type level const-ness.
Those will be rw at the type level but may change const-ness at
runtime, then operations start failing at runtime.

Anyway, I guess we are done here. Again, thanks for sticking along and
taking the time to describe it in detail :).

>
> [...]