netdev - Re: [PATCH bpf-next 2/2] bpftool: show flow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 24 Apr 2019 09:12:47 +0100
From:   Quentin Monnet <quentin.monnet@...ronome.com>
To:     Stanislav Fomichev <sdf@...gle.com>, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Cc:     davem@...emloft.net, ast@...nel.org, daniel@...earbox.net,
        jakub.kicinski@...ronome.com
Subject: Re: [PATCH bpf-next 2/2] bpftool: show flow_dissector attachment
 status

2019-04-23 16:22 UTC-0700 ~ Stanislav Fomichev <sdf@...gle.com>
> Right now there is no way to query whether BPF flow_dissector program
> is attached to a network namespace or not. In previous commit, I added
> support for querying that info, show it when doing `bpftool net`:
> 
> $ bpftool prog loadall ./bpf_flow.o \
> 	/sys/fs/bpf/flow type flow_dissector \
> 	pinmaps /sys/fs/bpf/flow
> $ bpftool prog
> 3: flow_dissector  name _dissect  tag 8c9e917b513dd5cc  gpl
>          loaded_at 2019-04-23T16:14:48-0700  uid 0
>          xlated 656B  jited 461B  memlock 4096B  map_ids 1,2
>          btf_id 1
> ...
> 
> $ bpftool net -j
> [{"xdp":[],"tc":[],"flow_dissector":[]}]
> 
> $ bpftool prog attach pinned \
> 	/sys/fs/bpf/flow/flow_dissector flow_dissector
> $ bpftool net -j
> [{"xdp":[],"tc":[],"flow_dissector":["id":3]}]
> 
> Doesn't show up in a different net namespace:
> $ ip netns add test
> $ ip netns exec test bpftool net -j
> [{"xdp":[],"tc":[],"flow_dissector":[]}]
> 
> Non-json output:
> $ bpftool net
> xdp:
> 
> tc:
> 
> flow_dissector:
> id 3
> 
> Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> ---
>   tools/bpf/bpftool/net.c | 52 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 52 insertions(+)
> 
> diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> index db0e7de49d49..afe0903201e2 100644
> --- a/tools/bpf/bpftool/net.c
> +++ b/tools/bpf/bpftool/net.c

> @@ -48,6 +51,10 @@ struct bpf_filter_t {
>   	int		ifindex;
>   };
>   
> +struct bpf_attach_info {
> +	__u32 flow_dissector_id;
> +};
> +
>   static int dump_link_nlmsg(void *cookie, void *msg, struct nlattr **tb)
>   {
>   	struct bpf_netdev_t *netinfo = cookie;
> @@ -180,8 +187,43 @@ static int show_dev_tc_bpf(int sock, unsigned int nl_pid,
>   	return 0;
>   }
>   
> +static int query_flow_dissector(struct bpf_attach_info *attach_info)
> +{
> +	__u32 prog_ids[1] = {0};
> +	__u32 prog_cnt = ARRAY_SIZE(prog_ids);
> +	__u32 attach_flags;
> +	int fd;
> +	int err;
> +
> +	fd = open("/proc/self/ns/net", O_RDONLY);
> +	if (fd < 0) {
> +		p_err("can't open /proc/self/ns/net: %d",
> +		      strerror(errno));
> +		return -1;
> +	}
> +	err = bpf_prog_query(fd, BPF_FLOW_DISSECTOR, 0,
> +			     &attach_flags, prog_ids, &prog_cnt);
> +	close(fd);
> +	if (err) {
> +		if (errno == EINVAL) {
> +			/* Older kernel's don't support querying
> +			 * flow dissector programs.
> +			 */
> +			return 0;

Hi Stanislav,

If we handle the "error" gracefully here, should we maybe reset errno to 
0 before returning? Batch mode, for example, stops processing commands 
when it sees that errno is set, but we probably want it to continue in 
the current case (commit 39c9f10639a3 addressed something similar for 
"bpftool cgroup tree").

> +		}
> +		p_err("can't query prog: %s", strerror(errno));
> +		return -1;
> +	}
> +
> +	if (prog_cnt == 1)
> +		attach_info->flow_dissector_id = prog_ids[0];
> +
> +	return 0;
> +}
> +
>   static int do_show(int argc, char **argv)
>   {
> +	struct bpf_attach_info attach_info = {};
>   	int i, sock, ret, filter_idx = -1;
>   	struct bpf_netdev_t dev_array;
>   	unsigned int nl_pid;