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.
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.
