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@xxxxxxxxx>
> ---
> 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
