Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Tue, Sep 6, 2022 at 9:40 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> On Wed, 7 Sept 2022 at 06:27, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Mon, Sep 5, 2022 at 6:14 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > > +int bpf_ct_set_nat_info(struct nf_conn___init *nfct__ref,
> > > +                       union nf_inet_addr *addr, __be16 *port,
> > > +                       enum nf_nat_manip_type manip)
> > > +{
> > ...
> > > @@ -437,6 +483,7 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
> > >  BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
> > >  BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
> > >  BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
> > > +BTF_ID_FLAGS(func, bpf_ct_set_nat_info)
> > >  BTF_SET8_END(nf_ct_kfunc_set)
> >
> > Instead of __ref and patch 1 and 2 it would be better to
> > change the meaning of "trusted_args".
> > In this case "addr" and "port" are just as "trusted".
> > They're not refcounted per verifier definition,
> > but they need to be "trusted" by the helper.
> > At the end the "trusted_args" flags would mean
> > "this helper can assume that all pointers can be safely
> > accessed without worrying about lifetime".
>
> So you mean it only forces PTR_TO_BTF_ID to have reg->ref_obj_id > 0?
>
> But suppose in the future you have a type that has scalars only.
>
> struct foo { int a; int b; ... };
> Just data, and this is acquired from a kfunc and released using another kfunc.
> Now with this new definition you are proposing, verifier ends up
> allowing PTR_TO_MEM to also be passed to such helpers for the struct
> foo *.
>
> I guess even reg->ref_obj_id check is not enough, user may also pass
> PTR_TO_MEM | MEM_ALLOC which can be refcounted.
>
> It would be easy to forget such subtle details later.

It may add headaches to the verifier side, but here we have to
think from pov of other subsystems that add kfuncs.
They shouldn't need to know the verifier details.
The internals will change anyway.
Ideally KF_TRUSTED_ARGS will become the default flag that every kfunc
will use to indicate that the function assumes valid pointers.
How the verifier recognizes them is irrelevant from kfunc pov.
People that write bpf progs are not that much different from
people that write kfuncs that bpf progs use.
Both should be easy to write.