Message-ID: <CAP01T77GxdU6AQE3ADVFZ6YA89diFFAev3aQFpYNboxM76QJ6w@mail.gmail.com>
Date:   Wed, 13 Jul 2022 14:13:17 +0200
From:   Kumar Kartikeya Dwivedi <memxor@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     bpf <bpf@...r.kernel.org>, Yonghong Song <yhs@...com>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Florian Westphal <fw@...len.de>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        netfilter-devel <netfilter-devel@...r.kernel.org>
Subject: Re: [PATCH bpf-next v5 1/8] bpf: Add support for forcing kfunc args
 to be referenced

On Thu, 7 Jul 2022 at 00:04, Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Wed, Jul 6, 2022 at 2:29 PM Alexei Starovoitov
> <alexei.starovoitov@...il.com> wrote:
> >
> > On Thu, Jul 07, 2022 at 12:51:15AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > On Thu, 7 Jul 2022 at 00:14, Alexei Starovoitov
> > > <alexei.starovoitov@...il.com> wrote:
> > > >
> > > > On Sun, Jul 03, 2022 at 11:04:22AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > > On Sun, 3 Jul 2022 at 10:54, Kumar Kartikeya Dwivedi <memxor@...il.com> wrote:
> > > > > >
> > > > > > On Wed, 29 Jun 2022 at 08:53, Alexei Starovoitov
> > > > > > <alexei.starovoitov@...il.com> wrote:
> > > > > > >
> > > > > > > On Fri, Jun 24, 2022 at 12:56:30AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > > > > > Similar to how we detect mem, size pairs in kfunc, teach verifier to
> > > > > > > > treat __ref suffix on argument name to imply that it must be a
> > > > > > > > referenced pointer when passed to kfunc. This is required to ensure that
> > > > > > > > kfunc that operate on some object only work on acquired pointers and not
> > > > > > > > normal PTR_TO_BTF_ID with same type which can be obtained by pointer
> > > > > > > > walking. Release functions need not specify such suffix on release
> > > > > > > > arguments as they are already expected to receive one referenced
> > > > > > > > argument.
> > > > > > > >
> > > > > > > > Note that we use strict type matching when a __ref suffix is present on
> > > > > > > > the argument.
> > > > > > > ...
> > > > > > > > +             /* Check if argument must be a referenced pointer, args + i has
> > > > > > > > +              * been verified to be a pointer (after skipping modifiers).
> > > > > > > > +              */
> > > > > > > > +             arg_ref = is_kfunc_arg_ref(btf, args + i);
> > > > > > > > +             if (is_kfunc && arg_ref && !reg->ref_obj_id) {
> > > > > > > > +                     bpf_log(log, "R%d must be referenced\n", regno);
> > > > > > > > +                     return -EINVAL;
> > > > > > > > +             }
> > > > > > > > +
> > > > > > >
> > > > > > > imo this suffix will be confusing to use.
> > > > > > > If I understand the intent the __ref should only be used
> > > > > > > in acquire (and other) kfuncs that also do release.
> > > > > > > Adding __ref to actual release kfunc will be a nop.
> > > > > > > It will be checked, but it's not necessary.
> > > > > > >
> > > > > > > At the end
> > > > > > > +struct nf_conn *bpf_ct_insert_entry(struct nf_conn___init *nfct__ref)
> > > > > > > will behave like kptr_xchg with exception that kptr_xchg takes any btf_id
> > > > > > > while here it's fixed.
> > > > > > >
> > > > > > > The code:
> > > > > > >  if (rel && reg->ref_obj_id)
> > > > > > >         arg_type |= OBJ_RELEASE;
> > > > > > > should probably be updated with '|| arg_ref'
> > > > > > > to make sure reg->off == 0 ?
> > > > > > > That looks like a small bug.
> > > > > > >
> > > > > >
> > > > > > Indeed, I missed that. Thanks for catching it.
> > > > > >
> > > > > > > But stepping back... why __ref is needed ?
> > > > > > > We can add bpf_ct_insert_entry to acq and rel sets and it should work?
> > > > > > > I'm assuming you're doing the orthogonal cleanup of resolve_btfid,
> > > > > > > so we will have a single kfunc set where bpf_ct_insert_entry will
> > > > > > > have both acq and rel flags.
> > > > > > > I'm surely missing something.
> > > > > >
> > > > > > It is needed to prevent the case where someone might do:
> > > > > > ct = bpf_xdp_ct_alloc(...);
> > > > > > bpf_ct_set_timeout(ct->master, ...);
> > > > > >
> > > > >
> > > > > A better illustration is probably bpf_xdp_ct_lookup and
> > > > > bpf_ct_change_timeout, since here the type for ct->master won't match
> > > > > with bpf_ct_set_timeout, but the point is the same.
> > > >
> > > > Sorry, I'm still not following.
> > > > Didn't we make pointer walking 'untrusted' so ct->master cannot be
> > > > passed into any kfunc?
> > > >
> > >
> > > I don't believe that is the case, it is only true for kptrs loaded
> > > from BPF maps (that too those with BPF_LDX, not the ones with
> > > kptr_xchg). There we had a chance to do things differently. For normal
> > > PTR_TO_BTF_ID obtained from kfuncs/BPF helpers, there is no untrusted
> > > flag set on them, nor is it set when walking them.
> > >
> > > I also think we discussed switching to this mode, by making many cases
> > > untrusted by default, and using annotation to allow cases, making
> > > pointers trusted at one level (like args for tracing/lsm progs, but
> > > next deref becomes untrusted), but admittedly it may not cover enough
> > > ground, and you didn't like it much either, so I stopped pursuing it.
> >
> > Ahh. Now I remember. Thanks for reminding :)
> > Could you please summarize this thread and add all of it as a big comment
> > in the source code next to __ref handling to explain the motivation
> > and an example on when and how this __ref suffix should be used.
> > Otherwise somebody, like me, will forget the context soon.
> >
> > I was thinking of better name than __ref, but couldn't come up with one.
> > __ref fits this use case the best.
>
> Actually, maybe a kfunc flag will be better?
> Like REF_ARGS
> that would apply to all arguments of the kfunc
> (not only those with __ref suffix).
>
> We have three types of ptr_btf_id:
> - ref counted
> - untrusted
> - old legacy that we cannot be break due to backward compat
>
> In the future we'll probably be adding new kfuncs where we'd want
> every argument to be trusted. In our naming convention these are
> the refcounted ptr_to_btf_id that come from lookup-like kfuncs.
> To consume them in the release kfunc they have to be refcounted,
> but non-release kfunc (like set_timeout) also want a trusted ptr.
> So the simple way of describe the intent would be:
> BTF_ID(func, bpf_ct_release, RELEASE)
> BTF_ID(func, bpf_ct_set_timeout, REF_ARGS)
>
> or maybe TRUSTED_ARGS would be a better flag name.
> wdyt?

Ok, I've implemented the kfunc flags and kept TRUSTED_ARGS as the
name. Just need to do a little bit of testing and will post it
together with this.

Just to confirm, should I still keep __ref or drop it? I think
TRUSTED_ARGS has its use but it may be too coarse. I already have the
patch so if you like we can add both ways now.

Powered by blists - more mailing lists