On Tue, Oct 26, 2021 at 11:45 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Oct 26, 2021 at 10:59 AM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Mon, Oct 25, 2021 at 10:14 PM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> > > >
> > > > Instead of adding new types,
> > > > can we do something like this instead:
> > > >
> > > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > > > index c8a78e830fca..5dbd2541aa86 100644
> > > > --- a/include/linux/bpf_verifier.h
> > > > +++ b/include/linux/bpf_verifier.h
> > > > @@ -68,7 +68,8 @@ struct bpf_reg_state {
> > > > u32 btf_id;
> > > > };
> > > >
> > > > - u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */
> > > > + u32 rd_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */
> > > > + u32 wr_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */
> > >
> > > This seems more confusing, it's technically possible to express a
> > > memory pointer from which you can read X bytes, but can write Y bytes.
> >
> > I'm fine it being a new flag instead of wr_mem_size.
> >
> > > I actually liked the idea that helpers will be explicit about whether
> > > they can write into a memory or only read from it.
> > >
> > > Apart from a few more lines of code, are there any downsides to having
> > > PTR_TO_MEM vs PTR_TO_RDONLY_MEM?
> >
> > because it's a churn and non scalable long term.
> > It's not just PTR_TO_RDONLY_MEM.
> > It's also ARG_PTR_TO_RDONLY_MEM,
> > and RET_PTR_TO_RDONLY_MEM,
> > and PTR_TO_RDONLY_MEM_OR_NULL
> > and *_OR_BTF_ID,
> > and *_OR_BTF_ID_OR_NULL.
> > It felt that expressing readonly-ness as a flag in bpf_reg_state
> > will make it easier to get right in the code and extend in the future.
>
> That's true, but while it's easy to add a flag to bpf_reg_state, it's
> not easy to do the same for BPF helper input (ARG_PTR_xxx) and output
> (RET_PTR_xxx) restrictions. So unless we extend ARG_PTR and RET_PTR
> with flags, it seems more consistent to keep the same pure enum
> approach for reg_state.
>
> > May be we will have a kernel vs user flag for PTR_TO_MEM in the future.
> > If we use different name to express that we will have:
> > PTR_TO_USER_RDONLY_MEM and
> > PTR_TO_USER_MEM
> > plus all variants of ARG_* and RET_* and *_OR_NULL.
> > With a flag approach it will be just another flag in bpf_reg_state.
>
> All true, but then maybe we should rethink how we do all those enums.
> And instead of having all the _OR_NULL variants, it should be
> ARG_NULLABLE/REG_NULLABLE/RET_NULLABLE flag that can be or-ed with the
> basic set of register/input/output type enums? Same for ARG_RDONLY
> flag. Same could technically be done for USER vs KERNEL memory in the
> future.
Exactly. OR_NULL is such a flag and we already struggled to
differentiate that flag with truly_not_equal_to_NULL and may_be_NULL.
That's why all bpf_skc* helpers have additional run-time !NULL check.
ARG_NULLABLE/REG_NULLABLE/RET_NULLABLE would make it cleaner.
And ARG_RDONLY would fit that model well.
> It's definitely a bunch of code changes, but if we are worried about
> an explosion of enum values, it might be the right move?
>
> On the other hand, if there are all those different variations and
> each is handled slightly differently, we'll have to have different
> logic for each of them. And whether it's an enum + flags, or a few
> more enumerators, doesn't change anything fundamentally. I feel like
> enums make code discovery a bit simpler in practice, but it's
> subjective.
I think it's a bit of a mess already.
ARG_PTR_TO_BTF_ID has may_be_NULL flag.
Just like ARG_PTR_TO_BTF_ID_SOCK_COMMON.
but RET_PTR_TO_BTF_ID doesn't.
PTR_TO_BTF_ID doesn't have that may_be_NULL assumption either.
imo cleaning up OR_NULL will be very nice.
RDONLY would be an addition on top.
We can probably fold UNINIT as a flag too.
All that will be a big change, but I think it's worth it.
