Message-ID: <CAP01T74UojWGWk=1nbE8N=fM9-vzyJJv=qNMM5dJO7A5qO7S6g@mail.gmail.com>
Date:   Sat, 27 Aug 2022 20:32:23 +0200
From:   Kumar Kartikeya Dwivedi <memxor@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Joanne Koong <joannelkoong@...il.com>, bpf@...r.kernel.org,
        andrii@...nel.org, daniel@...earbox.net, ast@...nel.org,
        kafai@...com, kuba@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v4 1/3] bpf: Add skb dynptrs

On Sat, 27 Aug 2022 at 19:22, Andrii Nakryiko <andrii.nakryiko@...il.com> wrote:
>
> On Sat, Aug 27, 2022 at 12:12 AM Kumar Kartikeya Dwivedi
> <memxor@...il.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
dynptr' you know nothing about even when you do know some information
about it statically (e.g. if it's on stack). You don't want to reject
things early at load even if you have all the info to do so. You want
operations on it to fail at runtime instead.

If you cannot track ro vs rw in the future statically, you won't be be
able to track local or ringbuf or skb either (since both are part of
the type of the dynptr). If you can, you can just as well encode
const-ness as part of the type where you declare it (e.g. in a map
field where the value is assigned dynamically, where you say
dynptr_ringbuf etc.). Type comprises local vs skb vs ringbuf | ro vs
rw for me. But maybe I could be wrong.

So following this line of reasoning, will you be relaxing the argument
type of helpers like bpf_ringbuf_submit_dynptr? Right now they take
'dynamic dynptr' as arg, but also expect DYNPTR_TYPE_RINGBUF, so you
reject load when it's not a ringbuf dynptr. Will it then fallback to
checking the type at runtime when the type will not be known? But then
it will also permit passing local or skb dynptr in the future which it
rejects right now.

I'm just hoping you are able to see why it's looking a bit
inconsistent to me. If DYNPTR_TYPE_RINGBUF has to be checked, it felt
to me like DYNPTR_RDONLY is as much part of that kind of type safety
wrt helpers. It would be set on the dynptr when skb passed to
dynptr_from_skb is rdonly in some program types, along with
DYNPTR_TYPE_SKB, and expect to be matched when invoking helpers on
such dynptrs.

It is fine to push checks to runtime, especially when you won't know
the type, because verifier cannot reasonably track the dynptr type
then.

But right now there's still some level of state you maintain in the
verifier about dynptrs (like it's type), and it seems to me like some
helpers are using that state to reject things at load time, while some
other helper will ignore it and fallback to runtime checks.

I hope this is a clear enough description to atleast justify why I'm
(still) a bit confused.

Powered by blists - more mailing lists