On Tue, Oct 26, 2021 at 11:00 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.
> 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.
Totally agree. Adding a variant incurs exponential cost. Introducing
another dimension in future may need to go over all the MEM,
RDONLY_MEM, MEM_OR_NULL, RDONLY_MEM_OR_NULL, multiplied by ARG_*,
RET_*, etc. It's a pain.
I have that in mind and start thinking more about how can we do a more
scalable flag approach.
