| Message ID | 20230413133228.20790-6-fw@strlen.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: add netfilter program type | expand |
Hi Florian, On Thu, 13 Apr 2023 at 14:36, Florian Westphal <fw@strlen.de> wrote: > > Dump protocol family, hook and priority value: > $ bpftool link > 2: type 10 prog 20 Could you please update link_type_name in libbpf (libbpf.c) so that we display "netfilter" here instead of "type 10"? > pf: 2, hook 1, prio -128 > > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > tools/bpf/bpftool/link.c | 24 ++++++++++++++++++++++++ > tools/include/uapi/linux/bpf.h | 15 +++++++++++++++ > tools/lib/bpf/libbpf.c | 1 + > 3 files changed, 40 insertions(+) > > diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c > index f985b79cca27..a2ea85d1ebbf 100644 > --- a/tools/bpf/bpftool/link.c > +++ b/tools/bpf/bpftool/link.c > @@ -135,6 +135,18 @@ static void show_iter_json(struct bpf_link_info *info, json_writer_t *wtr) > } > } > > +static void show_netfilter_json(const struct bpf_link_info *info, json_writer_t *wtr) > +{ > + jsonw_uint_field(json_wtr, "pf", > + info->netfilter.pf); > + jsonw_uint_field(json_wtr, "hook", > + info->netfilter.hooknum); > + jsonw_int_field(json_wtr, "prio", > + info->netfilter.priority); > + jsonw_uint_field(json_wtr, "flags", > + info->netfilter.flags); > +} > + > static int get_prog_info(int prog_id, struct bpf_prog_info *info) > { > __u32 len = sizeof(*info); > @@ -195,6 +207,10 @@ static int show_link_close_json(int fd, struct bpf_link_info *info) > info->netns.netns_ino); > show_link_attach_type_json(info->netns.attach_type, json_wtr); > break; > + case BPF_LINK_TYPE_NETFILTER: > + show_netfilter_json(info, json_wtr); > + break; > + > default: > break; > } > @@ -301,6 +317,14 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info) > printf("\n\tnetns_ino %u ", info->netns.netns_ino); > show_link_attach_type_plain(info->netns.attach_type); > break; > + case BPF_LINK_TYPE_NETFILTER: > + printf("\n\tpf: %d, hook %u, prio %d", > + info->netfilter.pf, > + info->netfilter.hooknum, > + info->netfilter.priority); > + if (info->netfilter.flags) > + printf(" flags 0x%x", info->netfilter.flags); > + break; > default: > break; > } > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > index 3823100b7934..c93febc4c75f 100644 > --- a/tools/include/uapi/linux/bpf.h > +++ b/tools/include/uapi/linux/bpf.h > @@ -986,6 +986,7 @@ enum bpf_prog_type { > BPF_PROG_TYPE_LSM, > BPF_PROG_TYPE_SK_LOOKUP, > BPF_PROG_TYPE_SYSCALL, /* a program that can execute syscalls */ > + BPF_PROG_TYPE_NETFILTER, If netfilter programs could be loaded with bpftool, we'd need to update bpftool's docs. But I don't think this is the case, right? We don't currently have a way to pass the pf, hooknum, priority and flags necessary to load the program with "bpftool prog load" so it would fail? Have you considered listing netfilter programs in the output of "bpftool net" as well? Given that they're related to networking, it would maybe make sense to have them listed alongside XDP, TC, and flow dissector programs? The patch looks good to me otherwise. Thanks, Quentin
Quentin Monnet <quentin@isovalent.com> wrote: > On Thu, 13 Apr 2023 at 14:36, Florian Westphal <fw@strlen.de> wrote: > > > > Dump protocol family, hook and priority value: > > $ bpftool link > > 2: type 10 prog 20 > > Could you please update link_type_name in libbpf (libbpf.c) so that we > display "netfilter" here instead of "type 10"? Done. > > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > > index 3823100b7934..c93febc4c75f 100644 > > --- a/tools/include/uapi/linux/bpf.h > > +++ b/tools/include/uapi/linux/bpf.h > > @@ -986,6 +986,7 @@ enum bpf_prog_type { > > BPF_PROG_TYPE_LSM, > > BPF_PROG_TYPE_SK_LOOKUP, > > BPF_PROG_TYPE_SYSCALL, /* a program that can execute syscalls */ > > + BPF_PROG_TYPE_NETFILTER, > > If netfilter programs could be loaded with bpftool, we'd need to > update bpftool's docs. But I don't think this is the case, right? bpftool prog load nftest.o /sys/fs/bpf/nftest will work, but the program isn't attached anywhere. > don't currently have a way to pass the pf, hooknum, priority and flags > necessary to load the program with "bpftool prog load" so it would > fail? I don't know how to make it work to actually attach it, because the hook is unregistered when the link fd is closed. So either bpftool would have to fork and auto-daemon (maybe unexpected...) or wait/block until CTRL-C. This also needs new libbpf api AFAICS because existing bpf_link are specific to the program type, so I'd have to add something like: struct bpf_link * bpf_program__attach_netfilter(const struct bpf_program *prog, const struct bpf_netfilter_opts *opts) Advice welcome. > Have you considered listing netfilter programs in the output of > "bpftool net" as well? Given that they're related to networking, it > would maybe make sense to have them listed alongside XDP, TC, and flow > dissector programs? I could print the same output that 'bpf link' already shows. Not sure on the real distinction between those two here. When should I use 'bpftool link' and when 'bpftool net', and what info and features should either of these provide for netfilter programs?
2023-04-14 12:41 UTC+0200 ~ Florian Westphal <fw@strlen.de> > Quentin Monnet <quentin@isovalent.com> wrote: >> On Thu, 13 Apr 2023 at 14:36, Florian Westphal <fw@strlen.de> wrote: >>> >>> Dump protocol family, hook and priority value: >>> $ bpftool link >>> 2: type 10 prog 20 >> >> Could you please update link_type_name in libbpf (libbpf.c) so that we >> display "netfilter" here instead of "type 10"? > > Done. Thanks! I'm just thinking we could also maybe print something nicer for the pf and the hook, "NF_INET_LOCAL_IN" would be more user-friendly than "hook 1"? > >>> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h >>> index 3823100b7934..c93febc4c75f 100644 >>> --- a/tools/include/uapi/linux/bpf.h >>> +++ b/tools/include/uapi/linux/bpf.h >>> @@ -986,6 +986,7 @@ enum bpf_prog_type { >>> BPF_PROG_TYPE_LSM, >>> BPF_PROG_TYPE_SK_LOOKUP, >>> BPF_PROG_TYPE_SYSCALL, /* a program that can execute syscalls */ >>> + BPF_PROG_TYPE_NETFILTER, >> >> If netfilter programs could be loaded with bpftool, we'd need to >> update bpftool's docs. But I don't think this is the case, right? > > bpftool prog load nftest.o /sys/fs/bpf/nftest > > will work, but the program isn't attached anywhere. Let's maybe not document it, then. It may still be useful to check whether a program load, but users would definitely expect the program to remain loaded after bpftool invocation has completed. Or alternatively, we could document, but print a warning. > >> don't currently have a way to pass the pf, hooknum, priority and flags >> necessary to load the program with "bpftool prog load" so it would >> fail? > > I don't know how to make it work to actually attach it, because > the hook is unregistered when the link fd is closed. > > So either bpftool would have to fork and auto-daemon (maybe > unexpected...) or wait/block until CTRL-C. > > This also needs new libbpf api AFAICS because existing bpf_link > are specific to the program type, so I'd have to add something like: > > struct bpf_link * > bpf_program__attach_netfilter(const struct bpf_program *prog, > const struct bpf_netfilter_opts *opts) > > Advice welcome. OK, yes we'd need something like this if we wanted to load and attach from bpftool. If you already have the tooling elsewhere, it's maybe not necessary to add it here. Depends if you want users to be able to attach netfilter programs with bpftool or even libbpf. There are other program types that are not supported for loading/attaching with bpftool (the bpftool-prog man page is not always correct in that regard, I think). I'd say let's keep this out of the current patchset anyway. If we have a use case for attaching via libbpf/bpftool we can do this as a follow-up. > >> Have you considered listing netfilter programs in the output of >> "bpftool net" as well? Given that they're related to networking, it >> would maybe make sense to have them listed alongside XDP, TC, and flow >> dissector programs? > > I could print the same output that 'bpf link' already shows. > > Not sure on the real distinction between those two here. There would probably be some overlap (to say the least), yes. > > When should I use 'bpftool link' and when 'bpftool net', and what info > and features should either of these provide for netfilter programs? That's a good question. I thought I'd check how we handle it for XDP for "bpftool net" vs. "bpftool link", but I realised this link type (and some others) were never added to the switch/case you update in bpftool/link.c, and we're not printing any particular information about them beyond type and associated program id. Conversely, I'd have to check whether we print XDP programs using links in "bpftool net". Maybe some things to improve here. Anyway. The way I see it, "bpftool net" should provide a more structured overview of the different programs affecting networking, in particular for JSON. The idea would be to display all BPF programs that can affect packet processing. See what we have for XDP for example: # bpftool net -p [{ "xdp": [{ "devname": "eni88np1", "ifindex": 12, "multi_attachments": [{ "mode": "driver", "id": 1238 },{ "mode": "offload", "id": 1239 } ] } ], "tc": [{ "devname": "eni88np1", "ifindex": 12, "kind": "clsact/ingress", "name": "sample_ret0.o:[.text]", "id": 1241 },{ "devname": "eni88np1", "ifindex": 12, "kind": "clsact/ingress", "name": "sample_ret0.o:[.text]", "id": 1240 } ], "flow_dissector": [ "id": 1434 ] } ] This gives us all the info about XDP programs at once, grouped by device when relevant. By contrast, listing them in "bpftool link" would likely only show one at a time, in an uncorrelated manner. Similarly, we could have netfilter sorted by pf then hook in "bpftool net". If there's more relevant info that we get from program info and not from the netfilter link, this would also be a good place to have it (but not sure there's any info we're missing from "bpftool link"?). But given that the info will be close, or identical, if not for the JSON structure, I don't mean to impose this to you - it's also OK to just skip "bpftool net" for now if you prefer. Quentin
Quentin Monnet <quentin@isovalent.com> wrote: > 2023-04-14 12:41 UTC+0200 ~ Florian Westphal <fw@strlen.de> > > Quentin Monnet <quentin@isovalent.com> wrote: > >> On Thu, 13 Apr 2023 at 14:36, Florian Westphal <fw@strlen.de> wrote: > >>> > >>> Dump protocol family, hook and priority value: > >>> $ bpftool link > >>> 2: type 10 prog 20 > >> > >> Could you please update link_type_name in libbpf (libbpf.c) so that we > >> display "netfilter" here instead of "type 10"? > > > > Done. > > Thanks! > > I'm just thinking we could also maybe print something nicer for the pf > and the hook, "NF_INET_LOCAL_IN" would be more user-friendly than "hook 1"? Done. I've also made the first patch more restrictive wrt. allowed attachment points and priorities. Better safe than sorry, we can be more liberal later if there are use-cases. v3 will be coming next week. > > I don't know how to make it work to actually attach it, because > > the hook is unregistered when the link fd is closed. > > > > So either bpftool would have to fork and auto-daemon (maybe > > unexpected...) or wait/block until CTRL-C. > > > > This also needs new libbpf api AFAICS because existing bpf_link > > are specific to the program type, so I'd have to add something like: > > > > struct bpf_link * > > bpf_program__attach_netfilter(const struct bpf_program *prog, > > const struct bpf_netfilter_opts *opts) > > > > Advice welcome. > > OK, yes we'd need something like this if we wanted to load and attach > from bpftool. If you already have the tooling elsewhere, it's maybe not > necessary to add it here. Depends if you want users to be able to attach > netfilter programs with bpftool or even libbpf. [..] > I'd say let's keep this out of the current patchset anyway. If we have a > use case for attaching via libbpf/bpftool we can do this as a follow-up. Sounds good to me. Quentin Deslandes or Daniel Xu might want/need libbpf support for their projects. > The way I see it, "bpftool net" should provide a more structured > overview of the different programs affecting networking, in particular > for JSON. The idea would be to display all BPF programs that can affect > packet processing. See what we have for XDP for example: > > > # bpftool net -p > [{ > "xdp": [{ > "devname": "eni88np1", > "ifindex": 12, > "multi_attachments": [{ > "mode": "driver", > "id": 1238 > },{ > "mode": "offload", > "id": 1239 > } > ] > } > ], > "tc": [{ > "devname": "eni88np1", > "ifindex": 12, > "kind": "clsact/ingress", > "name": "sample_ret0.o:[.text]", > "id": 1241 > },{ > "devname": "eni88np1", > "ifindex": 12, > "kind": "clsact/ingress", > "name": "sample_ret0.o:[.text]", > "id": 1240 > } > ], > "flow_dissector": [ > "id": 1434 > ] > } > ] > > This gives us all the info about XDP programs at once, grouped by device > when relevant. By contrast, listing them in "bpftool link" would likely > only show one at a time, in an uncorrelated manner. Similarly, we could > have netfilter sorted by pf then hook in "bpftool net". If there's more > relevant info that we get from program info and not from the netfilter > link, this would also be a good place to have it (but not sure there's > any info we're missing from "bpftool link"?). Currently 'bpftool link' shows everything wrt. netfilter bpf programs. > But given that the info will be close, or identical, if not for the JSON > structure, I don't mean to impose this to you - it's also OK to just > skip "bpftool net" for now if you prefer. I'll probably make 'bpftool net' and 'bpftool link' print identical netfilter output, I'll check this on Monday (to make sure the formatting doesn't seem out of place). Its kinda silly to not have anything netfilter related in 'bpftool net', this thing isn't named 'linkfilter' after all 8-)
2023-04-14 16:49 UTC+0200 ~ Florian Westphal <fw@strlen.de> > Quentin Monnet <quentin@isovalent.com> wrote: >> 2023-04-14 12:41 UTC+0200 ~ Florian Westphal <fw@strlen.de> >>> Quentin Monnet <quentin@isovalent.com> wrote: >>>> On Thu, 13 Apr 2023 at 14:36, Florian Westphal <fw@strlen.de> wrote: >>>>> >>>>> Dump protocol family, hook and priority value: >>>>> $ bpftool link >>>>> 2: type 10 prog 20 >>>> >>>> Could you please update link_type_name in libbpf (libbpf.c) so that we >>>> display "netfilter" here instead of "type 10"? >>> >>> Done. >> >> Thanks! >> >> I'm just thinking we could also maybe print something nicer for the pf >> and the hook, "NF_INET_LOCAL_IN" would be more user-friendly than "hook 1"? > > Done. I've also made the first patch more restrictive wrt. allowed > attachment points and priorities. > > Better safe than sorry, we can be more liberal later if there are > use-cases. > > v3 will be coming next week. > >>> I don't know how to make it work to actually attach it, because >>> the hook is unregistered when the link fd is closed. >>> >>> So either bpftool would have to fork and auto-daemon (maybe >>> unexpected...) or wait/block until CTRL-C. >>> >>> This also needs new libbpf api AFAICS because existing bpf_link >>> are specific to the program type, so I'd have to add something like: >>> >>> struct bpf_link * >>> bpf_program__attach_netfilter(const struct bpf_program *prog, >>> const struct bpf_netfilter_opts *opts) >>> >>> Advice welcome. >> >> OK, yes we'd need something like this if we wanted to load and attach >> from bpftool. If you already have the tooling elsewhere, it's maybe not >> necessary to add it here. Depends if you want users to be able to attach >> netfilter programs with bpftool or even libbpf. > > [..] > >> I'd say let's keep this out of the current patchset anyway. If we have a >> use case for attaching via libbpf/bpftool we can do this as a follow-up. > > Sounds good to me. > > Quentin Deslandes or Daniel Xu might want/need libbpf support for their > projects. > >> The way I see it, "bpftool net" should provide a more structured >> overview of the different programs affecting networking, in particular >> for JSON. The idea would be to display all BPF programs that can affect >> packet processing. See what we have for XDP for example: >> >> >> # bpftool net -p >> [{ >> "xdp": [{ >> "devname": "eni88np1", >> "ifindex": 12, >> "multi_attachments": [{ >> "mode": "driver", >> "id": 1238 >> },{ >> "mode": "offload", >> "id": 1239 >> } >> ] >> } >> ], >> "tc": [{ >> "devname": "eni88np1", >> "ifindex": 12, >> "kind": "clsact/ingress", >> "name": "sample_ret0.o:[.text]", >> "id": 1241 >> },{ >> "devname": "eni88np1", >> "ifindex": 12, >> "kind": "clsact/ingress", >> "name": "sample_ret0.o:[.text]", >> "id": 1240 >> } >> ], >> "flow_dissector": [ >> "id": 1434 >> ] >> } >> ] >> >> This gives us all the info about XDP programs at once, grouped by device >> when relevant. By contrast, listing them in "bpftool link" would likely >> only show one at a time, in an uncorrelated manner. Similarly, we could >> have netfilter sorted by pf then hook in "bpftool net". If there's more >> relevant info that we get from program info and not from the netfilter >> link, this would also be a good place to have it (but not sure there's >> any info we're missing from "bpftool link"?). > > Currently 'bpftool link' shows everything wrt. netfilter bpf programs. > >> But given that the info will be close, or identical, if not for the JSON >> structure, I don't mean to impose this to you - it's also OK to just >> skip "bpftool net" for now if you prefer. > > I'll probably make 'bpftool net' and 'bpftool link' print identical > netfilter output, I'll check this on Monday (to make sure the formatting > doesn't seem out of place). > > Its kinda silly to not have anything netfilter related in 'bpftool > net', this thing isn't named 'linkfilter' after all 8-) Sounds all good to me :) Thanks!
diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c index f985b79cca27..a2ea85d1ebbf 100644 --- a/tools/bpf/bpftool/link.c +++ b/tools/bpf/bpftool/link.c @@ -135,6 +135,18 @@ static void show_iter_json(struct bpf_link_info *info, json_writer_t *wtr) } } +static void show_netfilter_json(const struct bpf_link_info *info, json_writer_t *wtr) +{ + jsonw_uint_field(json_wtr, "pf", + info->netfilter.pf); + jsonw_uint_field(json_wtr, "hook", + info->netfilter.hooknum); + jsonw_int_field(json_wtr, "prio", + info->netfilter.priority); + jsonw_uint_field(json_wtr, "flags", + info->netfilter.flags); +} + static int get_prog_info(int prog_id, struct bpf_prog_info *info) { __u32 len = sizeof(*info); @@ -195,6 +207,10 @@ static int show_link_close_json(int fd, struct bpf_link_info *info) info->netns.netns_ino); show_link_attach_type_json(info->netns.attach_type, json_wtr); break; + case BPF_LINK_TYPE_NETFILTER: + show_netfilter_json(info, json_wtr); + break; + default: break; } @@ -301,6 +317,14 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info) printf("\n\tnetns_ino %u ", info->netns.netns_ino); show_link_attach_type_plain(info->netns.attach_type); break; + case BPF_LINK_TYPE_NETFILTER: + printf("\n\tpf: %d, hook %u, prio %d", + info->netfilter.pf, + info->netfilter.hooknum, + info->netfilter.priority); + if (info->netfilter.flags) + printf(" flags 0x%x", info->netfilter.flags); + break; default: break; } diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 3823100b7934..c93febc4c75f 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -986,6 +986,7 @@ enum bpf_prog_type { BPF_PROG_TYPE_LSM, BPF_PROG_TYPE_SK_LOOKUP, BPF_PROG_TYPE_SYSCALL, /* a program that can execute syscalls */ + BPF_PROG_TYPE_NETFILTER, }; enum bpf_attach_type { @@ -1050,6 +1051,7 @@ enum bpf_link_type { BPF_LINK_TYPE_PERF_EVENT = 7, BPF_LINK_TYPE_KPROBE_MULTI = 8, BPF_LINK_TYPE_STRUCT_OPS = 9, + BPF_LINK_TYPE_NETFILTER = 10, MAX_BPF_LINK_TYPE, }; @@ -1560,6 +1562,13 @@ union bpf_attr { */ __u64 cookie; } tracing; + struct { + __u32 pf; + __u32 hooknum; + __s32 prio; + __u32 flags; + __u64 reserved[2]; + } netfilter; }; } link_create; @@ -6410,6 +6419,12 @@ struct bpf_link_info { struct { __u32 map_id; } struct_ops; + struct { + __u32 pf; + __u32 hooknum; + __s32 priority; + __u32 flags; + } netfilter; }; } __attribute__((aligned(8))); diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 49cd304ae3bc..ae27451002ae 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -8641,6 +8641,7 @@ static const struct bpf_sec_def section_defs[] = { SEC_DEF("struct_ops+", STRUCT_OPS, 0, SEC_NONE), SEC_DEF("struct_ops.s+", STRUCT_OPS, 0, SEC_SLEEPABLE), SEC_DEF("sk_lookup", SK_LOOKUP, BPF_SK_LOOKUP, SEC_ATTACHABLE), + SEC_DEF("netfilter", NETFILTER, 0, SEC_NONE), }; static size_t custom_sec_def_cnt;
Dump protocol family, hook and priority value: $ bpftool link 2: type 10 prog 20 pf: 2, hook 1, prio -128 Signed-off-by: Florian Westphal <fw@strlen.de> --- tools/bpf/bpftool/link.c | 24 ++++++++++++++++++++++++ tools/include/uapi/linux/bpf.h | 15 +++++++++++++++ tools/lib/bpf/libbpf.c | 1 + 3 files changed, 40 insertions(+)