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.
