linux-kernel - Re: [v2 PATCH bpf-next 2/4] bpftool: add net attach/detach command to tcx prog

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]

Message-ID: <fa9bb6d5-7a72-4d77-8862-d8489a759506@kernel.org>
Date: Fri, 19 Jul 2024 17:17:20 +0100
From: Quentin Monnet <qmo@...nel.org>
To: Tao Chen <chen.dylane@...il.com>, Alexei Starovoitov <ast@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
 Martin KaFai Lau <martin.lau@...ux.dev>
Cc: Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>,
 Yonghong Song <yonghong.song@...ux.dev>,
 John Fastabend <john.fastabend@...il.com>, bpf@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [v2 PATCH bpf-next 2/4] bpftool: add net attach/detach command to
 tcx prog

On 17/07/2024 18:47, Tao Chen wrote:
> Now, attach/detach tcx prog supported in libbpf, so we can add new
> command 'bpftool attach/detach tcx' to attach tcx prog with bpftool
> for user.
> 
>  # bpftool prog load tc_prog.bpf.o /sys/fs/bpf/tc_prog
>  # bpftool prog show
> 	...
> 	192: sched_cls  name tc_prog  tag 187aeb611ad00cfc  gpl
> 	loaded_at 2024-07-11T15:58:16+0800  uid 0
> 	xlated 152B  jited 97B  memlock 4096B  map_ids 100,99,97
> 	btf_id 260
>  # bpftool net attach tcx_ingress name tc_prog dev lo
>  # bpftool net
> 	...
> 	tc:
> 	lo(1) tcx/ingress tc_prog prog_id 29
> 
>  # bpftool net detach tcx_ingress dev lo
>  # bpftool net
> 	...
> 	tc:
>  # bpftool net attach tcx_ingress name tc_prog dev lo
>  # bpftool net
> 	tc:
> 	lo(1) tcx/ingress tc_prog prog_id 29
> 
> Test environment: ubuntu_22_04, 6.7.0-060700-generic
> 
> Signed-off-by: Tao Chen <chen.dylane@...il.com>
> ---
>  tools/bpf/bpftool/net.c | 43 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> index 1b9f4225b394..60b0af40109a 100644
> --- a/tools/bpf/bpftool/net.c
> +++ b/tools/bpf/bpftool/net.c
> @@ -67,6 +67,8 @@ enum net_attach_type {
>  	NET_ATTACH_TYPE_XDP_GENERIC,
>  	NET_ATTACH_TYPE_XDP_DRIVER,
>  	NET_ATTACH_TYPE_XDP_OFFLOAD,
> +	NET_ATTACH_TYPE_TCX_INGRESS,
> +	NET_ATTACH_TYPE_TCX_EGRESS,
>  };
>  
>  static const char * const attach_type_strings[] = {
> @@ -74,6 +76,8 @@ static const char * const attach_type_strings[] = {
>  	[NET_ATTACH_TYPE_XDP_GENERIC]	= "xdpgeneric",
>  	[NET_ATTACH_TYPE_XDP_DRIVER]	= "xdpdrv",
>  	[NET_ATTACH_TYPE_XDP_OFFLOAD]	= "xdpoffload",
> +	[NET_ATTACH_TYPE_TCX_INGRESS]	= "tcx_ingress",
> +	[NET_ATTACH_TYPE_TCX_EGRESS]	= "tcx_egress",
>  };
>  
>  static const char * const attach_loc_strings[] = {
> @@ -647,6 +651,32 @@ static int do_attach_detach_xdp(int progfd, enum net_attach_type attach_type,
>  	return bpf_xdp_attach(ifindex, progfd, flags, NULL);
>  }
>  
> +static int get_tcx_type(enum net_attach_type attach_type)
> +{
> +	switch (attach_type) {
> +	case NET_ATTACH_TYPE_TCX_INGRESS:
> +		return BPF_TCX_INGRESS;
> +	case NET_ATTACH_TYPE_TCX_EGRESS:
> +		return BPF_TCX_EGRESS;
> +	default:
> +		return __MAX_BPF_ATTACH_TYPE;


Can we return -1 here instead, please? In the current code, we validate
the attach_type before entering this function and the "default" case is
never reached, it's only here to discard compiler's warning. But if we
ever reuse this function elsewhere, this could cause bugs: if the header
file used for compiling the bpftool binary is not in sync with the
header corresponding to the running kernel, __MAX_BPF_ATTACH_TYPE could
correspond to a newly introduced type, and bpftool/libbpf would try to
use that to attach the program, instead of detecting an error.


> +	}
> +}
> +
> +static int do_attach_tcx(int progfd, enum net_attach_type attach_type, int ifindex)
> +{
> +	int type = get_tcx_type(attach_type);
> +
> +	return bpf_prog_attach(progfd, ifindex, type, 0);
> +}
> +
> +static int do_detach_tcx(int targetfd, enum net_attach_type attach_type)
> +{
> +	int type = get_tcx_type(attach_type);
> +
> +	return bpf_prog_detach(targetfd, type);
> +}
> +
>  static int do_attach(int argc, char **argv)
>  {
>  	enum net_attach_type attach_type;
> @@ -692,6 +722,11 @@ static int do_attach(int argc, char **argv)
>  	case NET_ATTACH_TYPE_XDP_OFFLOAD:
>  		err = do_attach_detach_xdp(progfd, attach_type, ifindex, overwrite);
>  		break;
> +	/* attach tcx prog */
> +	case NET_ATTACH_TYPE_TCX_INGRESS:
> +	case NET_ATTACH_TYPE_TCX_EGRESS:
> +		err = do_attach_tcx(progfd, attach_type, ifindex);
> +		break;
>  	default:
>  		break;
>  	}
> @@ -738,6 +773,11 @@ static int do_detach(int argc, char **argv)
>  		progfd = -1;
>  		err = do_attach_detach_xdp(progfd, attach_type, ifindex, NULL);
>  		break;
> +	/* detach tcx prog */
> +	case NET_ATTACH_TYPE_TCX_INGRESS:
> +	case NET_ATTACH_TYPE_TCX_EGRESS:
> +		err = do_detach_tcx(ifindex, attach_type);
> +		break;
>  	default:
>  		break;
>  	}
> @@ -944,7 +984,8 @@ static int do_help(int argc, char **argv)
>  		"       %1$s %2$s help\n"
>  		"\n"
>  		"       " HELP_SPEC_PROGRAM "\n"
> -		"       ATTACH_TYPE := { xdp | xdpgeneric | xdpdrv | xdpoffload }\n"
> +		"       ATTACH_TYPE := { xdp | xdpgeneric | xdpdrv | xdpoffload | tcx_ingress\n"
> +		"			 | tcx_egress }\n"

                 ^^^^^^^^^^^^^^^^^^^^^^^
This indent space between the quote and the pipe needs to be spaces
instead of three tabs, please.

The rest of the patches looks good, thank you!

Quentin