Re: [PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH

From: Jiri Benc <jbenc@redhat.com>
To: Lorenz Bauer <lmb@cloudflare.com>
Cc: ast@kernel.org, daniel@iogearbox.net, sdf@google.com,
	jakub@cloudflare.com, john.fastabend@gmail.com,
	kernel-team@cloudflare.com, bpf@vger.kernel.org
Subject: Re: [PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH
Date: Wed, 4 Nov 2020 19:08:08 +0100	[thread overview]
Message-ID: <20201104190808.417b9a4b@redhat.com> (raw)
In-Reply-To: <20200629095630.7933-2-lmb@cloudflare.com>

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@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 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;
        }
	[...]
}

The mapfd is later used as attr->target_fd:

static int do_attach(int argc, char **argv)
{
	[...]
        err = bpf_prog_attach(progfd, mapfd, attach_type, 0);
	[...]
}

and rejected in the kernel by the line added by this patch. Seems that
setting flow dissector using bpftool does not work since this patch was
applied? What am I missing?

 Jiri