| 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
| ||
|
Message-ID: <43c23468-530b-45f3-af22-f03484e5148c@kernel.org>
Date: Sat, 1 Nov 2025 16:21:37 +0000
From: Quentin Monnet <qmo@...nel.org>
To: Gyutae Bae <gyutae.bae@...ercorp.com>, bpf@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman
<eddyz87@...il.com>, Song Liu <song@...nel.org>,
Yonghong Song <yonghong.song@...ux.dev>,
John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>,
Jiri Olsa <jolsa@...nel.org>, Siwan Kim <siwan.kim@...ercorp.com>,
Daniel Xu <dxu@...uu.xyz>, Jiayuan Chen <jiayuan.chen@...ux.dev>,
Tao Chen <chen.dylane@...ux.dev>, Kumar Kartikeya Dwivedi <memxor@...il.com>
Subject: Re: [PATCH] bpftool: Add 'head' option for tcx attach to insert at
chain start
2025-10-23 18:31 UTC+0900 ~ Gyutae Bae <gyutae.bae@...ercorp.com>
> Add support for the 'head' option when attaching tcx_ingress and
> tcx_egress programs. This option allows inserting a BPF program at
> the beginning of the TCX chain instead of appending it at the end.
>
> The implementation queries the first program ID in the chain and uses
> BPF_F_BEFORE flag with the relative_id to insert the new program before
> the existing first program. If the chain is empty, the program is simply
> attached normally.
>
> This change includes:
> - Add get_first_tcx_prog_id() helper to retrieve the first program ID
> - Modify do_attach_tcx() to support head insertion using BPF_F_BEFORE
> - Update documentation to describe the new 'head' option
> - Add bash completion support for the 'head' option on tcx attach types
> - Add example usage in the documentation
>
> The 'head' option is only valid for tcx_ingress and tcx_egress attach
> types. For XDP attach types, the existing 'overwrite' option remains
> available.
>
> Example usage:
> # bpftool net attach tcx_ingress name tc_prog dev lo head
>
> This feature is useful when the order of program execution in the TCX
> chain matters and users need to ensure certain programs run first.
Hi, thank you for this patch!
Sorry for the delay, your message was filtered as spam for some reason
and I just found it today; I note that I can't find it on Patchwork or
on Lore either, so you'll have to resend it. I also have some comments,
inline below.
>
> Co-developed-by: Siwan Kim <siwan.kim@...ercorp.com>
> Signed-off-by: Siwan Kim <siwan.kim@...ercorp.com>
> Signed-off-by: Gyutae Bae <gyutae.bae@...ercorp.com>
> ---
> .../bpf/bpftool/Documentation/bpftool-net.rst | 17 ++++++-
> tools/bpf/bpftool/bash-completion/bpftool | 9 +++-
> tools/bpf/bpftool/net.c | 51 +++++++++++++++++--
> 3 files changed, 70 insertions(+), 7 deletions(-)
>
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-net.rst b/tools/bpf/bpftool/Documentation/bpftool-net.rst
> index a9ed8992800f..e161039a7d1e 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-net.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-net.rst
> @@ -24,7 +24,7 @@ NET COMMANDS
> ============
>
> | **bpftool** **net** { **show** | **list** } [ **dev** *NAME* ]
> -| **bpftool** **net attach** *ATTACH_TYPE* *PROG* **dev** *NAME* [ **overwrite** ]
> +| **bpftool** **net attach** *ATTACH_TYPE* *PROG* **dev** *NAME* [ **overwrite** | **head** ]
How about "prepend" rather than "head"? The meaning would be more
explicit to me, and it would be consistent with "overwrite", which is a
verb as well.
> | **bpftool** **net detach** *ATTACH_TYPE* **dev** *NAME*
> | **bpftool** **net help**
> |
> @@ -72,6 +72,10 @@ bpftool net attach *ATTACH_TYPE* *PROG* dev *NAME* [ overwrite ]
> **tcx_ingress** - Ingress TCX. runs on ingress net traffic;
> **tcx_egress** - Egress TCX. runs on egress net traffic;
>
> + For **tcx_ingress** and **tcx_egress** attach types, the **head** option
> + can be used to attach the program at the beginning of the chain instead of
> + at the end.
Thanks! Would you mind updating the description of the "overwrite"
keyword above this, to mention that "overwrite" only applies to
XDP-related types (and to remove the note saying only XDP-related types
are supported by the command, while at it)?
> +
> bpftool net detach *ATTACH_TYPE* dev *NAME*
> Detach bpf program attached to network interface *NAME* with type specified
> by *ATTACH_TYPE*. To detach bpf program, same *ATTACH_TYPE* previously used
> @@ -191,6 +195,17 @@ EXAMPLES
> tc:
> lo(1) tcx/ingress tc_prog prog_id 29
>
> +|
> +| **# bpftool net attach tcx_ingress name tc_prog2 dev lo head**
> +| **# bpftool net**
> +|
> +
> +::
> +
> + tc:
> + lo(1) tcx/ingress tc_prog2 prog_id 30
> + lo(1) tcx/ingress tc_prog prog_id 29
> +
> |
> | **# bpftool net attach tcx_ingress name tc_prog dev lo**
> | **# bpftool net detach tcx_ingress dev lo**
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index 53bcfeb1a76e..7aba07e8629d 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -1142,7 +1142,14 @@ _bpftool()
> return 0
> ;;
> 8)
> - _bpftool_once_attr 'overwrite'
> + case ${words[3]} in
> + tcx_ingress|tcx_egress)
> + _bpftool_once_attr 'head'
> + ;;
> + *)
> + _bpftool_once_attr 'overwrite'
> + ;;
> + esac
Thanks for this!
> return 0
> ;;
> esac
> diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> index cfc6f944f7c3..d8408a109478 100644
> --- a/tools/bpf/bpftool/net.c
> +++ b/tools/bpf/bpftool/net.c
> @@ -637,6 +637,25 @@ static int net_parse_dev(int *argc, char ***argv)
> return ifindex;
> }
>
> +static int get_first_tcx_prog_id(int ifindex, enum bpf_attach_type type, __u32 *first_id)
> +{
> + int ret;
> +
> + LIBBPF_OPTS(bpf_prog_query_opts, optq);
> + __u32 prog_ids[1] = {};
> +
> + optq.prog_ids = prog_ids;
> + optq.count = ARRAY_SIZE(prog_ids);
> +
> + ret = bpf_prog_query_opts(ifindex, type, &optq);
> +
> + if (ret == 0 && optq.count > 0) {
> + *first_id = prog_ids[0];
> + return 0;
> + }
> + return -1;
> +}
> +
> static int do_attach_detach_xdp(int progfd, enum net_attach_type attach_type,
> int ifindex, bool overwrite)
> {
> @@ -666,10 +685,20 @@ static int get_tcx_type(enum net_attach_type attach_type)
> }
> }
>
> -static int do_attach_tcx(int progfd, enum net_attach_type attach_type, int ifindex)
> +static int do_attach_tcx(int progfd, enum net_attach_type attach_type, int ifindex, bool head)
> {
> int type = get_tcx_type(attach_type);
> -
> + __u32 relative_id = 0;
Nit: Please move relative_id to the relevant scope, we don't need it
defined if "head" is false.
> +
> + if (head) {
> + if (get_first_tcx_prog_id(ifindex, type, &relative_id) == 0) {
Nit:
if (!get_first_tcx_prog_id(ifindex, type, &relative_id)) {
> + LIBBPF_OPTS(bpf_prog_attach_opts, opts,
> + .flags = BPF_F_BEFORE | BPF_F_ID,
> + .relative_id = relative_id
> + );
> + return bpf_prog_attach_opts(progfd, ifindex, type, &opts);
> + }
> + }
> return bpf_prog_attach(progfd, ifindex, type, 0);
> }
>
> @@ -685,6 +714,7 @@ static int do_attach(int argc, char **argv)
> enum net_attach_type attach_type;
> int progfd, ifindex, err = 0;
> bool overwrite = false;
> + bool head = false;
>
> /* parse attach args */
> if (!REQ_ARGS(5))
> @@ -710,8 +740,16 @@ static int do_attach(int argc, char **argv)
> if (argc) {
> if (is_prefix(*argv, "overwrite")) {
> overwrite = true;
Note: Not directly related to your contribution, but we could also
filter program types here for the "overwrite" keyword, and raise an
error if it's not an XDP-related type, because we don't overwrite
existing programs for TCX (I think we silently ignore the keyword, at
the moment - not great)
> + } else if (is_prefix(*argv, "head")) {
> + if (attach_type != NET_ATTACH_TYPE_TCX_INGRESS &&
> + attach_type != NET_ATTACH_TYPE_TCX_EGRESS) {
> + p_err("'head' is only supported for tcx_ingress/tcx_egress");
> + err = -EINVAL;
> + goto cleanup;
> + }
> + head = true;
> } else {
> - p_err("expected 'overwrite', got: '%s'?", *argv);
> + p_err("expected 'overwrite' or 'head', got: '%s'?", *argv);
> err = -EINVAL;
> goto cleanup;
> }
> @@ -728,7 +766,7 @@ static int do_attach(int argc, char **argv)
> /* attach tcx prog */
> case NET_ATTACH_TYPE_TCX_INGRESS:
> case NET_ATTACH_TYPE_TCX_EGRESS:
> - err = do_attach_tcx(progfd, attach_type, ifindex);
> + err = do_attach_tcx(progfd, attach_type, ifindex, head);
> break;
> default:
> break;
> @@ -985,7 +1023,7 @@ static int do_help(int argc, char **argv)
>
> fprintf(stderr,
> "Usage: %1$s %2$s { show | list } [dev <devname>]\n"
> - " %1$s %2$s attach ATTACH_TYPE PROG dev <devname> [ overwrite ]\n"
> + " %1$s %2$s attach ATTACH_TYPE PROG dev <devname> [ overwrite | head ]\n"
> " %1$s %2$s detach ATTACH_TYPE dev <devname>\n"
> " %1$s %2$s help\n"
> "\n"
> @@ -994,6 +1032,9 @@ static int do_help(int argc, char **argv)
> " | tcx_egress }\n"
> " " HELP_SPEC_OPTIONS " }\n"
> "\n"
> + " For tcx_ingress/tcx_egress attach types:\n"
> + " head - option attaches the program at the beginning of the chain.\n"
> + "\n"
I would probably drop that note. Not because it's incorrect, but because
we don't list or explain all keywords in the interactive help for other
commands and it's something that should remain consistent. The
documentation for such keywords at the moment is the man page, I don't
really want a mix of man pages and "--help" description that gets out of
sync.
> "Note: Only xdp, tcx, tc, netkit, flow_dissector and netfilter attachments\n"
> " are currently supported.\n"
> " For progs attached to cgroups, use \"bpftool cgroup\"\n"
Powered by blists - more mailing lists