On Sat, 27 Aug 2022 at 19:22, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Sat, Aug 27, 2022 at 12:12 AM Kumar Kartikeya Dwivedi > <memxor@xxxxxxxxx> 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.
