Re: [PATCH bpf 1/2] flow_dissector: reject invalid attach_flags

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

On Mon, Jun 15, 2020 at 7:43 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> On Fri, 12 Jun 2020 at 23:36, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Fri, Jun 12, 2020 at 9:02 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > >
> > > Using BPF_PROG_ATTACH on a flow dissector program supports neither flags
> > > nor target_fd but accepts any value. Return EINVAL if either are non-zero.
> > >
> > > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > > Fixes: b27f7bb590ba ("flow_dissector: Move out netns_bpf prog callbacks")
> > > ---
> > >  kernel/bpf/net_namespace.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/kernel/bpf/net_namespace.c b/kernel/bpf/net_namespace.c
> > > index 78cf061f8179..56133e78ae4f 100644
> > > --- a/kernel/bpf/net_namespace.c
> > > +++ b/kernel/bpf/net_namespace.c
> > > @@ -192,6 +192,9 @@ int netns_bpf_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > >         struct net *net;
> > >         int ret;
> > >
> > > +       if (attr->attach_flags || attr->target_fd)
> > > +               return -EINVAL;
> > > +
> >
> > In theory it makes sense, but how did you test it?
>
> Not properly it seems, sorry!
>
> > test_progs -t flow
> > fails 5 tests.
>
> I spent today digging through this, and the issue is actually more annoying than
> I thought. BPF_PROG_DETACH for sockmap and flow_dissector ignores
> attach_bpf_fd. The cgroup and lirc2 attach point use this to make sure that the
> program being detached is actually what user space expects. We actually have
> tests that set attach_bpf_fd for these to attach points, which tells
> me that this is
> an easy mistake to make.
>
> Unfortunately I can't come up with a good fix that seems backportable:
> - Making sockmap and flow_dissector have the same semantics as cgroup
>   and lirc2 requires a bunch of changes (probably a new function for sockmap)

making flow dissector pass prog_fd as cg and lirc is certainly my preference.
Especially since tests are passing fd user code is likely doing the same,
so breakage is unlikely. Also it wasn't done that long ago, so
we can backport far enough.
It will remove cap_net_admin ugly check in bpf_prog_detach()
which is the only exception now in cap model.