On Wed, 4 Nov 2020 at 18:08, Jiri Benc <jbenc@xxxxxxxxxx> wrote:
>
> On Mon, 29 Jun 2020 10:56:25 +0100, Lorenz Bauer wrote:
> > Using BPF_PROG_ATTACH on a flow dissector program supports neither
> > target_fd, attach_flags or replace_bpf_fd but accepts any value.
> >
> > Enforce that all of them are zero. This is fine for replace_bpf_fd
> > since its presence is indicated by BPF_F_REPLACE. It's more
> > problematic for target_fd, since zero is a valid fd. Should we
> > want to use the flag later on we'd have to add an exception for
> > fd 0. The alternative is to force a value like -1. This requires
> > more changes to tests. There is also precedent for using 0,
> > since bpf_iter uses this for target_fd as well.
> >
> > Signed-off-by: Lorenz Bauer <lmb@xxxxxxxxxxxxxx>
> > 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 3e89c7ad42cb..bf18eabeaea2 100644
> > --- a/kernel/bpf/net_namespace.c
> > +++ b/kernel/bpf/net_namespace.c
> > @@ -217,6 +217,9 @@ int netns_bpf_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > struct net *net;
> > int ret;
> >
> > + if (attr->target_fd || attr->attach_flags || attr->replace_bpf_fd)
> > + return -EINVAL;
>
> I'm debugging failing test_flow_dissector.sh selftest and I wonder how
> this patch works.
>
> The test_flow_dissector.sh selftest at line 28 runs:
>
> bpftool prog -d attach pinned /sys/fs/bpf/flow/flow_dissector flow_dissector
>
> which invokes this code:
>
> static int parse_attach_detach_args(int argc, char **argv, int *progfd,
> enum bpf_attach_type *attach_type,
> int *mapfd)
> {
> [...]
> if (*attach_type == BPF_FLOW_DISSECTOR) {
> *mapfd = -1;
> return 0;
> }
> [...]
> }
Hi Jiri,
Thanks for the bug report. The commit indeed breaks attaching the flow
dissector using bpftool as you point out. I think I didn't catch this
because I don't have bpftool in my path, so that test was actually
skipped... The other parts of the test use a custom loader, which
passes 0 for the arguments and are not affected.
I had a cursory look at bpftool packaging in Debian, Ubuntu and
Fedora, it seems they all package bpftool in a kernel version
dependent package. So the most straightforward fix is probably to
change bpftool to use *mapfd = 0 and then land that via the bpf tree.
What do you think?
Lorenz
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
