On Tue, Oct 26, 2021 at 11:53 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Oct 26, 2021 at 10:51 AM Hao Luo <haoluo@xxxxxxxxxx> wrote:
> >
> > On Mon, Oct 25, 2021 at 10:06 PM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> > >
> > > On Mon, Oct 25, 2021 at 4:13 PM Hao Luo <haoluo@xxxxxxxxxx> wrote:
> > > >
> > > > Some helper functions may modify its arguments, for example,
> > > > bpf_d_path, bpf_get_stack etc. Previously, their argument types
> > > > were marked as ARG_PTR_TO_MEM, which is compatible with read-only
> > > > mem types, such as PTR_TO_RDONLY_BUF. Therefore it's legitimate
> > > > to modify a read-only memory by passing it into one of such helper
> > > > functions.
> > > >
> > > > This patch introduces a new arg type ARG_PTR_TO_WRITABLE_MEM to
> > > > annotate the arguments that may be modified by the helpers. For
> > > > arguments that are of ARG_PTR_TO_MEM, it's ok to take any mem type,
> > > > while for ARG_PTR_TO_WRITABLE_MEM, readonly mem reg types are not
> > > > acceptable.
> > > >
> > > > In short, when a helper may modify its input parameter, use
> > > > ARG_PTR_TO_WRITABLE_MEM instead of ARG_PTR_TO_MEM.
> > >
> > > This is inconsistent with the other uses where we have something
> > > that's writable by default and mark it as RDONLY if it's read-only.
> > > Same here, why not keep ARG_PTR_TO_MEM to mean "writable memory", and
> > > add ARG_PTR_TO_RDONLY_MEM for helpers that are not writing into the
> > > memory? It will also be safer default: if helper defines
> > > ARG_PTR_TO_MEM but never writes to it, worst thing that can happen
> > > would be that you won't be able to pass REG_PTR_TO_RDONLY_MEM into it
> > > without fixing helper definition. The other way is more dangerous. If
> > > ARG_PTR_TO_MEM means read-only mem and helper forgot this distinction
> > > and actually writes into the memory, then we are in much bigger
> > > trouble.
> > >
> >
> > My original implementation was adding ARG_PTR_TO_RDONLY_MEM. But I
> > find it's not intuitive for developers when adding helpers. The
> > majority of PTR_TO_MEM arguments are read-only. When adding a new
> > helper, I would expect the default arg type (that is, ARG_PTR_TO_MEM)
> > to match the default case (that is, read-only argument). Requiring
> > explicitly adding RDONLY to most cases seems a little unintuitive to
> > me.
> >
> > But other than that, I agree that narrowing ARG_PTR_TO_MEM down to
> > writable memory fosters more strict checks and safer behavior. But
> > when people add helpers, they are clearly aware which argument will be
> > modified and which will not. IMHO I do trust that the developers and
> > the reviewers can choose the right type at the review time. :)
>
> I don't trust myself, and neither should you :) See 5b029a32cfe4
> ("bpf: Fix ringbuf helper function compatibility") as an example of
> the things that shouldn't have happened, but slipped through my own
> testing and code review anyway. And there were multiple cases where we
> accidentally enabled stuff that we shouldn't or didn't check something
> that should have been prevented.
>
> All that is to say that if we can have safer behavior by default (not
> as enforced by humans), then it's better. In this sense,
> ARG_PTR_TO_MEM meaning writable access is safer, because even if we
> accidentally forget to mark some input as ARG_PTR_TO_RDONLY_MEM, worst
> thing is that users won't be able to use helper in some situation and
> hopefully will report this and we'll fix it. The alternative is that a
> helper declares the argument as read-only memory (accidentally,
> because it's shorter enum and sort of default), but actually does the
> write to that memory. Now we have a much bigger issue.
>
Acknowledged. Will make this change in this next iteration.
> >
> > > >
> > > > So far the difference between ARG_PTR_TO_MEM and ARG_PTR_TO_WRITABLE_MEM
> > > > is PTR_TO_RDONLY_BUF and PTR_TO_RDONLY_MEM. PTR_TO_RDONLY_BUF is
> > > > only used in bpf_iter prog as the type of key, which hasn't been
> > > > used in the affected helper functions. PTR_TO_RDONLY_MEM currently
> > > > has no consumers.
> > > >
> > > > Signed-off-by: Hao Luo <haoluo@xxxxxxxxxx>
> > > > ---
> > > > Changes since v1:
> > > > - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate
> > > > read-only and read-write mem arg types.
> > > >
>
> [...]
