| Message ID | 20210325120020.236504-4-memxor@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | libbpf: Add TC-BPF API | expand |
On Thu, Mar 25, 2021 at 5:02 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > This adds functions that wrap the netlink API used for adding, > manipulating, and removing filters and actions. These functions operate > directly on the loaded prog's fd, and return a handle to the filter and > action using an out parameter (id for tc_cls, and index for tc_act). > > The basic featureset is covered to allow for attaching, manipulation of > properties, and removal of filters and actions. Some additional features > like TCA_BPF_POLICE and TCA_RATE for tc_cls have been omitted. These can > added on top later by extending the bpf_tc_cls_opts struct. > > Support for binding actions directly to a classifier by passing them in > during filter creation has also been omitted for now. These actions > have an auto clean up property because their lifetime is bound to the > filter they are attached to. This can be added later, but was omitted > for now as direct action mode is a better alternative to it. > > An API summary: > > The BPF TC-CLS API > > bpf_tc_act_{attach, change, replace}_{dev, block} may be used to attach, > change, and replace SCHED_CLS bpf classifiers. Separate set of functions > are provided for network interfaces and shared filter blocks. > > bpf_tc_cls_detach_{dev, block} may be used to detach existing SCHED_CLS > filter. The bpf_tc_cls_attach_id object filled in during attach, > change, or replace must be passed in to the detach functions for them to > remove the filter and its attached classififer correctly. > > bpf_tc_cls_get_info is a helper that can be used to obtain attributes > for the filter and classififer. The opts structure may be used to > choose the granularity of search, such that info for a specific filter > corresponding to the same loaded bpf program can be obtained. By > default, the first match is returned to the user. > > Examples: > > struct bpf_tc_cls_attach_id id = {}; > struct bpf_object *obj; > struct bpf_program *p; > int fd, r; > > obj = bpf_object_open("foo.o"); > if (IS_ERR_OR_NULL(obj)) > return PTR_ERR(obj); > > p = bpf_object__find_program_by_title(obj, "classifier"); > if (IS_ERR_OR_NULL(p)) > return PTR_ERR(p); > > if (bpf_object__load(obj) < 0) > return -1; > > fd = bpf_program__fd(p); > > r = bpf_tc_cls_attach_dev(fd, if_nametoindex("lo"), > BPF_TC_CLSACT_INGRESS, ETH_P_IP, > NULL, &id); > if (r < 0) > return r; > > ... which is roughly equivalent to (after clsact qdisc setup): > # tc filter add dev lo ingress bpf obj /home/kkd/foo.o sec classifier > > If a user wishes to modify existing options on an attached filter, the > bpf_tc_cls_change_{dev, block} API may be used. Parameters like > chain_index, priority, and handle are ignored in the bpf_tc_cls_opts > struct as they cannot be modified after attaching a filter. > > Example: > > /* Optional parameters necessary to select the right filter */ > DECLARE_LIBBPF_OPTS(bpf_tc_cls_opts, opts, > .handle = id.handle, > .priority = id.priority, > .chain_index = id.chain_index) > /* Turn on direct action mode */ > opts.direct_action = true; > r = bpf_tc_cls_change_dev(fd, id.ifindex, id.parent_id, > id.protocol, &opts, &id); > if (r < 0) > return r; > > /* Verify that the direct action mode has been set */ > struct bpf_tc_cls_info info = {}; > r = bpf_tc_cls_get_info_dev(fd, id.ifindex, id.parent_id, > id.protocol, &opts, &info); > if (r < 0) > return r; > > assert(info.bpf_flags & TCA_BPF_FLAG_ACT_DIRECT); > > This would be roughly equivalent to doing: > # tc filter change dev lo egress prio <p> handle <h> bpf obj /home/kkd/foo.o section classifier da > > ... except a new bpf program will be loaded and replace existing one. > > If a user wishes to either replace an existing filter, or create a new > one with the same properties, they can use bpf_tc_cls_replace_dev. The > benefit of bpf_tc_cls_change is that it fails if no matching filter > exists. > > The BPF TC-ACT API Is there some succinct but complete enough documentation/tutorial/etc that I can reasonably read to understand kernel APIs provided by TC (w.r.t. BPF, of course). I'm trying to wrap my head around this and whether API makes sense or not. Please share links, if you have some. > > bpf_tc_act_{attach, replace} may be used to attach and replace already > attached SCHED_ACT actions. Passing an index of 0 has special meaning, > in that an index will be automatically chosen by the kernel. The index > chosen by the kernel is the return value of these functions in case of > success. > > bpf_tc_act_detach may be used to detach a SCHED_ACT action prog > identified by the index parameter. The index 0 again has a special > meaning, in that passing it will flush all existing SCHED_ACT actions > loaded using the ACT API. > > bpf_tc_act_get_info is a helper to get the required attributes of a > loaded program to be able to manipulate it futher, by passing them > into the aforementioned functions. > [...]
On Sun, Mar 28, 2021 at 10:12:40AM IST, Andrii Nakryiko wrote: > Is there some succinct but complete enough documentation/tutorial/etc > that I can reasonably read to understand kernel APIs provided by TC > (w.r.t. BPF, of course). I'm trying to wrap my head around this and > whether API makes sense or not. Please share links, if you have some. > Hi Andrii, Unfortunately for the kernel API part, I couldn't find any when I was working on this. So I had to read the iproute2 tc code (tc_filter.c, f_bpf.c, m_action.c, m_bpf.c) and the kernel side bits (cls_api.c, cls_bpf.c, act_api.c, act_bpf.c) to grok anything I didn't understand. There's also similar code in libnl (lib/route/{act,cls}.c). Other than that, these resources were useful (perhaps you already went through some/all of them): https://docs.cilium.io/en/latest/bpf/#tc-traffic-control https://qmonnet.github.io/whirl-offload/2020/04/11/tc-bpf-direct-action/ tc(8), and tc-bpf(8) man pages I hope this is helpful! -- Kartikeya
On Thu 25 Mar 2021 at 14:00, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > This adds functions that wrap the netlink API used for adding, > manipulating, and removing filters and actions. These functions operate > directly on the loaded prog's fd, and return a handle to the filter and > action using an out parameter (id for tc_cls, and index for tc_act). > > The basic featureset is covered to allow for attaching, manipulation of > properties, and removal of filters and actions. Some additional features > like TCA_BPF_POLICE and TCA_RATE for tc_cls have been omitted. These can > added on top later by extending the bpf_tc_cls_opts struct. > > Support for binding actions directly to a classifier by passing them in > during filter creation has also been omitted for now. These actions > have an auto clean up property because their lifetime is bound to the > filter they are attached to. This can be added later, but was omitted > for now as direct action mode is a better alternative to it. > > An API summary: > > The BPF TC-CLS API > > bpf_tc_act_{attach, change, replace}_{dev, block} may be used to attach, > change, and replace SCHED_CLS bpf classifiers. Separate set of functions > are provided for network interfaces and shared filter blocks. > > bpf_tc_cls_detach_{dev, block} may be used to detach existing SCHED_CLS > filter. The bpf_tc_cls_attach_id object filled in during attach, > change, or replace must be passed in to the detach functions for them to > remove the filter and its attached classififer correctly. > > bpf_tc_cls_get_info is a helper that can be used to obtain attributes > for the filter and classififer. The opts structure may be used to > choose the granularity of search, such that info for a specific filter > corresponding to the same loaded bpf program can be obtained. By > default, the first match is returned to the user. > > Examples: > > struct bpf_tc_cls_attach_id id = {}; > struct bpf_object *obj; > struct bpf_program *p; > int fd, r; > > obj = bpf_object_open("foo.o"); > if (IS_ERR_OR_NULL(obj)) > return PTR_ERR(obj); > > p = bpf_object__find_program_by_title(obj, "classifier"); > if (IS_ERR_OR_NULL(p)) > return PTR_ERR(p); > > if (bpf_object__load(obj) < 0) > return -1; > > fd = bpf_program__fd(p); > > r = bpf_tc_cls_attach_dev(fd, if_nametoindex("lo"), > BPF_TC_CLSACT_INGRESS, ETH_P_IP, > NULL, &id); > if (r < 0) > return r; > > ... which is roughly equivalent to (after clsact qdisc setup): > # tc filter add dev lo ingress bpf obj /home/kkd/foo.o sec classifier > > If a user wishes to modify existing options on an attached filter, the > bpf_tc_cls_change_{dev, block} API may be used. Parameters like > chain_index, priority, and handle are ignored in the bpf_tc_cls_opts > struct as they cannot be modified after attaching a filter. > > Example: > > /* Optional parameters necessary to select the right filter */ > DECLARE_LIBBPF_OPTS(bpf_tc_cls_opts, opts, > .handle = id.handle, > .priority = id.priority, > .chain_index = id.chain_index) > /* Turn on direct action mode */ > opts.direct_action = true; > r = bpf_tc_cls_change_dev(fd, id.ifindex, id.parent_id, > id.protocol, &opts, &id); > if (r < 0) > return r; > > /* Verify that the direct action mode has been set */ > struct bpf_tc_cls_info info = {}; > r = bpf_tc_cls_get_info_dev(fd, id.ifindex, id.parent_id, > id.protocol, &opts, &info); > if (r < 0) > return r; > > assert(info.bpf_flags & TCA_BPF_FLAG_ACT_DIRECT); > > This would be roughly equivalent to doing: > # tc filter change dev lo egress prio <p> handle <h> bpf obj /home/kkd/foo.o section classifier da > > ... except a new bpf program will be loaded and replace existing one. > > If a user wishes to either replace an existing filter, or create a new > one with the same properties, they can use bpf_tc_cls_replace_dev. The > benefit of bpf_tc_cls_change is that it fails if no matching filter > exists. > > The BPF TC-ACT API > > bpf_tc_act_{attach, replace} may be used to attach and replace already > attached SCHED_ACT actions. Passing an index of 0 has special meaning, > in that an index will be automatically chosen by the kernel. The index > chosen by the kernel is the return value of these functions in case of > success. > > bpf_tc_act_detach may be used to detach a SCHED_ACT action prog > identified by the index parameter. The index 0 again has a special > meaning, in that passing it will flush all existing SCHED_ACT actions > loaded using the ACT API. > > bpf_tc_act_get_info is a helper to get the required attributes of a > loaded program to be able to manipulate it futher, by passing them > into the aforementioned functions. > > Example: > > struct bpf_object *obj; > struct bpf_program *p; > __u32 index; > int fd, r; > > obj = bpf_object_open("foo.o"); > if (IS_ERR_OR_NULL(obj)) > return PTR_ERR(obj); > > p = bpf_object__find_program_by_title(obj, "action"); > if (IS_ERR_OR_NULL(p)) > return PTR_ERR(p); > > if (bpf_object__load(obj) < 0) > return -1; > > fd = bpf_program__fd(p); > > r = bpf_tc_act_attach(fd, NULL, &index); > if (r < 0) > return r; > > if (bpf_tc_act_detach(index)) > return -1; > > ... which is equivalent to the following sequence: > tc action add action bpf obj /home/kkd/foo.o sec action > tc action del action bpf index <idx> How do you handle the locking here? Please note that while RTM_{NEW|GET|DEL}FILTER API has been refactored to handle its own locking internally (and registered with RTNL_FLAG_DOIT_UNLOCKED flag), RTM_{NEW|GET|DEL}ACTION API still expects to be called with rtnl lock taken. [...]
Vlad Buslov <vladbu@nvidia.com> writes: > On Thu 25 Mar 2021 at 14:00, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: >> This adds functions that wrap the netlink API used for adding, >> manipulating, and removing filters and actions. These functions operate >> directly on the loaded prog's fd, and return a handle to the filter and >> action using an out parameter (id for tc_cls, and index for tc_act). >> >> The basic featureset is covered to allow for attaching, manipulation of >> properties, and removal of filters and actions. Some additional features >> like TCA_BPF_POLICE and TCA_RATE for tc_cls have been omitted. These can >> added on top later by extending the bpf_tc_cls_opts struct. >> >> Support for binding actions directly to a classifier by passing them in >> during filter creation has also been omitted for now. These actions >> have an auto clean up property because their lifetime is bound to the >> filter they are attached to. This can be added later, but was omitted >> for now as direct action mode is a better alternative to it. >> >> An API summary: >> >> The BPF TC-CLS API >> >> bpf_tc_act_{attach, change, replace}_{dev, block} may be used to attach, >> change, and replace SCHED_CLS bpf classifiers. Separate set of functions >> are provided for network interfaces and shared filter blocks. >> >> bpf_tc_cls_detach_{dev, block} may be used to detach existing SCHED_CLS >> filter. The bpf_tc_cls_attach_id object filled in during attach, >> change, or replace must be passed in to the detach functions for them to >> remove the filter and its attached classififer correctly. >> >> bpf_tc_cls_get_info is a helper that can be used to obtain attributes >> for the filter and classififer. The opts structure may be used to >> choose the granularity of search, such that info for a specific filter >> corresponding to the same loaded bpf program can be obtained. By >> default, the first match is returned to the user. >> >> Examples: >> >> struct bpf_tc_cls_attach_id id = {}; >> struct bpf_object *obj; >> struct bpf_program *p; >> int fd, r; >> >> obj = bpf_object_open("foo.o"); >> if (IS_ERR_OR_NULL(obj)) >> return PTR_ERR(obj); >> >> p = bpf_object__find_program_by_title(obj, "classifier"); >> if (IS_ERR_OR_NULL(p)) >> return PTR_ERR(p); >> >> if (bpf_object__load(obj) < 0) >> return -1; >> >> fd = bpf_program__fd(p); >> >> r = bpf_tc_cls_attach_dev(fd, if_nametoindex("lo"), >> BPF_TC_CLSACT_INGRESS, ETH_P_IP, >> NULL, &id); >> if (r < 0) >> return r; >> >> ... which is roughly equivalent to (after clsact qdisc setup): >> # tc filter add dev lo ingress bpf obj /home/kkd/foo.o sec classifier >> >> If a user wishes to modify existing options on an attached filter, the >> bpf_tc_cls_change_{dev, block} API may be used. Parameters like >> chain_index, priority, and handle are ignored in the bpf_tc_cls_opts >> struct as they cannot be modified after attaching a filter. >> >> Example: >> >> /* Optional parameters necessary to select the right filter */ >> DECLARE_LIBBPF_OPTS(bpf_tc_cls_opts, opts, >> .handle = id.handle, >> .priority = id.priority, >> .chain_index = id.chain_index) >> /* Turn on direct action mode */ >> opts.direct_action = true; >> r = bpf_tc_cls_change_dev(fd, id.ifindex, id.parent_id, >> id.protocol, &opts, &id); >> if (r < 0) >> return r; >> >> /* Verify that the direct action mode has been set */ >> struct bpf_tc_cls_info info = {}; >> r = bpf_tc_cls_get_info_dev(fd, id.ifindex, id.parent_id, >> id.protocol, &opts, &info); >> if (r < 0) >> return r; >> >> assert(info.bpf_flags & TCA_BPF_FLAG_ACT_DIRECT); >> >> This would be roughly equivalent to doing: >> # tc filter change dev lo egress prio <p> handle <h> bpf obj /home/kkd/foo.o section classifier da >> >> ... except a new bpf program will be loaded and replace existing one. >> >> If a user wishes to either replace an existing filter, or create a new >> one with the same properties, they can use bpf_tc_cls_replace_dev. The >> benefit of bpf_tc_cls_change is that it fails if no matching filter >> exists. >> >> The BPF TC-ACT API >> >> bpf_tc_act_{attach, replace} may be used to attach and replace already >> attached SCHED_ACT actions. Passing an index of 0 has special meaning, >> in that an index will be automatically chosen by the kernel. The index >> chosen by the kernel is the return value of these functions in case of >> success. >> >> bpf_tc_act_detach may be used to detach a SCHED_ACT action prog >> identified by the index parameter. The index 0 again has a special >> meaning, in that passing it will flush all existing SCHED_ACT actions >> loaded using the ACT API. >> >> bpf_tc_act_get_info is a helper to get the required attributes of a >> loaded program to be able to manipulate it futher, by passing them >> into the aforementioned functions. >> >> Example: >> >> struct bpf_object *obj; >> struct bpf_program *p; >> __u32 index; >> int fd, r; >> >> obj = bpf_object_open("foo.o"); >> if (IS_ERR_OR_NULL(obj)) >> return PTR_ERR(obj); >> >> p = bpf_object__find_program_by_title(obj, "action"); >> if (IS_ERR_OR_NULL(p)) >> return PTR_ERR(p); >> >> if (bpf_object__load(obj) < 0) >> return -1; >> >> fd = bpf_program__fd(p); >> >> r = bpf_tc_act_attach(fd, NULL, &index); >> if (r < 0) >> return r; >> >> if (bpf_tc_act_detach(index)) >> return -1; >> >> ... which is equivalent to the following sequence: >> tc action add action bpf obj /home/kkd/foo.o sec action >> tc action del action bpf index <idx> > > How do you handle the locking here? Please note that while > RTM_{NEW|GET|DEL}FILTER API has been refactored to handle its own > locking internally (and registered with RTNL_FLAG_DOIT_UNLOCKED flag), > RTM_{NEW|GET|DEL}ACTION API still expects to be called with rtnl lock > taken. Huh, locking? This is all userspace code that uses the netlink API... -Toke
On Mon 29 Mar 2021 at 15:32, Toke Høiland-Jørgensen <toke@redhat.com> wrote: > Vlad Buslov <vladbu@nvidia.com> writes: > >> On Thu 25 Mar 2021 at 14:00, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: >>> This adds functions that wrap the netlink API used for adding, >>> manipulating, and removing filters and actions. These functions operate >>> directly on the loaded prog's fd, and return a handle to the filter and >>> action using an out parameter (id for tc_cls, and index for tc_act). >>> >>> The basic featureset is covered to allow for attaching, manipulation of >>> properties, and removal of filters and actions. Some additional features >>> like TCA_BPF_POLICE and TCA_RATE for tc_cls have been omitted. These can >>> added on top later by extending the bpf_tc_cls_opts struct. >>> >>> Support for binding actions directly to a classifier by passing them in >>> during filter creation has also been omitted for now. These actions >>> have an auto clean up property because their lifetime is bound to the >>> filter they are attached to. This can be added later, but was omitted >>> for now as direct action mode is a better alternative to it. >>> >>> An API summary: >>> >>> The BPF TC-CLS API >>> >>> bpf_tc_act_{attach, change, replace}_{dev, block} may be used to attach, >>> change, and replace SCHED_CLS bpf classifiers. Separate set of functions >>> are provided for network interfaces and shared filter blocks. >>> >>> bpf_tc_cls_detach_{dev, block} may be used to detach existing SCHED_CLS >>> filter. The bpf_tc_cls_attach_id object filled in during attach, >>> change, or replace must be passed in to the detach functions for them to >>> remove the filter and its attached classififer correctly. >>> >>> bpf_tc_cls_get_info is a helper that can be used to obtain attributes >>> for the filter and classififer. The opts structure may be used to >>> choose the granularity of search, such that info for a specific filter >>> corresponding to the same loaded bpf program can be obtained. By >>> default, the first match is returned to the user. >>> >>> Examples: >>> >>> struct bpf_tc_cls_attach_id id = {}; >>> struct bpf_object *obj; >>> struct bpf_program *p; >>> int fd, r; >>> >>> obj = bpf_object_open("foo.o"); >>> if (IS_ERR_OR_NULL(obj)) >>> return PTR_ERR(obj); >>> >>> p = bpf_object__find_program_by_title(obj, "classifier"); >>> if (IS_ERR_OR_NULL(p)) >>> return PTR_ERR(p); >>> >>> if (bpf_object__load(obj) < 0) >>> return -1; >>> >>> fd = bpf_program__fd(p); >>> >>> r = bpf_tc_cls_attach_dev(fd, if_nametoindex("lo"), >>> BPF_TC_CLSACT_INGRESS, ETH_P_IP, >>> NULL, &id); >>> if (r < 0) >>> return r; >>> >>> ... which is roughly equivalent to (after clsact qdisc setup): >>> # tc filter add dev lo ingress bpf obj /home/kkd/foo.o sec classifier >>> >>> If a user wishes to modify existing options on an attached filter, the >>> bpf_tc_cls_change_{dev, block} API may be used. Parameters like >>> chain_index, priority, and handle are ignored in the bpf_tc_cls_opts >>> struct as they cannot be modified after attaching a filter. >>> >>> Example: >>> >>> /* Optional parameters necessary to select the right filter */ >>> DECLARE_LIBBPF_OPTS(bpf_tc_cls_opts, opts, >>> .handle = id.handle, >>> .priority = id.priority, >>> .chain_index = id.chain_index) >>> /* Turn on direct action mode */ >>> opts.direct_action = true; >>> r = bpf_tc_cls_change_dev(fd, id.ifindex, id.parent_id, >>> id.protocol, &opts, &id); >>> if (r < 0) >>> return r; >>> >>> /* Verify that the direct action mode has been set */ >>> struct bpf_tc_cls_info info = {}; >>> r = bpf_tc_cls_get_info_dev(fd, id.ifindex, id.parent_id, >>> id.protocol, &opts, &info); >>> if (r < 0) >>> return r; >>> >>> assert(info.bpf_flags & TCA_BPF_FLAG_ACT_DIRECT); >>> >>> This would be roughly equivalent to doing: >>> # tc filter change dev lo egress prio <p> handle <h> bpf obj /home/kkd/foo.o section classifier da >>> >>> ... except a new bpf program will be loaded and replace existing one. >>> >>> If a user wishes to either replace an existing filter, or create a new >>> one with the same properties, they can use bpf_tc_cls_replace_dev. The >>> benefit of bpf_tc_cls_change is that it fails if no matching filter >>> exists. >>> >>> The BPF TC-ACT API >>> >>> bpf_tc_act_{attach, replace} may be used to attach and replace already >>> attached SCHED_ACT actions. Passing an index of 0 has special meaning, >>> in that an index will be automatically chosen by the kernel. The index >>> chosen by the kernel is the return value of these functions in case of >>> success. >>> >>> bpf_tc_act_detach may be used to detach a SCHED_ACT action prog >>> identified by the index parameter. The index 0 again has a special >>> meaning, in that passing it will flush all existing SCHED_ACT actions >>> loaded using the ACT API. >>> >>> bpf_tc_act_get_info is a helper to get the required attributes of a >>> loaded program to be able to manipulate it futher, by passing them >>> into the aforementioned functions. >>> >>> Example: >>> >>> struct bpf_object *obj; >>> struct bpf_program *p; >>> __u32 index; >>> int fd, r; >>> >>> obj = bpf_object_open("foo.o"); >>> if (IS_ERR_OR_NULL(obj)) >>> return PTR_ERR(obj); >>> >>> p = bpf_object__find_program_by_title(obj, "action"); >>> if (IS_ERR_OR_NULL(p)) >>> return PTR_ERR(p); >>> >>> if (bpf_object__load(obj) < 0) >>> return -1; >>> >>> fd = bpf_program__fd(p); >>> >>> r = bpf_tc_act_attach(fd, NULL, &index); >>> if (r < 0) >>> return r; >>> >>> if (bpf_tc_act_detach(index)) >>> return -1; >>> >>> ... which is equivalent to the following sequence: >>> tc action add action bpf obj /home/kkd/foo.o sec action >>> tc action del action bpf index <idx> >> >> How do you handle the locking here? Please note that while >> RTM_{NEW|GET|DEL}FILTER API has been refactored to handle its own >> locking internally (and registered with RTNL_FLAG_DOIT_UNLOCKED flag), >> RTM_{NEW|GET|DEL}ACTION API still expects to be called with rtnl lock >> taken. > > Huh, locking? This is all userspace code that uses the netlink API... > > -Toke Thanks for the clarification. I'm not familiar with libbpf internals and it wasn't obvious to me that this functionality is not for creating classifiers/actions from BPF program executing in kernel-space.
On Sun, Mar 28, 2021 at 1:11 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Sun, Mar 28, 2021 at 10:12:40AM IST, Andrii Nakryiko wrote: > > Is there some succinct but complete enough documentation/tutorial/etc > > that I can reasonably read to understand kernel APIs provided by TC > > (w.r.t. BPF, of course). I'm trying to wrap my head around this and > > whether API makes sense or not. Please share links, if you have some. > > > > Hi Andrii, > > Unfortunately for the kernel API part, I couldn't find any when I was working > on this. So I had to read the iproute2 tc code (tc_filter.c, f_bpf.c, > m_action.c, m_bpf.c) and the kernel side bits (cls_api.c, cls_bpf.c, act_api.c, > act_bpf.c) to grok anything I didn't understand. There's also similar code in > libnl (lib/route/{act,cls}.c). > > Other than that, these resources were useful (perhaps you already went through > some/all of them): > > https://docs.cilium.io/en/latest/bpf/#tc-traffic-control > https://qmonnet.github.io/whirl-offload/2020/04/11/tc-bpf-direct-action/ > tc(8), and tc-bpf(8) man pages > > I hope this is helpful! Thanks! I'll take a look. Sorry, I'm a bit behind with all the stuff, trying to catch up. I was just wondering if it would be more natural instead of having _dev _block variants and having to specify __u32 ifindex, __u32 parent_id, __u32 protocol, to have some struct specifying TC "destination"? Maybe not, but I thought I'd bring this up early. So you'd have just bpf_tc_cls_attach(), and you'd so something like bpf_tc_cls_attach(prog_fd, TC_DEV(ifindex, parent_id, protocol)) or bpf_tc_cls_attach(prog_fd, TC_BLOCK(block_idx, protocol)) ? Or it's taking it too far? But even if not, I think detaching can be unified between _dev and _block, can't it? > > -- > Kartikeya
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Sun, Mar 28, 2021 at 1:11 AM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: >> >> On Sun, Mar 28, 2021 at 10:12:40AM IST, Andrii Nakryiko wrote: >> > Is there some succinct but complete enough documentation/tutorial/etc >> > that I can reasonably read to understand kernel APIs provided by TC >> > (w.r.t. BPF, of course). I'm trying to wrap my head around this and >> > whether API makes sense or not. Please share links, if you have some. >> > >> >> Hi Andrii, >> >> Unfortunately for the kernel API part, I couldn't find any when I was working >> on this. So I had to read the iproute2 tc code (tc_filter.c, f_bpf.c, >> m_action.c, m_bpf.c) and the kernel side bits (cls_api.c, cls_bpf.c, act_api.c, >> act_bpf.c) to grok anything I didn't understand. There's also similar code in >> libnl (lib/route/{act,cls}.c). >> >> Other than that, these resources were useful (perhaps you already went through >> some/all of them): >> >> https://docs.cilium.io/en/latest/bpf/#tc-traffic-control >> https://qmonnet.github.io/whirl-offload/2020/04/11/tc-bpf-direct-action/ >> tc(8), and tc-bpf(8) man pages >> >> I hope this is helpful! > > Thanks! I'll take a look. Sorry, I'm a bit behind with all the stuff, > trying to catch up. > > I was just wondering if it would be more natural instead of having > _dev _block variants and having to specify __u32 ifindex, __u32 > parent_id, __u32 protocol, to have some struct specifying TC > "destination"? Maybe not, but I thought I'd bring this up early. So > you'd have just bpf_tc_cls_attach(), and you'd so something like > > bpf_tc_cls_attach(prog_fd, TC_DEV(ifindex, parent_id, protocol)) > > or > > bpf_tc_cls_attach(prog_fd, TC_BLOCK(block_idx, protocol)) > > ? Or it's taking it too far? Hmm, that's not a bad idea, actually. An earlier version of the series did have only a single set of functions, but with way too many arguments, which is why we ended up agreeing to split them. But encapsulating the destination in a separate struct and combining it with some helper macros might just make this work! I like it! Kumar, WDYT? -Toke
On 3/30/21 10:39 PM, Andrii Nakryiko wrote: > On Sun, Mar 28, 2021 at 1:11 AM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: >> On Sun, Mar 28, 2021 at 10:12:40AM IST, Andrii Nakryiko wrote: >>> Is there some succinct but complete enough documentation/tutorial/etc >>> that I can reasonably read to understand kernel APIs provided by TC >>> (w.r.t. BPF, of course). I'm trying to wrap my head around this and >>> whether API makes sense or not. Please share links, if you have some. >> >> Hi Andrii, >> >> Unfortunately for the kernel API part, I couldn't find any when I was working >> on this. So I had to read the iproute2 tc code (tc_filter.c, f_bpf.c, >> m_action.c, m_bpf.c) and the kernel side bits (cls_api.c, cls_bpf.c, act_api.c, >> act_bpf.c) to grok anything I didn't understand. There's also similar code in >> libnl (lib/route/{act,cls}.c). >> >> Other than that, these resources were useful (perhaps you already went through >> some/all of them): >> >> https://docs.cilium.io/en/latest/bpf/#tc-traffic-control >> https://qmonnet.github.io/whirl-offload/2020/04/11/tc-bpf-direct-action/ >> tc(8), and tc-bpf(8) man pages >> >> I hope this is helpful! > > Thanks! I'll take a look. Sorry, I'm a bit behind with all the stuff, > trying to catch up. > > I was just wondering if it would be more natural instead of having > _dev _block variants and having to specify __u32 ifindex, __u32 > parent_id, __u32 protocol, to have some struct specifying TC > "destination"? Maybe not, but I thought I'd bring this up early. So > you'd have just bpf_tc_cls_attach(), and you'd so something like > > bpf_tc_cls_attach(prog_fd, TC_DEV(ifindex, parent_id, protocol)) > > or > > bpf_tc_cls_attach(prog_fd, TC_BLOCK(block_idx, protocol)) > > ? Or it's taking it too far? > > But even if not, I think detaching can be unified between _dev and > _block, can't it? Do we even need the _block variant? I would rather prefer to take the chance and make it as simple as possible, and only iff really needed extend with other APIs, for example: bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}); Internally, this will create the sch_clsact qdisc & cls_bpf filter instance iff not present yet, and attach to a default prio 1 handle 1, and _always_ in direct-action mode. This is /as simple as it gets/ and we don't need to bother users with more complex tc/cls_bpf internals unless desired. For example, extended APIs could add prio/parent so that multi-prog can be attached to a single cls_bpf instance, but even that could be a second step, imho. Thanks, Daniel
On Tue, Mar 30, 2021 at 2:26 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 3/30/21 10:39 PM, Andrii Nakryiko wrote: > > On Sun, Mar 28, 2021 at 1:11 AM Kumar Kartikeya Dwivedi > > <memxor@gmail.com> wrote: > >> On Sun, Mar 28, 2021 at 10:12:40AM IST, Andrii Nakryiko wrote: > >>> Is there some succinct but complete enough documentation/tutorial/etc > >>> that I can reasonably read to understand kernel APIs provided by TC > >>> (w.r.t. BPF, of course). I'm trying to wrap my head around this and > >>> whether API makes sense or not. Please share links, if you have some. > >> > >> Hi Andrii, > >> > >> Unfortunately for the kernel API part, I couldn't find any when I was working > >> on this. So I had to read the iproute2 tc code (tc_filter.c, f_bpf.c, > >> m_action.c, m_bpf.c) and the kernel side bits (cls_api.c, cls_bpf.c, act_api.c, > >> act_bpf.c) to grok anything I didn't understand. There's also similar code in > >> libnl (lib/route/{act,cls}.c). > >> > >> Other than that, these resources were useful (perhaps you already went through > >> some/all of them): > >> > >> https://docs.cilium.io/en/latest/bpf/#tc-traffic-control > >> https://qmonnet.github.io/whirl-offload/2020/04/11/tc-bpf-direct-action/ > >> tc(8), and tc-bpf(8) man pages > >> > >> I hope this is helpful! > > > > Thanks! I'll take a look. Sorry, I'm a bit behind with all the stuff, > > trying to catch up. > > > > I was just wondering if it would be more natural instead of having > > _dev _block variants and having to specify __u32 ifindex, __u32 > > parent_id, __u32 protocol, to have some struct specifying TC > > "destination"? Maybe not, but I thought I'd bring this up early. So > > you'd have just bpf_tc_cls_attach(), and you'd so something like > > > > bpf_tc_cls_attach(prog_fd, TC_DEV(ifindex, parent_id, protocol)) > > > > or > > > > bpf_tc_cls_attach(prog_fd, TC_BLOCK(block_idx, protocol)) > > > > ? Or it's taking it too far? > > > > But even if not, I think detaching can be unified between _dev and > > _block, can't it? > > Do we even need the _block variant? I would rather prefer to take the chance > and make it as simple as possible, and only iff really needed extend with > other APIs, for example: > > bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}); > > Internally, this will create the sch_clsact qdisc & cls_bpf filter instance > iff not present yet, and attach to a default prio 1 handle 1, and _always_ in > direct-action mode. This is /as simple as it gets/ and we don't need to bother > users with more complex tc/cls_bpf internals unless desired. For example, > extended APIs could add prio/parent so that multi-prog can be attached to a > single cls_bpf instance, but even that could be a second step, imho. +1 to support sched_cls in direct-action mode only.
On Wed, Mar 31, 2021 at 02:41:40AM IST, Toke Høiland-Jørgensen wrote: > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Sun, Mar 28, 2021 at 1:11 AM Kumar Kartikeya Dwivedi > > <memxor@gmail.com> wrote: > >> > >> On Sun, Mar 28, 2021 at 10:12:40AM IST, Andrii Nakryiko wrote: > >> > Is there some succinct but complete enough documentation/tutorial/etc > >> > that I can reasonably read to understand kernel APIs provided by TC > >> > (w.r.t. BPF, of course). I'm trying to wrap my head around this and > >> > whether API makes sense or not. Please share links, if you have some. > >> > > >> > >> Hi Andrii, > >> > >> Unfortunately for the kernel API part, I couldn't find any when I was working > >> on this. So I had to read the iproute2 tc code (tc_filter.c, f_bpf.c, > >> m_action.c, m_bpf.c) and the kernel side bits (cls_api.c, cls_bpf.c, act_api.c, > >> act_bpf.c) to grok anything I didn't understand. There's also similar code in > >> libnl (lib/route/{act,cls}.c). > >> > >> Other than that, these resources were useful (perhaps you already went through > >> some/all of them): > >> > >> https://docs.cilium.io/en/latest/bpf/#tc-traffic-control > >> https://qmonnet.github.io/whirl-offload/2020/04/11/tc-bpf-direct-action/ > >> tc(8), and tc-bpf(8) man pages > >> > >> I hope this is helpful! > > > > Thanks! I'll take a look. Sorry, I'm a bit behind with all the stuff, > > trying to catch up. > > > > I was just wondering if it would be more natural instead of having > > _dev _block variants and having to specify __u32 ifindex, __u32 > > parent_id, __u32 protocol, to have some struct specifying TC > > "destination"? Maybe not, but I thought I'd bring this up early. So > > you'd have just bpf_tc_cls_attach(), and you'd so something like > > > > bpf_tc_cls_attach(prog_fd, TC_DEV(ifindex, parent_id, protocol)) > > > > or > > > > bpf_tc_cls_attach(prog_fd, TC_BLOCK(block_idx, protocol)) > > > > ? Or it's taking it too far? > > Hmm, that's not a bad idea, actually. An earlier version of the series > did have only a single set of functions, but with way too many > arguments, which is why we ended up agreeing to split them. But > encapsulating the destination in a separate struct and combining it with > some helper macros might just make this work! I like it! Kumar, WDYT? > SGTM. > -Toke > -- Kartikeya
On Wed, Mar 31, 2021 at 02:55:47AM IST, Daniel Borkmann wrote: > Do we even need the _block variant? I would rather prefer to take the chance > and make it as simple as possible, and only iff really needed extend with > other APIs, for example: The block variant can be dropped, I'll use the TC_BLOCK/TC_DEV alternative which sets parent_id/ifindex properly. > > bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}); > > Internally, this will create the sch_clsact qdisc & cls_bpf filter instance > iff not present yet, and attach to a default prio 1 handle 1, and _always_ in > direct-action mode. This is /as simple as it gets/ and we don't need to bother > users with more complex tc/cls_bpf internals unless desired. For example, > extended APIs could add prio/parent so that multi-prog can be attached to a > single cls_bpf instance, but even that could be a second step, imho. > I am not opposed to clsact qdisc setup if INGRESS/EGRESS is supplied (not sure how others feel about it). We could make direct_action mode default, and similarly choose prio as 1 by default instead of letting the kernel do it. Then you can just pass in NULL for bpf_tc_cls_opts and be close to what you're proposing. For protocol we can choose ETH_P_ALL by default too if the user doesn't set it. With these modifications, the equivalent would look like bpf_tc_cls_attach(prog_fd, TC_DEV(ifindex, INGRESS), NULL, &id); So as long as the user doesn't care about other details, they can just pass opts as NULL. WDYT? > Thanks, > Daniel -- Kartikeya
Daniel Borkmann <daniel@iogearbox.net> writes: > On 3/30/21 10:39 PM, Andrii Nakryiko wrote: >> On Sun, Mar 28, 2021 at 1:11 AM Kumar Kartikeya Dwivedi >> <memxor@gmail.com> wrote: >>> On Sun, Mar 28, 2021 at 10:12:40AM IST, Andrii Nakryiko wrote: >>>> Is there some succinct but complete enough documentation/tutorial/etc >>>> that I can reasonably read to understand kernel APIs provided by TC >>>> (w.r.t. BPF, of course). I'm trying to wrap my head around this and >>>> whether API makes sense or not. Please share links, if you have some. >>> >>> Hi Andrii, >>> >>> Unfortunately for the kernel API part, I couldn't find any when I was working >>> on this. So I had to read the iproute2 tc code (tc_filter.c, f_bpf.c, >>> m_action.c, m_bpf.c) and the kernel side bits (cls_api.c, cls_bpf.c, act_api.c, >>> act_bpf.c) to grok anything I didn't understand. There's also similar code in >>> libnl (lib/route/{act,cls}.c). >>> >>> Other than that, these resources were useful (perhaps you already went through >>> some/all of them): >>> >>> https://docs.cilium.io/en/latest/bpf/#tc-traffic-control >>> https://qmonnet.github.io/whirl-offload/2020/04/11/tc-bpf-direct-action/ >>> tc(8), and tc-bpf(8) man pages >>> >>> I hope this is helpful! >> >> Thanks! I'll take a look. Sorry, I'm a bit behind with all the stuff, >> trying to catch up. >> >> I was just wondering if it would be more natural instead of having >> _dev _block variants and having to specify __u32 ifindex, __u32 >> parent_id, __u32 protocol, to have some struct specifying TC >> "destination"? Maybe not, but I thought I'd bring this up early. So >> you'd have just bpf_tc_cls_attach(), and you'd so something like >> >> bpf_tc_cls_attach(prog_fd, TC_DEV(ifindex, parent_id, protocol)) >> >> or >> >> bpf_tc_cls_attach(prog_fd, TC_BLOCK(block_idx, protocol)) >> >> ? Or it's taking it too far? >> >> But even if not, I think detaching can be unified between _dev and >> _block, can't it? > > Do we even need the _block variant? I would rather prefer to take the chance > and make it as simple as possible, and only iff really needed extend with > other APIs, for example: > > bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}); > > Internally, this will create the sch_clsact qdisc & cls_bpf filter instance > iff not present yet, and attach to a default prio 1 handle 1, and _always_ in > direct-action mode. This is /as simple as it gets/ and we don't need to bother > users with more complex tc/cls_bpf internals unless desired. For example, > extended APIs could add prio/parent so that multi-prog can be attached to a > single cls_bpf instance, but even that could be a second step, imho. While I'm all for simplifying where possible, the question becomes at what level? I.e., we initially figured we'd expose (most of) the netlink API in the low-level API (patch 3 in the series) and then have the bpf_program__* level API be the simple "just attach" one... We could simplify the low-level one further, of course, for instance by getting rid of the block stuff entirely, but I don't see much value in leaving out the support for prio/parent in the bpf_tc_cls_* - we'd have to make the API extensible so it could be added later anyway, so why not just include it from the get-go (especially as Kumar has already written the code?) -Toke
On 3/31/21 11:44 AM, Kumar Kartikeya Dwivedi wrote: > On Wed, Mar 31, 2021 at 02:55:47AM IST, Daniel Borkmann wrote: >> Do we even need the _block variant? I would rather prefer to take the chance >> and make it as simple as possible, and only iff really needed extend with >> other APIs, for example: > > The block variant can be dropped, I'll use the TC_BLOCK/TC_DEV alternative which > sets parent_id/ifindex properly. > >> bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}); >> >> Internally, this will create the sch_clsact qdisc & cls_bpf filter instance >> iff not present yet, and attach to a default prio 1 handle 1, and _always_ in >> direct-action mode. This is /as simple as it gets/ and we don't need to bother >> users with more complex tc/cls_bpf internals unless desired. For example, >> extended APIs could add prio/parent so that multi-prog can be attached to a >> single cls_bpf instance, but even that could be a second step, imho. > > I am not opposed to clsact qdisc setup if INGRESS/EGRESS is supplied (not sure > how others feel about it). What speaks against it? It would be 100% clear from API side where the prog is being attached. Same as with tc cmdline where you specify 'ingress'/'egress'. > We could make direct_action mode default, and similarly choose prio To be honest, I wouldn't even support a mode from the lib/API side where direct_action is not set. It should always be forced to true. Everything else is rather broken setup-wise, imho, since it won't scale. We added direct_action a bit later to the kernel than original cls_bpf, but if I would do it again today, I'd make it the only available option. I don't see a reasonable use-case where you have it to false. > as 1 by default instead of letting the kernel do it. Then you can just pass in > NULL for bpf_tc_cls_opts and be close to what you're proposing. For protocol we > can choose ETH_P_ALL by default too if the user doesn't set it. Same here with ETH_P_ALL, I'm not sure anyone uses anything other than ETH_P_ALL, so yes, that should be default. > With these modifications, the equivalent would look like > bpf_tc_cls_attach(prog_fd, TC_DEV(ifindex, INGRESS), NULL, &id); Few things compared to bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): 1) nit, but why even 'cls' in the name. I think we shouldn't expose such old-days tc semantics to a user. Just bpf_tc_attach() is cleaner/simpler to understand. 2) What's the 'TC_DEV(ifindex, INGRESS)' macro doing exactly? Looks unnecessary, why not regular args to the API? 3) Exposed bpf_tc_attach() API could internally call a bpf_tc_attach_opts() API with preset defaults, and the latter could have all the custom bits if the user needs to go beyond the simple API, so from your bpf_tc_cls_attach() I'd also drop the NULL. 4) For the simple API I'd likely also drop the id (you could have a query API if needed). > So as long as the user doesn't care about other details, they can just pass opts > as NULL.
On Fri, Apr 02, 2021 at 05:49:29AM IST, Daniel Borkmann wrote: > On 3/31/21 11:44 AM, Kumar Kartikeya Dwivedi wrote: > > On Wed, Mar 31, 2021 at 02:55:47AM IST, Daniel Borkmann wrote: > > > Do we even need the _block variant? I would rather prefer to take the chance > > > and make it as simple as possible, and only iff really needed extend with > > > other APIs, for example: > > > > The block variant can be dropped, I'll use the TC_BLOCK/TC_DEV alternative which > > sets parent_id/ifindex properly. > > > > > bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}); > > > > > > Internally, this will create the sch_clsact qdisc & cls_bpf filter instance > > > iff not present yet, and attach to a default prio 1 handle 1, and _always_ in > > > direct-action mode. This is /as simple as it gets/ and we don't need to bother > > > users with more complex tc/cls_bpf internals unless desired. For example, > > > extended APIs could add prio/parent so that multi-prog can be attached to a > > > single cls_bpf instance, but even that could be a second step, imho. > > > > I am not opposed to clsact qdisc setup if INGRESS/EGRESS is supplied (not sure > > how others feel about it). > > What speaks against it? It would be 100% clear from API side where the prog is > being attached. Same as with tc cmdline where you specify 'ingress'/'egress'. > Ok, I will add the qdisc setup in the next revision. > > We could make direct_action mode default, and similarly choose prio > > To be honest, I wouldn't even support a mode from the lib/API side where direct_action > is not set. It should always be forced to true. Everything else is rather broken > setup-wise, imho, since it won't scale. We added direct_action a bit later to the > kernel than original cls_bpf, but if I would do it again today, I'd make it the > only available option. I don't see a reasonable use-case where you have it to false. > I'm all for doing that, but in some sense that also speaks against SCHED_ACT support. Currently, you can load SCHED_ACT programs using this series, but not really bind them to classifier. I left that option open to a future patch, it would just reuse the existing tc_act_add_action helper (also why I kept it in its own function). Maybe we need to reconsider that, if direct action is the only recommended way going forward (to discourage people from using SCHED_ACT), or just add opts to do all the setup in low level API, instead of leaving it incomplete. > > as 1 by default instead of letting the kernel do it. Then you can just pass in > > NULL for bpf_tc_cls_opts and be close to what you're proposing. For protocol we > > can choose ETH_P_ALL by default too if the user doesn't set it. > > Same here with ETH_P_ALL, I'm not sure anyone uses anything other than ETH_P_ALL, > so yes, that should be default. Ack. > > > With these modifications, the equivalent would look like > > bpf_tc_cls_attach(prog_fd, TC_DEV(ifindex, INGRESS), NULL, &id); > > Few things compared to bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): > > 1) nit, but why even 'cls' in the name. I think we shouldn't expose such old-days > tc semantics to a user. Just bpf_tc_attach() is cleaner/simpler to understand. Since it would make it clear this is for SCHED_CLS progs, likewise bpf_tc_act_* is for SCHED_ACT progs. Not opposed to changing the name. > 2) What's the 'TC_DEV(ifindex, INGRESS)' macro doing exactly? Looks unnecessary, > why not regular args to the API? It is very easy to support BLOCK (I know it's not really popular here, but I think if supporting it just requires adding a macro, then we can go ahead). So the user can use TC_BLOCK(block_idx) instead of remembering ifindex is to be set to TCM_IFINDEX_MAGIC_BLOCK and parent_id to actual block index. It will just expand to: #define TC_BLOCK(block_idx) TCM_IFINDEX_MAGIC_BLOCK, (block_idx) TC_DEV macro can be dropped, since user can directly pass ifindex and parent_id. > 3) Exposed bpf_tc_attach() API could internally call a bpf_tc_attach_opts() API > with preset defaults, and the latter could have all the custom bits if the user > needs to go beyond the simple API, so from your bpf_tc_cls_attach() I'd also > drop the NULL. Ok, this is probably better (but maybe we can do this for the high-level bpf_program__attach that returns a bpf_link * instead of introducing yet another function). > 4) For the simple API I'd likely also drop the id (you could have a query API if > needed). > This would be fine, because it's not a fast path or anything, but right now we return the id using the netlink response, otherwise for query we have to open the socket, prepare the msg, send and recv again. So it's a minor optimization. However, there's one other problem. In an earlier version of this series, I didn't keep the id/index out parameters (to act as handle to the newly attached filter/action). This lead to problems on query. Suppose a user doesn't properly fill the opts during query (e.g. in case of filters). This means the netlink dump includes all filters matching filled in attributes. If the prog_id for all of these is same (e.g. all have same bpf classifier prog attached to them), it becomes impossible to determine which one is the filter user asked for. It is not possible to enforce filling in all kinds of attributes since some can be left out and assigned by default in the kernel (priority, chain_index etc.). So returning the newly created filter's id turned out to be the best option. This is also used to stash filter related information in bpf_link to properly release it later. The same problem happens with actions, where we look up using the prog_id, we multiple actions with different index can match on same prog_id. It is not possible to determine which index corresponds to last loaded action. So unless there's a better idea on how to deal with this, a query API won't work for the case where same bpf prog is attached more than once. Returning the id/index during attach seemed better than all other options we considered. -- Kartikeya
On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > This would be fine, because it's not a fast path or anything, but right now we > return the id using the netlink response, otherwise for query we have to open > the socket, prepare the msg, send and recv again. So it's a minor optimization. > > However, there's one other problem. In an earlier version of this series, I > didn't keep the id/index out parameters (to act as handle to the newly attached > filter/action). This lead to problems on query. Suppose a user doesn't properly > fill the opts during query (e.g. in case of filters). This means the netlink > dump includes all filters matching filled in attributes. If the prog_id for all > of these is same (e.g. all have same bpf classifier prog attached to them), it > becomes impossible to determine which one is the filter user asked for. It is > not possible to enforce filling in all kinds of attributes since some can be > left out and assigned by default in the kernel (priority, chain_index etc.). So > returning the newly created filter's id turned out to be the best option. This > is also used to stash filter related information in bpf_link to properly release > it later. > > The same problem happens with actions, where we look up using the prog_id, we > multiple actions with different index can match on same prog_id. It is not > possible to determine which index corresponds to last loaded action. > > So unless there's a better idea on how to deal with this, a query API won't work > for the case where same bpf prog is attached more than once. Returning the > id/index during attach seemed better than all other options we considered. All of these things are messy because of tc legacy. bpf tried to follow tc style with cls and act distinction and it didn't quite work. cls with direct-action is the only thing that became mainstream while tc style attach wasn't really addressed. There were several incidents where tc had tens of thousands of progs attached because of this attach/query/index weirdness described above. I think the only way to address this properly is to introduce bpf_link style of attaching to tc. Such bpf_link would support ingress/egress only. direction-action will be implied. There won't be any index and query will be obvious. So I would like to propose to take this patch set a step further from what Daniel said: int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): and make this proposed api to return FD. To detach from tc ingress/egress just close(fd). The user processes will not conflict with each other and will not accidently detach bpf program that was attached by another user process. Such api will address the existing tc query/attach/detach race race conditions. And libbpf side of support for this api will be trivial. Single bpf link_create command with ifindex and ingress|egress arguments. wdyt?
On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: > On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > [...] > > All of these things are messy because of tc legacy. bpf tried to follow tc style > with cls and act distinction and it didn't quite work. cls with > direct-action is the only > thing that became mainstream while tc style attach wasn't really addressed. > There were several incidents where tc had tens of thousands of progs attached > because of this attach/query/index weirdness described above. > I think the only way to address this properly is to introduce bpf_link style of > attaching to tc. Such bpf_link would support ingress/egress only. > direction-action will be implied. There won't be any index and query > will be obvious. Note that we already have bpf_link support working (without support for pinning ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle, chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link and are able to operate on the exact filter during release. > So I would like to propose to take this patch set a step further from > what Daniel said: > int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): > and make this proposed api to return FD. > To detach from tc ingress/egress just close(fd). You mean adding an fd-based TC API to the kernel? > The user processes will not conflict with each other and will not accidently > detach bpf program that was attached by another user process. > Such api will address the existing tc query/attach/detach race race conditions. Hmm, I think we do solve the race condition by returning the id. As long as you don't misuse the interface and go around deleting filters arbitrarily (i.e. only detach using the id), programs won't step over each other's filters. Storing the id from the netlink response received during detach also eliminates any ambigiuity from probing through get_info after attach. Same goes for actions, and the same applies to the bpf_link returning API (which stashes id/index). Do you have any other example that can still be racy given the current API? The only advantage of fd would be the possibility of pinning it, and extending lifetime of the filter. > And libbpf side of support for this api will be trivial. Single bpf > link_create command > with ifindex and ingress|egress arguments. > wdyt? -- Kartikeya
On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi wrote: > On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: > > On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > [...] > > > > All of these things are messy because of tc legacy. bpf tried to follow tc style > > with cls and act distinction and it didn't quite work. cls with > > direct-action is the only > > thing that became mainstream while tc style attach wasn't really addressed. > > There were several incidents where tc had tens of thousands of progs attached > > because of this attach/query/index weirdness described above. > > I think the only way to address this properly is to introduce bpf_link style of > > attaching to tc. Such bpf_link would support ingress/egress only. > > direction-action will be implied. There won't be any index and query > > will be obvious. > > Note that we already have bpf_link support working (without support for pinning > ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle, > chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link > and are able to operate on the exact filter during release. Except they're not unique. The library can stash them, but something else doing detach via iproute2 or their own netlink calls will detach the prog. This other app can attach to the same spot a different prog and now bpf_link__destroy will be detaching somebody else prog. > > So I would like to propose to take this patch set a step further from > > what Daniel said: > > int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): > > and make this proposed api to return FD. > > To detach from tc ingress/egress just close(fd). > > You mean adding an fd-based TC API to the kernel? yes. > > The user processes will not conflict with each other and will not accidently > > detach bpf program that was attached by another user process. > > Such api will address the existing tc query/attach/detach race race conditions. > > Hmm, I think we do solve the race condition by returning the id. As long as you > don't misuse the interface and go around deleting filters arbitrarily (i.e. only > detach using the id), programs won't step over each other's filters. Storing the > id from the netlink response received during detach also eliminates any > ambigiuity from probing through get_info after attach. Same goes for actions, > and the same applies to the bpf_link returning API (which stashes id/index). There are plenty of tools and libraries out there that do attach/detach of bpf to tc. Everyone is not going to convert to this new libbpf api overnight. So 'miuse of the interface' is not a misuse. It's a reality that is going to keep happening unless the kernel guarantees ownership of the attachment via FD. > The only advantage of fd would be the possibility of pinning it, and extending > lifetime of the filter. Pinning is one of the advantages. The main selling point of FD is ownership of the attachment.
On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Fri, Apr 02, 2021 at 05:49:29AM IST, Daniel Borkmann wrote: > > On 3/31/21 11:44 AM, Kumar Kartikeya Dwivedi wrote: > > > On Wed, Mar 31, 2021 at 02:55:47AM IST, Daniel Borkmann wrote: > > > > Do we even need the _block variant? I would rather prefer to take the chance > > > > and make it as simple as possible, and only iff really needed extend with > > > > other APIs, for example: > > > > > > The block variant can be dropped, I'll use the TC_BLOCK/TC_DEV alternative which > > > sets parent_id/ifindex properly. > > > > > > > bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}); > > > > > > > > Internally, this will create the sch_clsact qdisc & cls_bpf filter instance > > > > iff not present yet, and attach to a default prio 1 handle 1, and _always_ in > > > > direct-action mode. This is /as simple as it gets/ and we don't need to bother > > > > users with more complex tc/cls_bpf internals unless desired. For example, > > > > extended APIs could add prio/parent so that multi-prog can be attached to a > > > > single cls_bpf instance, but even that could be a second step, imho. > > > > > > I am not opposed to clsact qdisc setup if INGRESS/EGRESS is supplied (not sure > > > how others feel about it). > > > > What speaks against it? It would be 100% clear from API side where the prog is > > being attached. Same as with tc cmdline where you specify 'ingress'/'egress'. > > > > Ok, I will add the qdisc setup in the next revision. > > > > We could make direct_action mode default, and similarly choose prio > > > > To be honest, I wouldn't even support a mode from the lib/API side where direct_action > > is not set. It should always be forced to true. Everything else is rather broken > > setup-wise, imho, since it won't scale. We added direct_action a bit later to the > > kernel than original cls_bpf, but if I would do it again today, I'd make it the > > only available option. I don't see a reasonable use-case where you have it to false. > > > > I'm all for doing that, but in some sense that also speaks against SCHED_ACT > support. Currently, you can load SCHED_ACT programs using this series, but not > really bind them to classifier. I left that option open to a future patch, it > would just reuse the existing tc_act_add_action helper (also why I kept it in > its own function). Maybe we need to reconsider that, if direct action is the > only recommended way going forward (to discourage people from using SCHED_ACT), > or just add opts to do all the setup in low level API, instead of leaving it > incomplete. > > > > as 1 by default instead of letting the kernel do it. Then you can just pass in > > > NULL for bpf_tc_cls_opts and be close to what you're proposing. For protocol we > > > can choose ETH_P_ALL by default too if the user doesn't set it. > > > > Same here with ETH_P_ALL, I'm not sure anyone uses anything other than ETH_P_ALL, > > so yes, that should be default. > > Ack. > > > > > > With these modifications, the equivalent would look like > > > bpf_tc_cls_attach(prog_fd, TC_DEV(ifindex, INGRESS), NULL, &id); > > > > Few things compared to bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): > > > > 1) nit, but why even 'cls' in the name. I think we shouldn't expose such old-days > > tc semantics to a user. Just bpf_tc_attach() is cleaner/simpler to understand. > > Since it would make it clear this is for SCHED_CLS progs, likewise bpf_tc_act_* > is for SCHED_ACT progs. Not opposed to changing the name. > > > 2) What's the 'TC_DEV(ifindex, INGRESS)' macro doing exactly? Looks unnecessary, > > why not regular args to the API? > > It is very easy to support BLOCK (I know it's not really popular here, but I > think if supporting it just requires adding a macro, then we can go ahead). So > the user can use TC_BLOCK(block_idx) instead of remembering ifindex is to be set > to TCM_IFINDEX_MAGIC_BLOCK and parent_id to actual block index. It will just > expand to: > > #define TC_BLOCK(block_idx) TCM_IFINDEX_MAGIC_BLOCK, (block_idx) > > TC_DEV macro can be dropped, since user can directly pass ifindex and parent_id. if _block variant is just a special ifindex value, then it should be fine for users to know such a detail (we can leave a comment mentioning this specifically), especially given it's not a very popular thing. Almost doubling amount of APIs just for this doesn't make much sense, IMO. > > > 3) Exposed bpf_tc_attach() API could internally call a bpf_tc_attach_opts() API > > with preset defaults, and the latter could have all the custom bits if the user > > needs to go beyond the simple API, so from your bpf_tc_cls_attach() I'd also > > drop the NULL. > > Ok, this is probably better (but maybe we can do this for the high-level > bpf_program__attach that returns a bpf_link * instead of introducing yet > another function). If we know that we need variant with options, I'd vote for having just one bpf_tc_attach() API which always takes options. Passing NULL for opts is simple, no need for two APIs, I think. > > > 4) For the simple API I'd likely also drop the id (you could have a query API if > > needed). > > > > This would be fine, because it's not a fast path or anything, but right now we > return the id using the netlink response, otherwise for query we have to open > the socket, prepare the msg, send and recv again. So it's a minor optimization. > > However, there's one other problem. In an earlier version of this series, I > didn't keep the id/index out parameters (to act as handle to the newly attached > filter/action). This lead to problems on query. Suppose a user doesn't properly > fill the opts during query (e.g. in case of filters). This means the netlink > dump includes all filters matching filled in attributes. If the prog_id for all > of these is same (e.g. all have same bpf classifier prog attached to them), it > becomes impossible to determine which one is the filter user asked for. It is > not possible to enforce filling in all kinds of attributes since some can be > left out and assigned by default in the kernel (priority, chain_index etc.). So > returning the newly created filter's id turned out to be the best option. This > is also used to stash filter related information in bpf_link to properly release > it later. > > The same problem happens with actions, where we look up using the prog_id, we > multiple actions with different index can match on same prog_id. It is not > possible to determine which index corresponds to last loaded action. > > So unless there's a better idea on how to deal with this, a query API won't work > for the case where same bpf prog is attached more than once. Returning the > id/index during attach seemed better than all other options we considered. Which parts of that id struct is the data that caller might not know or can't know? Is it handle and chain_index? Or just one of them? Or?... If there is something that has to be returned back, I'd keep only that, instead of returning 6+ fields, most of which user should already know. > > -- > Kartikeya
On Sat, Apr 3, 2021 at 10:47 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi wrote: > > On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: > > > On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > [...] > > > > > > All of these things are messy because of tc legacy. bpf tried to follow tc style > > > with cls and act distinction and it didn't quite work. cls with > > > direct-action is the only > > > thing that became mainstream while tc style attach wasn't really addressed. > > > There were several incidents where tc had tens of thousands of progs attached > > > because of this attach/query/index weirdness described above. > > > I think the only way to address this properly is to introduce bpf_link style of > > > attaching to tc. Such bpf_link would support ingress/egress only. > > > direction-action will be implied. There won't be any index and query > > > will be obvious. > > > > Note that we already have bpf_link support working (without support for pinning > > ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle, > > chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link > > and are able to operate on the exact filter during release. > > Except they're not unique. The library can stash them, but something else > doing detach via iproute2 or their own netlink calls will detach the prog. > This other app can attach to the same spot a different prog and now > bpf_link__destroy will be detaching somebody else prog. > > > > So I would like to propose to take this patch set a step further from > > > what Daniel said: > > > int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): > > > and make this proposed api to return FD. > > > To detach from tc ingress/egress just close(fd). > > > > You mean adding an fd-based TC API to the kernel? > > yes. I'm totally for bpf_link-based TC attachment. But I think *also* having "legacy" netlink-based APIs will allow applications to handle older kernels in a much nicer way without extra dependency on iproute2. We have a similar situation with kprobe, where currently libbpf only supports "modern" fd-based attachment, but users periodically ask questions and struggle to figure out issues on older kernels that don't support new APIs. So I think we'd have to support legacy TC APIs, but I agree with Alexei and Daniel that we should keep it to the simplest and most straightforward API of supporting direction-action attachments and setting up qdisc transparently (if I'm getting all the terminology right, after reading Quentin's blog post). That coincidentally should probably match how bpf_link-based TC API will look like, so all that can be abstracted behind a single bpf_link__attach_tc() API as well, right? That's the plan for dealing with kprobe right now, btw. Libbpf will detect the best available API and transparently fall back (maybe with some warning for awareness, due to inherent downsides of legacy APIs: no auto-cleanup being the most prominent one). > > > > The user processes will not conflict with each other and will not accidently > > > detach bpf program that was attached by another user process. > > > Such api will address the existing tc query/attach/detach race race conditions. > > > > Hmm, I think we do solve the race condition by returning the id. As long as you > > don't misuse the interface and go around deleting filters arbitrarily (i.e. only > > detach using the id), programs won't step over each other's filters. Storing the > > id from the netlink response received during detach also eliminates any > > ambigiuity from probing through get_info after attach. Same goes for actions, > > and the same applies to the bpf_link returning API (which stashes id/index). > > There are plenty of tools and libraries out there that do attach/detach of bpf > to tc. Everyone is not going to convert to this new libbpf api overnight. > So 'miuse of the interface' is not a misuse. It's a reality that is going to keep > happening unless the kernel guarantees ownership of the attachment via FD. > > > The only advantage of fd would be the possibility of pinning it, and extending > > lifetime of the filter. > > Pinning is one of the advantages. The main selling point of FD is ownership > of the attachment.
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Sat, Apr 3, 2021 at 10:47 AM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: >> >> On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi wrote: >> > On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: >> > > On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: >> > > > [...] >> > > >> > > All of these things are messy because of tc legacy. bpf tried to follow tc style >> > > with cls and act distinction and it didn't quite work. cls with >> > > direct-action is the only >> > > thing that became mainstream while tc style attach wasn't really addressed. >> > > There were several incidents where tc had tens of thousands of progs attached >> > > because of this attach/query/index weirdness described above. >> > > I think the only way to address this properly is to introduce bpf_link style of >> > > attaching to tc. Such bpf_link would support ingress/egress only. >> > > direction-action will be implied. There won't be any index and query >> > > will be obvious. >> > >> > Note that we already have bpf_link support working (without support for pinning >> > ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle, >> > chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link >> > and are able to operate on the exact filter during release. >> >> Except they're not unique. The library can stash them, but something else >> doing detach via iproute2 or their own netlink calls will detach the prog. >> This other app can attach to the same spot a different prog and now >> bpf_link__destroy will be detaching somebody else prog. >> >> > > So I would like to propose to take this patch set a step further from >> > > what Daniel said: >> > > int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): >> > > and make this proposed api to return FD. >> > > To detach from tc ingress/egress just close(fd). >> > >> > You mean adding an fd-based TC API to the kernel? >> >> yes. > > I'm totally for bpf_link-based TC attachment. > > But I think *also* having "legacy" netlink-based APIs will allow > applications to handle older kernels in a much nicer way without extra > dependency on iproute2. We have a similar situation with kprobe, where > currently libbpf only supports "modern" fd-based attachment, but users > periodically ask questions and struggle to figure out issues on older > kernels that don't support new APIs. +1; I am OK with adding a new bpf_link-based way to attach TC programs, but we still need to support the netlink API in libbpf. > So I think we'd have to support legacy TC APIs, but I agree with > Alexei and Daniel that we should keep it to the simplest and most > straightforward API of supporting direction-action attachments and > setting up qdisc transparently (if I'm getting all the terminology > right, after reading Quentin's blog post). That coincidentally should > probably match how bpf_link-based TC API will look like, so all that > can be abstracted behind a single bpf_link__attach_tc() API as well, > right? That's the plan for dealing with kprobe right now, btw. Libbpf > will detect the best available API and transparently fall back (maybe > with some warning for awareness, due to inherent downsides of legacy > APIs: no auto-cleanup being the most prominent one). Yup, SGTM: Expose both in the low-level API (in bpf.c), and make the high-level API auto-detect. That way users can also still use the netlink attach function if they don't want the fd-based auto-close behaviour of bpf_link. -Toke
On Mon, Apr 05, 2021 at 10:51:09PM IST, Andrii Nakryiko wrote: > > [...] > > if _block variant is just a special ifindex value, then it should be > fine for users to know such a detail (we can leave a comment > mentioning this specifically), especially given it's not a very > popular thing. Almost doubling amount of APIs just for this doesn't > make much sense, IMO. > Ok. > > If we know that we need variant with options, I'd vote for having just > one bpf_tc_attach() API which always takes options. Passing NULL for > opts is simple, no need for two APIs, I think. > Ack. > > Which parts of that id struct is the data that caller might not know > or can't know? Is it handle and chain_index? Or just one of them? > Or?... If there is something that has to be returned back, I'd keep > only that, instead of returning 6+ fields, most of which user should > already know. > The user will know ifindex and parent_id, and perhaps protocol (it would be ETH_P_ALL if they don't supply one by default). Other fields like handle, priority and chain_index can all be kernel assigned, so keeping those still makes sense. I'll change this in v2. -- Kartikeya
On Tue, Apr 6, 2021 at 3:06 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Sat, Apr 3, 2021 at 10:47 AM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > >> > >> On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi wrote: > >> > On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: > >> > > On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > >> > > > [...] > >> > > > >> > > All of these things are messy because of tc legacy. bpf tried to follow tc style > >> > > with cls and act distinction and it didn't quite work. cls with > >> > > direct-action is the only > >> > > thing that became mainstream while tc style attach wasn't really addressed. > >> > > There were several incidents where tc had tens of thousands of progs attached > >> > > because of this attach/query/index weirdness described above. > >> > > I think the only way to address this properly is to introduce bpf_link style of > >> > > attaching to tc. Such bpf_link would support ingress/egress only. > >> > > direction-action will be implied. There won't be any index and query > >> > > will be obvious. > >> > > >> > Note that we already have bpf_link support working (without support for pinning > >> > ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle, > >> > chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link > >> > and are able to operate on the exact filter during release. > >> > >> Except they're not unique. The library can stash them, but something else > >> doing detach via iproute2 or their own netlink calls will detach the prog. > >> This other app can attach to the same spot a different prog and now > >> bpf_link__destroy will be detaching somebody else prog. > >> > >> > > So I would like to propose to take this patch set a step further from > >> > > what Daniel said: > >> > > int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): > >> > > and make this proposed api to return FD. > >> > > To detach from tc ingress/egress just close(fd). > >> > > >> > You mean adding an fd-based TC API to the kernel? > >> > >> yes. > > > > I'm totally for bpf_link-based TC attachment. > > > > But I think *also* having "legacy" netlink-based APIs will allow > > applications to handle older kernels in a much nicer way without extra > > dependency on iproute2. We have a similar situation with kprobe, where > > currently libbpf only supports "modern" fd-based attachment, but users > > periodically ask questions and struggle to figure out issues on older > > kernels that don't support new APIs. > > +1; I am OK with adding a new bpf_link-based way to attach TC programs, > but we still need to support the netlink API in libbpf. > > > So I think we'd have to support legacy TC APIs, but I agree with > > Alexei and Daniel that we should keep it to the simplest and most > > straightforward API of supporting direction-action attachments and > > setting up qdisc transparently (if I'm getting all the terminology > > right, after reading Quentin's blog post). That coincidentally should > > probably match how bpf_link-based TC API will look like, so all that > > can be abstracted behind a single bpf_link__attach_tc() API as well, > > right? That's the plan for dealing with kprobe right now, btw. Libbpf > > will detect the best available API and transparently fall back (maybe > > with some warning for awareness, due to inherent downsides of legacy > > APIs: no auto-cleanup being the most prominent one). > > Yup, SGTM: Expose both in the low-level API (in bpf.c), and make the > high-level API auto-detect. That way users can also still use the > netlink attach function if they don't want the fd-based auto-close > behaviour of bpf_link. So I thought a bit more about this, and it feels like the right move would be to expose only higher-level TC BPF API behind bpf_link. It will keep the API complexity and amount of APIs that libbpf will have to support to the minimum, and will keep the API itself simple: direct-attach with the minimum amount of input arguments. By not exposing low-level APIs we also table the whole bpf_tc_cls_attach_id design discussion, as we now can keep as much info as needed inside bpf_link_tc (which will embed bpf_link internally as well) to support detachment and possibly some additional querying, if needed. I think that's the best and least controversial step forward for getting this API into libbpf. > > -Toke >
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Tue, Apr 6, 2021 at 3:06 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >> >> > On Sat, Apr 3, 2021 at 10:47 AM Alexei Starovoitov >> > <alexei.starovoitov@gmail.com> wrote: >> >> >> >> On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi wrote: >> >> > On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: >> >> > > On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: >> >> > > > [...] >> >> > > >> >> > > All of these things are messy because of tc legacy. bpf tried to follow tc style >> >> > > with cls and act distinction and it didn't quite work. cls with >> >> > > direct-action is the only >> >> > > thing that became mainstream while tc style attach wasn't really addressed. >> >> > > There were several incidents where tc had tens of thousands of progs attached >> >> > > because of this attach/query/index weirdness described above. >> >> > > I think the only way to address this properly is to introduce bpf_link style of >> >> > > attaching to tc. Such bpf_link would support ingress/egress only. >> >> > > direction-action will be implied. There won't be any index and query >> >> > > will be obvious. >> >> > >> >> > Note that we already have bpf_link support working (without support for pinning >> >> > ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle, >> >> > chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link >> >> > and are able to operate on the exact filter during release. >> >> >> >> Except they're not unique. The library can stash them, but something else >> >> doing detach via iproute2 or their own netlink calls will detach the prog. >> >> This other app can attach to the same spot a different prog and now >> >> bpf_link__destroy will be detaching somebody else prog. >> >> >> >> > > So I would like to propose to take this patch set a step further from >> >> > > what Daniel said: >> >> > > int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): >> >> > > and make this proposed api to return FD. >> >> > > To detach from tc ingress/egress just close(fd). >> >> > >> >> > You mean adding an fd-based TC API to the kernel? >> >> >> >> yes. >> > >> > I'm totally for bpf_link-based TC attachment. >> > >> > But I think *also* having "legacy" netlink-based APIs will allow >> > applications to handle older kernels in a much nicer way without extra >> > dependency on iproute2. We have a similar situation with kprobe, where >> > currently libbpf only supports "modern" fd-based attachment, but users >> > periodically ask questions and struggle to figure out issues on older >> > kernels that don't support new APIs. >> >> +1; I am OK with adding a new bpf_link-based way to attach TC programs, >> but we still need to support the netlink API in libbpf. >> >> > So I think we'd have to support legacy TC APIs, but I agree with >> > Alexei and Daniel that we should keep it to the simplest and most >> > straightforward API of supporting direction-action attachments and >> > setting up qdisc transparently (if I'm getting all the terminology >> > right, after reading Quentin's blog post). That coincidentally should >> > probably match how bpf_link-based TC API will look like, so all that >> > can be abstracted behind a single bpf_link__attach_tc() API as well, >> > right? That's the plan for dealing with kprobe right now, btw. Libbpf >> > will detect the best available API and transparently fall back (maybe >> > with some warning for awareness, due to inherent downsides of legacy >> > APIs: no auto-cleanup being the most prominent one). >> >> Yup, SGTM: Expose both in the low-level API (in bpf.c), and make the >> high-level API auto-detect. That way users can also still use the >> netlink attach function if they don't want the fd-based auto-close >> behaviour of bpf_link. > > So I thought a bit more about this, and it feels like the right move > would be to expose only higher-level TC BPF API behind bpf_link. It > will keep the API complexity and amount of APIs that libbpf will have > to support to the minimum, and will keep the API itself simple: > direct-attach with the minimum amount of input arguments. By not > exposing low-level APIs we also table the whole bpf_tc_cls_attach_id > design discussion, as we now can keep as much info as needed inside > bpf_link_tc (which will embed bpf_link internally as well) to support > detachment and possibly some additional querying, if needed. But then there would be no way for the caller to explicitly select a mechanism? I.e., if I write a BPF program using this mechanism targeting a 5.12 kernel, I'll get netlink attachment, which can stick around when I do bpf_link__disconnect(). But then if the kernel gets upgraded to support bpf_link for TC programs I'll suddenly transparently get bpf_link and the attachments will go away unless I pin them. This seems... less than ideal? If we expose the low-level API I can elect to just use this if I know I want netlink behaviour, but if bpf_program__attach_tc() is the only API available it would at least need a flag to enforce one mode or the other (I can see someone wanting to enforce kernel bpf_link semantics as well, so a flag for either mode seems reasonable?). -Toke
On Wed, Apr 14, 2021 at 3:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Tue, Apr 6, 2021 at 3:06 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> > >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >> > >> > On Sat, Apr 3, 2021 at 10:47 AM Alexei Starovoitov > >> > <alexei.starovoitov@gmail.com> wrote: > >> >> > >> >> On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi wrote: > >> >> > On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: > >> >> > > On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > >> >> > > > [...] > >> >> > > > >> >> > > All of these things are messy because of tc legacy. bpf tried to follow tc style > >> >> > > with cls and act distinction and it didn't quite work. cls with > >> >> > > direct-action is the only > >> >> > > thing that became mainstream while tc style attach wasn't really addressed. > >> >> > > There were several incidents where tc had tens of thousands of progs attached > >> >> > > because of this attach/query/index weirdness described above. > >> >> > > I think the only way to address this properly is to introduce bpf_link style of > >> >> > > attaching to tc. Such bpf_link would support ingress/egress only. > >> >> > > direction-action will be implied. There won't be any index and query > >> >> > > will be obvious. > >> >> > > >> >> > Note that we already have bpf_link support working (without support for pinning > >> >> > ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle, > >> >> > chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link > >> >> > and are able to operate on the exact filter during release. > >> >> > >> >> Except they're not unique. The library can stash them, but something else > >> >> doing detach via iproute2 or their own netlink calls will detach the prog. > >> >> This other app can attach to the same spot a different prog and now > >> >> bpf_link__destroy will be detaching somebody else prog. > >> >> > >> >> > > So I would like to propose to take this patch set a step further from > >> >> > > what Daniel said: > >> >> > > int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): > >> >> > > and make this proposed api to return FD. > >> >> > > To detach from tc ingress/egress just close(fd). > >> >> > > >> >> > You mean adding an fd-based TC API to the kernel? > >> >> > >> >> yes. > >> > > >> > I'm totally for bpf_link-based TC attachment. > >> > > >> > But I think *also* having "legacy" netlink-based APIs will allow > >> > applications to handle older kernels in a much nicer way without extra > >> > dependency on iproute2. We have a similar situation with kprobe, where > >> > currently libbpf only supports "modern" fd-based attachment, but users > >> > periodically ask questions and struggle to figure out issues on older > >> > kernels that don't support new APIs. > >> > >> +1; I am OK with adding a new bpf_link-based way to attach TC programs, > >> but we still need to support the netlink API in libbpf. > >> > >> > So I think we'd have to support legacy TC APIs, but I agree with > >> > Alexei and Daniel that we should keep it to the simplest and most > >> > straightforward API of supporting direction-action attachments and > >> > setting up qdisc transparently (if I'm getting all the terminology > >> > right, after reading Quentin's blog post). That coincidentally should > >> > probably match how bpf_link-based TC API will look like, so all that > >> > can be abstracted behind a single bpf_link__attach_tc() API as well, > >> > right? That's the plan for dealing with kprobe right now, btw. Libbpf > >> > will detect the best available API and transparently fall back (maybe > >> > with some warning for awareness, due to inherent downsides of legacy > >> > APIs: no auto-cleanup being the most prominent one). > >> > >> Yup, SGTM: Expose both in the low-level API (in bpf.c), and make the > >> high-level API auto-detect. That way users can also still use the > >> netlink attach function if they don't want the fd-based auto-close > >> behaviour of bpf_link. > > > > So I thought a bit more about this, and it feels like the right move > > would be to expose only higher-level TC BPF API behind bpf_link. It > > will keep the API complexity and amount of APIs that libbpf will have > > to support to the minimum, and will keep the API itself simple: > > direct-attach with the minimum amount of input arguments. By not > > exposing low-level APIs we also table the whole bpf_tc_cls_attach_id > > design discussion, as we now can keep as much info as needed inside > > bpf_link_tc (which will embed bpf_link internally as well) to support > > detachment and possibly some additional querying, if needed. > > But then there would be no way for the caller to explicitly select a > mechanism? I.e., if I write a BPF program using this mechanism targeting > a 5.12 kernel, I'll get netlink attachment, which can stick around when > I do bpf_link__disconnect(). But then if the kernel gets upgraded to > support bpf_link for TC programs I'll suddenly transparently get > bpf_link and the attachments will go away unless I pin them. This > seems... less than ideal? That's what we are doing with bpf_program__attach_kprobe(), though. And so far I've only seen people (privately) saying how good it would be to have bpf_link-based TC APIs, doesn't seem like anyone with a realistic use case prefers the current APIs. So I suspect it's not going to be a problem in practice. But at least I'd start there and see how people are using it and if they need anything else. > > If we expose the low-level API I can elect to just use this if I know I > want netlink behaviour, but if bpf_program__attach_tc() is the only API > available it would at least need a flag to enforce one mode or the other > (I can see someone wanting to enforce kernel bpf_link semantics as well, > so a flag for either mode seems reasonable?). Sophisticated enough users can also do feature detection to know if it's going to work or not. There are many ways to skin this cat. I'd prioritize bpf_link-based TC APIs to be added with legacy TC API as a fallback. > > -Toke >
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Wed, Apr 14, 2021 at 3:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >> >> > On Tue, Apr 6, 2021 at 3:06 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >> >> >> >> > On Sat, Apr 3, 2021 at 10:47 AM Alexei Starovoitov >> >> > <alexei.starovoitov@gmail.com> wrote: >> >> >> >> >> >> On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi wrote: >> >> >> > On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: >> >> >> > > On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: >> >> >> > > > [...] >> >> >> > > >> >> >> > > All of these things are messy because of tc legacy. bpf tried to follow tc style >> >> >> > > with cls and act distinction and it didn't quite work. cls with >> >> >> > > direct-action is the only >> >> >> > > thing that became mainstream while tc style attach wasn't really addressed. >> >> >> > > There were several incidents where tc had tens of thousands of progs attached >> >> >> > > because of this attach/query/index weirdness described above. >> >> >> > > I think the only way to address this properly is to introduce bpf_link style of >> >> >> > > attaching to tc. Such bpf_link would support ingress/egress only. >> >> >> > > direction-action will be implied. There won't be any index and query >> >> >> > > will be obvious. >> >> >> > >> >> >> > Note that we already have bpf_link support working (without support for pinning >> >> >> > ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle, >> >> >> > chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link >> >> >> > and are able to operate on the exact filter during release. >> >> >> >> >> >> Except they're not unique. The library can stash them, but something else >> >> >> doing detach via iproute2 or their own netlink calls will detach the prog. >> >> >> This other app can attach to the same spot a different prog and now >> >> >> bpf_link__destroy will be detaching somebody else prog. >> >> >> >> >> >> > > So I would like to propose to take this patch set a step further from >> >> >> > > what Daniel said: >> >> >> > > int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): >> >> >> > > and make this proposed api to return FD. >> >> >> > > To detach from tc ingress/egress just close(fd). >> >> >> > >> >> >> > You mean adding an fd-based TC API to the kernel? >> >> >> >> >> >> yes. >> >> > >> >> > I'm totally for bpf_link-based TC attachment. >> >> > >> >> > But I think *also* having "legacy" netlink-based APIs will allow >> >> > applications to handle older kernels in a much nicer way without extra >> >> > dependency on iproute2. We have a similar situation with kprobe, where >> >> > currently libbpf only supports "modern" fd-based attachment, but users >> >> > periodically ask questions and struggle to figure out issues on older >> >> > kernels that don't support new APIs. >> >> >> >> +1; I am OK with adding a new bpf_link-based way to attach TC programs, >> >> but we still need to support the netlink API in libbpf. >> >> >> >> > So I think we'd have to support legacy TC APIs, but I agree with >> >> > Alexei and Daniel that we should keep it to the simplest and most >> >> > straightforward API of supporting direction-action attachments and >> >> > setting up qdisc transparently (if I'm getting all the terminology >> >> > right, after reading Quentin's blog post). That coincidentally should >> >> > probably match how bpf_link-based TC API will look like, so all that >> >> > can be abstracted behind a single bpf_link__attach_tc() API as well, >> >> > right? That's the plan for dealing with kprobe right now, btw. Libbpf >> >> > will detect the best available API and transparently fall back (maybe >> >> > with some warning for awareness, due to inherent downsides of legacy >> >> > APIs: no auto-cleanup being the most prominent one). >> >> >> >> Yup, SGTM: Expose both in the low-level API (in bpf.c), and make the >> >> high-level API auto-detect. That way users can also still use the >> >> netlink attach function if they don't want the fd-based auto-close >> >> behaviour of bpf_link. >> > >> > So I thought a bit more about this, and it feels like the right move >> > would be to expose only higher-level TC BPF API behind bpf_link. It >> > will keep the API complexity and amount of APIs that libbpf will have >> > to support to the minimum, and will keep the API itself simple: >> > direct-attach with the minimum amount of input arguments. By not >> > exposing low-level APIs we also table the whole bpf_tc_cls_attach_id >> > design discussion, as we now can keep as much info as needed inside >> > bpf_link_tc (which will embed bpf_link internally as well) to support >> > detachment and possibly some additional querying, if needed. >> >> But then there would be no way for the caller to explicitly select a >> mechanism? I.e., if I write a BPF program using this mechanism targeting >> a 5.12 kernel, I'll get netlink attachment, which can stick around when >> I do bpf_link__disconnect(). But then if the kernel gets upgraded to >> support bpf_link for TC programs I'll suddenly transparently get >> bpf_link and the attachments will go away unless I pin them. This >> seems... less than ideal? > > That's what we are doing with bpf_program__attach_kprobe(), though. > And so far I've only seen people (privately) saying how good it would > be to have bpf_link-based TC APIs, doesn't seem like anyone with a > realistic use case prefers the current APIs. So I suspect it's not > going to be a problem in practice. But at least I'd start there and > see how people are using it and if they need anything else. *sigh* - I really wish you would stop arbitrarily declaring your own use cases "realistic" and mine (implied) "unrealistic". Makes it really hard to have a productive discussion... >> If we expose the low-level API I can elect to just use this if I know I >> want netlink behaviour, but if bpf_program__attach_tc() is the only API >> available it would at least need a flag to enforce one mode or the other >> (I can see someone wanting to enforce kernel bpf_link semantics as well, >> so a flag for either mode seems reasonable?). > > Sophisticated enough users can also do feature detection to know if > it's going to work or not. Sure, but that won't help if there's no API to pick the attach mode they want. > There are many ways to skin this cat. I'd prioritize bpf_link-based TC > APIs to be added with legacy TC API as a fallback. I'm fine with adding that; I just want the functions implementing the TC API to also be exported so users can use those if they prefer... -Toke
On Wed, Apr 14, 2021 at 3:51 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Wed, Apr 14, 2021 at 3:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> > >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >> > >> > On Tue, Apr 6, 2021 at 3:06 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> >> > >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >> >> > >> >> > On Sat, Apr 3, 2021 at 10:47 AM Alexei Starovoitov > >> >> > <alexei.starovoitov@gmail.com> wrote: > >> >> >> > >> >> >> On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi wrote: > >> >> >> > On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: > >> >> >> > > On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > >> >> >> > > > [...] > >> >> >> > > > >> >> >> > > All of these things are messy because of tc legacy. bpf tried to follow tc style > >> >> >> > > with cls and act distinction and it didn't quite work. cls with > >> >> >> > > direct-action is the only > >> >> >> > > thing that became mainstream while tc style attach wasn't really addressed. > >> >> >> > > There were several incidents where tc had tens of thousands of progs attached > >> >> >> > > because of this attach/query/index weirdness described above. > >> >> >> > > I think the only way to address this properly is to introduce bpf_link style of > >> >> >> > > attaching to tc. Such bpf_link would support ingress/egress only. > >> >> >> > > direction-action will be implied. There won't be any index and query > >> >> >> > > will be obvious. > >> >> >> > > >> >> >> > Note that we already have bpf_link support working (without support for pinning > >> >> >> > ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle, > >> >> >> > chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link > >> >> >> > and are able to operate on the exact filter during release. > >> >> >> > >> >> >> Except they're not unique. The library can stash them, but something else > >> >> >> doing detach via iproute2 or their own netlink calls will detach the prog. > >> >> >> This other app can attach to the same spot a different prog and now > >> >> >> bpf_link__destroy will be detaching somebody else prog. > >> >> >> > >> >> >> > > So I would like to propose to take this patch set a step further from > >> >> >> > > what Daniel said: > >> >> >> > > int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): > >> >> >> > > and make this proposed api to return FD. > >> >> >> > > To detach from tc ingress/egress just close(fd). > >> >> >> > > >> >> >> > You mean adding an fd-based TC API to the kernel? > >> >> >> > >> >> >> yes. > >> >> > > >> >> > I'm totally for bpf_link-based TC attachment. > >> >> > > >> >> > But I think *also* having "legacy" netlink-based APIs will allow > >> >> > applications to handle older kernels in a much nicer way without extra > >> >> > dependency on iproute2. We have a similar situation with kprobe, where > >> >> > currently libbpf only supports "modern" fd-based attachment, but users > >> >> > periodically ask questions and struggle to figure out issues on older > >> >> > kernels that don't support new APIs. > >> >> > >> >> +1; I am OK with adding a new bpf_link-based way to attach TC programs, > >> >> but we still need to support the netlink API in libbpf. > >> >> > >> >> > So I think we'd have to support legacy TC APIs, but I agree with > >> >> > Alexei and Daniel that we should keep it to the simplest and most > >> >> > straightforward API of supporting direction-action attachments and > >> >> > setting up qdisc transparently (if I'm getting all the terminology > >> >> > right, after reading Quentin's blog post). That coincidentally should > >> >> > probably match how bpf_link-based TC API will look like, so all that > >> >> > can be abstracted behind a single bpf_link__attach_tc() API as well, > >> >> > right? That's the plan for dealing with kprobe right now, btw. Libbpf > >> >> > will detect the best available API and transparently fall back (maybe > >> >> > with some warning for awareness, due to inherent downsides of legacy > >> >> > APIs: no auto-cleanup being the most prominent one). > >> >> > >> >> Yup, SGTM: Expose both in the low-level API (in bpf.c), and make the > >> >> high-level API auto-detect. That way users can also still use the > >> >> netlink attach function if they don't want the fd-based auto-close > >> >> behaviour of bpf_link. > >> > > >> > So I thought a bit more about this, and it feels like the right move > >> > would be to expose only higher-level TC BPF API behind bpf_link. It > >> > will keep the API complexity and amount of APIs that libbpf will have > >> > to support to the minimum, and will keep the API itself simple: > >> > direct-attach with the minimum amount of input arguments. By not > >> > exposing low-level APIs we also table the whole bpf_tc_cls_attach_id > >> > design discussion, as we now can keep as much info as needed inside > >> > bpf_link_tc (which will embed bpf_link internally as well) to support > >> > detachment and possibly some additional querying, if needed. > >> > >> But then there would be no way for the caller to explicitly select a > >> mechanism? I.e., if I write a BPF program using this mechanism targeting > >> a 5.12 kernel, I'll get netlink attachment, which can stick around when > >> I do bpf_link__disconnect(). But then if the kernel gets upgraded to > >> support bpf_link for TC programs I'll suddenly transparently get > >> bpf_link and the attachments will go away unless I pin them. This > >> seems... less than ideal? > > > > That's what we are doing with bpf_program__attach_kprobe(), though. > > And so far I've only seen people (privately) saying how good it would > > be to have bpf_link-based TC APIs, doesn't seem like anyone with a > > realistic use case prefers the current APIs. So I suspect it's not > > going to be a problem in practice. But at least I'd start there and > > see how people are using it and if they need anything else. > > *sigh* - I really wish you would stop arbitrarily declaring your own use > cases "realistic" and mine (implied) "unrealistic". Makes it really hard > to have a productive discussion... Well (sigh?..), this wasn't my intention, sorry you read it this way. But we had similar discussions when I was adding bpf_link-based XDP attach APIs. And guess what, now I see that samples/bpf/whatever_xdp is switched to bpf_link-based XDP, because that makes everything simpler and more reliable. What I also know is that in production we ran into multiple issues with anything that doesn't auto-detach on process exit/crash (unless pinned explicitly, of course). And that people that are trying to use TC right now are saying how having bpf_link-based TC APIs would make everything *simpler* and *safer*. So I don't know... I understand it might be convenient in some cases to not care about a lifetime of BPF programs you are attaching, but then there are usually explicit and intentional ways to achieve at least similar behavior with safety by default. So I guess call me unconvinced (yet? still?). Give it another shot, though. > > >> If we expose the low-level API I can elect to just use this if I know I > >> want netlink behaviour, but if bpf_program__attach_tc() is the only API > >> available it would at least need a flag to enforce one mode or the other > >> (I can see someone wanting to enforce kernel bpf_link semantics as well, > >> so a flag for either mode seems reasonable?). > > > > Sophisticated enough users can also do feature detection to know if > > it's going to work or not. > > Sure, but that won't help if there's no API to pick the attach mode they > want. I'm not intending to allow legacy kprobe APIs to be "chosen", for instance. Because I'm convinced it's a bad API that no one should use if they can use an FD-based one. It might be a different case for TC, who knows. I'd just start with safer APIs and then evaluate whether there is a real demand for less safe ones. It's just some minor refactoring and exposing more APIs, when/if we need them. > > > There are many ways to skin this cat. I'd prioritize bpf_link-based TC > > APIs to be added with legacy TC API as a fallback. > > I'm fine with adding that; I just want the functions implementing the TC > API to also be exported so users can use those if they prefer... > > -Toke >
On 4/15/21 1:19 AM, Andrii Nakryiko wrote: > On Wed, Apr 14, 2021 at 3:51 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >>> On Wed, Apr 14, 2021 at 3:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >>>>> On Tue, Apr 6, 2021 at 3:06 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >>>>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >>>>>>> On Sat, Apr 3, 2021 at 10:47 AM Alexei Starovoitov >>>>>>> <alexei.starovoitov@gmail.com> wrote: >>>>>>>> On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi wrote: >>>>>>>>> On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: >>>>>>>>>> On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: >>>>>>>>>>> [...] >>>>>>>>>> >>>>>>>>>> All of these things are messy because of tc legacy. bpf tried to follow tc style >>>>>>>>>> with cls and act distinction and it didn't quite work. cls with >>>>>>>>>> direct-action is the only >>>>>>>>>> thing that became mainstream while tc style attach wasn't really addressed. >>>>>>>>>> There were several incidents where tc had tens of thousands of progs attached >>>>>>>>>> because of this attach/query/index weirdness described above. >>>>>>>>>> I think the only way to address this properly is to introduce bpf_link style of >>>>>>>>>> attaching to tc. Such bpf_link would support ingress/egress only. >>>>>>>>>> direction-action will be implied. There won't be any index and query >>>>>>>>>> will be obvious. >>>>>>>>> >>>>>>>>> Note that we already have bpf_link support working (without support for pinning >>>>>>>>> ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle, >>>>>>>>> chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link >>>>>>>>> and are able to operate on the exact filter during release. >>>>>>>> >>>>>>>> Except they're not unique. The library can stash them, but something else >>>>>>>> doing detach via iproute2 or their own netlink calls will detach the prog. >>>>>>>> This other app can attach to the same spot a different prog and now >>>>>>>> bpf_link__destroy will be detaching somebody else prog. >>>>>>>> >>>>>>>>>> So I would like to propose to take this patch set a step further from >>>>>>>>>> what Daniel said: >>>>>>>>>> int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): >>>>>>>>>> and make this proposed api to return FD. >>>>>>>>>> To detach from tc ingress/egress just close(fd). >>>>>>>>> >>>>>>>>> You mean adding an fd-based TC API to the kernel? >>>>>>>> >>>>>>>> yes. >>>>>>> >>>>>>> I'm totally for bpf_link-based TC attachment. >>>>>>> >>>>>>> But I think *also* having "legacy" netlink-based APIs will allow >>>>>>> applications to handle older kernels in a much nicer way without extra >>>>>>> dependency on iproute2. We have a similar situation with kprobe, where >>>>>>> currently libbpf only supports "modern" fd-based attachment, but users >>>>>>> periodically ask questions and struggle to figure out issues on older >>>>>>> kernels that don't support new APIs. >>>>>> >>>>>> +1; I am OK with adding a new bpf_link-based way to attach TC programs, >>>>>> but we still need to support the netlink API in libbpf. >>>>>> >>>>>>> So I think we'd have to support legacy TC APIs, but I agree with >>>>>>> Alexei and Daniel that we should keep it to the simplest and most >>>>>>> straightforward API of supporting direction-action attachments and >>>>>>> setting up qdisc transparently (if I'm getting all the terminology >>>>>>> right, after reading Quentin's blog post). That coincidentally should >>>>>>> probably match how bpf_link-based TC API will look like, so all that >>>>>>> can be abstracted behind a single bpf_link__attach_tc() API as well, >>>>>>> right? That's the plan for dealing with kprobe right now, btw. Libbpf >>>>>>> will detect the best available API and transparently fall back (maybe >>>>>>> with some warning for awareness, due to inherent downsides of legacy >>>>>>> APIs: no auto-cleanup being the most prominent one). >>>>>> >>>>>> Yup, SGTM: Expose both in the low-level API (in bpf.c), and make the >>>>>> high-level API auto-detect. That way users can also still use the >>>>>> netlink attach function if they don't want the fd-based auto-close >>>>>> behaviour of bpf_link. >>>>> >>>>> So I thought a bit more about this, and it feels like the right move >>>>> would be to expose only higher-level TC BPF API behind bpf_link. It >>>>> will keep the API complexity and amount of APIs that libbpf will have >>>>> to support to the minimum, and will keep the API itself simple: >>>>> direct-attach with the minimum amount of input arguments. By not >>>>> exposing low-level APIs we also table the whole bpf_tc_cls_attach_id >>>>> design discussion, as we now can keep as much info as needed inside >>>>> bpf_link_tc (which will embed bpf_link internally as well) to support >>>>> detachment and possibly some additional querying, if needed. >>>> >>>> But then there would be no way for the caller to explicitly select a >>>> mechanism? I.e., if I write a BPF program using this mechanism targeting >>>> a 5.12 kernel, I'll get netlink attachment, which can stick around when >>>> I do bpf_link__disconnect(). But then if the kernel gets upgraded to >>>> support bpf_link for TC programs I'll suddenly transparently get >>>> bpf_link and the attachments will go away unless I pin them. This >>>> seems... less than ideal? >>> >>> That's what we are doing with bpf_program__attach_kprobe(), though. >>> And so far I've only seen people (privately) saying how good it would >>> be to have bpf_link-based TC APIs, doesn't seem like anyone with a >>> realistic use case prefers the current APIs. So I suspect it's not >>> going to be a problem in practice. But at least I'd start there and >>> see how people are using it and if they need anything else. >> >> *sigh* - I really wish you would stop arbitrarily declaring your own use >> cases "realistic" and mine (implied) "unrealistic". Makes it really hard >> to have a productive discussion... > > Well (sigh?..), this wasn't my intention, sorry you read it this way. > But we had similar discussions when I was adding bpf_link-based XDP > attach APIs. And guess what, now I see that samples/bpf/whatever_xdp > is switched to bpf_link-based XDP, because that makes everything > simpler and more reliable. What I also know is that in production we > ran into multiple issues with anything that doesn't auto-detach on > process exit/crash (unless pinned explicitly, of course). And that > people that are trying to use TC right now are saying how having > bpf_link-based TC APIs would make everything *simpler* and *safer*. So > I don't know... I understand it might be convenient in some cases to > not care about a lifetime of BPF programs you are attaching, but then > there are usually explicit and intentional ways to achieve at least > similar behavior with safety by default. [...] >>> There are many ways to skin this cat. I'd prioritize bpf_link-based TC >>> APIs to be added with legacy TC API as a fallback. I think the problem here is though that this would need to be deterministic when upgrading from one kernel version to another where we don't use the fallback anymore, e.g. in case of Cilium we always want to keep the progs attached to allow headless updates on the agent, meaning, traffic keeps flowing through the BPF datapath while in user space, our agent restarts after upgrade, and atomically replaces the BPF progs once up and running (we're doing this for the whole range of 4.9 to 5.x kernels that we support). While we use the 'simple' api that is discussed here internally in Cilium, this attach behavior would have to be consistent, so transparent fallback inside libbpf on link vs non-link availability won't work (at least in our case). > So I guess call me unconvinced (yet? still?). Give it another shot, though. > >>>> If we expose the low-level API I can elect to just use this if I know I >>>> want netlink behaviour, but if bpf_program__attach_tc() is the only API >>>> available it would at least need a flag to enforce one mode or the other >>>> (I can see someone wanting to enforce kernel bpf_link semantics as well, >>>> so a flag for either mode seems reasonable?). >>> >>> Sophisticated enough users can also do feature detection to know if >>> it's going to work or not. >> >> Sure, but that won't help if there's no API to pick the attach mode they >> want. > > I'm not intending to allow legacy kprobe APIs to be "chosen", for > instance. Because I'm convinced it's a bad API that no one should use > if they can use an FD-based one. It might be a different case for TC, > who knows. I'd just start with safer APIs and then evaluate whether > there is a real demand for less safe ones. It's just some minor > refactoring and exposing more APIs, when/if we need them. > >>> There are many ways to skin this cat. I'd prioritize bpf_link-based TC >>> APIs to be added with legacy TC API as a fallback. >> >> I'm fine with adding that; I just want the functions implementing the TC >> API to also be exported so users can use those if they prefer... >> >> -Toke
On Wed, Apr 14, 2021 at 4:32 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 4/15/21 1:19 AM, Andrii Nakryiko wrote: > > On Wed, Apr 14, 2021 at 3:51 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >>> On Wed, Apr 14, 2021 at 3:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >>>>> On Tue, Apr 6, 2021 at 3:06 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >>>>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >>>>>>> On Sat, Apr 3, 2021 at 10:47 AM Alexei Starovoitov > >>>>>>> <alexei.starovoitov@gmail.com> wrote: > >>>>>>>> On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi wrote: > >>>>>>>>> On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: > >>>>>>>>>> On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > >>>>>>>>>>> [...] > >>>>>>>>>> > >>>>>>>>>> All of these things are messy because of tc legacy. bpf tried to follow tc style > >>>>>>>>>> with cls and act distinction and it didn't quite work. cls with > >>>>>>>>>> direct-action is the only > >>>>>>>>>> thing that became mainstream while tc style attach wasn't really addressed. > >>>>>>>>>> There were several incidents where tc had tens of thousands of progs attached > >>>>>>>>>> because of this attach/query/index weirdness described above. > >>>>>>>>>> I think the only way to address this properly is to introduce bpf_link style of > >>>>>>>>>> attaching to tc. Such bpf_link would support ingress/egress only. > >>>>>>>>>> direction-action will be implied. There won't be any index and query > >>>>>>>>>> will be obvious. > >>>>>>>>> > >>>>>>>>> Note that we already have bpf_link support working (without support for pinning > >>>>>>>>> ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle, > >>>>>>>>> chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link > >>>>>>>>> and are able to operate on the exact filter during release. > >>>>>>>> > >>>>>>>> Except they're not unique. The library can stash them, but something else > >>>>>>>> doing detach via iproute2 or their own netlink calls will detach the prog. > >>>>>>>> This other app can attach to the same spot a different prog and now > >>>>>>>> bpf_link__destroy will be detaching somebody else prog. > >>>>>>>> > >>>>>>>>>> So I would like to propose to take this patch set a step further from > >>>>>>>>>> what Daniel said: > >>>>>>>>>> int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): > >>>>>>>>>> and make this proposed api to return FD. > >>>>>>>>>> To detach from tc ingress/egress just close(fd). > >>>>>>>>> > >>>>>>>>> You mean adding an fd-based TC API to the kernel? > >>>>>>>> > >>>>>>>> yes. > >>>>>>> > >>>>>>> I'm totally for bpf_link-based TC attachment. > >>>>>>> > >>>>>>> But I think *also* having "legacy" netlink-based APIs will allow > >>>>>>> applications to handle older kernels in a much nicer way without extra > >>>>>>> dependency on iproute2. We have a similar situation with kprobe, where > >>>>>>> currently libbpf only supports "modern" fd-based attachment, but users > >>>>>>> periodically ask questions and struggle to figure out issues on older > >>>>>>> kernels that don't support new APIs. > >>>>>> > >>>>>> +1; I am OK with adding a new bpf_link-based way to attach TC programs, > >>>>>> but we still need to support the netlink API in libbpf. > >>>>>> > >>>>>>> So I think we'd have to support legacy TC APIs, but I agree with > >>>>>>> Alexei and Daniel that we should keep it to the simplest and most > >>>>>>> straightforward API of supporting direction-action attachments and > >>>>>>> setting up qdisc transparently (if I'm getting all the terminology > >>>>>>> right, after reading Quentin's blog post). That coincidentally should > >>>>>>> probably match how bpf_link-based TC API will look like, so all that > >>>>>>> can be abstracted behind a single bpf_link__attach_tc() API as well, > >>>>>>> right? That's the plan for dealing with kprobe right now, btw. Libbpf > >>>>>>> will detect the best available API and transparently fall back (maybe > >>>>>>> with some warning for awareness, due to inherent downsides of legacy > >>>>>>> APIs: no auto-cleanup being the most prominent one). > >>>>>> > >>>>>> Yup, SGTM: Expose both in the low-level API (in bpf.c), and make the > >>>>>> high-level API auto-detect. That way users can also still use the > >>>>>> netlink attach function if they don't want the fd-based auto-close > >>>>>> behaviour of bpf_link. > >>>>> > >>>>> So I thought a bit more about this, and it feels like the right move > >>>>> would be to expose only higher-level TC BPF API behind bpf_link. It > >>>>> will keep the API complexity and amount of APIs that libbpf will have > >>>>> to support to the minimum, and will keep the API itself simple: > >>>>> direct-attach with the minimum amount of input arguments. By not > >>>>> exposing low-level APIs we also table the whole bpf_tc_cls_attach_id > >>>>> design discussion, as we now can keep as much info as needed inside > >>>>> bpf_link_tc (which will embed bpf_link internally as well) to support > >>>>> detachment and possibly some additional querying, if needed. > >>>> > >>>> But then there would be no way for the caller to explicitly select a > >>>> mechanism? I.e., if I write a BPF program using this mechanism targeting > >>>> a 5.12 kernel, I'll get netlink attachment, which can stick around when > >>>> I do bpf_link__disconnect(). But then if the kernel gets upgraded to > >>>> support bpf_link for TC programs I'll suddenly transparently get > >>>> bpf_link and the attachments will go away unless I pin them. This > >>>> seems... less than ideal? > >>> > >>> That's what we are doing with bpf_program__attach_kprobe(), though. > >>> And so far I've only seen people (privately) saying how good it would > >>> be to have bpf_link-based TC APIs, doesn't seem like anyone with a > >>> realistic use case prefers the current APIs. So I suspect it's not > >>> going to be a problem in practice. But at least I'd start there and > >>> see how people are using it and if they need anything else. > >> > >> *sigh* - I really wish you would stop arbitrarily declaring your own use > >> cases "realistic" and mine (implied) "unrealistic". Makes it really hard > >> to have a productive discussion... > > > > Well (sigh?..), this wasn't my intention, sorry you read it this way. > > But we had similar discussions when I was adding bpf_link-based XDP > > attach APIs. And guess what, now I see that samples/bpf/whatever_xdp > > is switched to bpf_link-based XDP, because that makes everything > > simpler and more reliable. What I also know is that in production we > > ran into multiple issues with anything that doesn't auto-detach on > > process exit/crash (unless pinned explicitly, of course). And that > > people that are trying to use TC right now are saying how having > > bpf_link-based TC APIs would make everything *simpler* and *safer*. So > > I don't know... I understand it might be convenient in some cases to > > not care about a lifetime of BPF programs you are attaching, but then > > there are usually explicit and intentional ways to achieve at least > > similar behavior with safety by default. > > [...] > > >>> There are many ways to skin this cat. I'd prioritize bpf_link-based TC > >>> APIs to be added with legacy TC API as a fallback. > > I think the problem here is though that this would need to be deterministic > when upgrading from one kernel version to another where we don't use the > fallback anymore, e.g. in case of Cilium we always want to keep the progs > attached to allow headless updates on the agent, meaning, traffic keeps > flowing through the BPF datapath while in user space, our agent restarts > after upgrade, and atomically replaces the BPF progs once up and running > (we're doing this for the whole range of 4.9 to 5.x kernels that we support). > While we use the 'simple' api that is discussed here internally in Cilium, > this attach behavior would have to be consistent, so transparent fallback > inside libbpf on link vs non-link availability won't work (at least in our > case). What about pinning? It's not exactly the same, but bpf_link could actually pin a BPF program, if using legacy TC, and pin bpf_link, if using bpf_link-based APIs. Of course before switching from iproute2 to libbpf APIs you'd need to design your applications to use pinning instead of relying implicitly on permanently attached BPF program. > > > So I guess call me unconvinced (yet? still?). Give it another shot, though. > > > >>>> If we expose the low-level API I can elect to just use this if I know I > >>>> want netlink behaviour, but if bpf_program__attach_tc() is the only API > >>>> available it would at least need a flag to enforce one mode or the other > >>>> (I can see someone wanting to enforce kernel bpf_link semantics as well, > >>>> so a flag for either mode seems reasonable?). > >>> > >>> Sophisticated enough users can also do feature detection to know if > >>> it's going to work or not. > >> > >> Sure, but that won't help if there's no API to pick the attach mode they > >> want. > > > > I'm not intending to allow legacy kprobe APIs to be "chosen", for > > instance. Because I'm convinced it's a bad API that no one should use > > if they can use an FD-based one. It might be a different case for TC, > > who knows. I'd just start with safer APIs and then evaluate whether > > there is a real demand for less safe ones. It's just some minor > > refactoring and exposing more APIs, when/if we need them. > > > >>> There are many ways to skin this cat. I'd prioritize bpf_link-based TC > >>> APIs to be added with legacy TC API as a fallback. > >> > >> I'm fine with adding that; I just want the functions implementing the TC > >> API to also be exported so users can use those if they prefer... > >> > >> -Toke >
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Wed, Apr 14, 2021 at 3:51 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >> >> > On Wed, Apr 14, 2021 at 3:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >> >> >> >> > On Tue, Apr 6, 2021 at 3:06 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> >> >> >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >> >> >> >> >> >> > On Sat, Apr 3, 2021 at 10:47 AM Alexei Starovoitov >> >> >> > <alexei.starovoitov@gmail.com> wrote: >> >> >> >> >> >> >> >> On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi wrote: >> >> >> >> > On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: >> >> >> >> > > On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: >> >> >> >> > > > [...] >> >> >> >> > > >> >> >> >> > > All of these things are messy because of tc legacy. bpf tried to follow tc style >> >> >> >> > > with cls and act distinction and it didn't quite work. cls with >> >> >> >> > > direct-action is the only >> >> >> >> > > thing that became mainstream while tc style attach wasn't really addressed. >> >> >> >> > > There were several incidents where tc had tens of thousands of progs attached >> >> >> >> > > because of this attach/query/index weirdness described above. >> >> >> >> > > I think the only way to address this properly is to introduce bpf_link style of >> >> >> >> > > attaching to tc. Such bpf_link would support ingress/egress only. >> >> >> >> > > direction-action will be implied. There won't be any index and query >> >> >> >> > > will be obvious. >> >> >> >> > >> >> >> >> > Note that we already have bpf_link support working (without support for pinning >> >> >> >> > ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle, >> >> >> >> > chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link >> >> >> >> > and are able to operate on the exact filter during release. >> >> >> >> >> >> >> >> Except they're not unique. The library can stash them, but something else >> >> >> >> doing detach via iproute2 or their own netlink calls will detach the prog. >> >> >> >> This other app can attach to the same spot a different prog and now >> >> >> >> bpf_link__destroy will be detaching somebody else prog. >> >> >> >> >> >> >> >> > > So I would like to propose to take this patch set a step further from >> >> >> >> > > what Daniel said: >> >> >> >> > > int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): >> >> >> >> > > and make this proposed api to return FD. >> >> >> >> > > To detach from tc ingress/egress just close(fd). >> >> >> >> > >> >> >> >> > You mean adding an fd-based TC API to the kernel? >> >> >> >> >> >> >> >> yes. >> >> >> > >> >> >> > I'm totally for bpf_link-based TC attachment. >> >> >> > >> >> >> > But I think *also* having "legacy" netlink-based APIs will allow >> >> >> > applications to handle older kernels in a much nicer way without extra >> >> >> > dependency on iproute2. We have a similar situation with kprobe, where >> >> >> > currently libbpf only supports "modern" fd-based attachment, but users >> >> >> > periodically ask questions and struggle to figure out issues on older >> >> >> > kernels that don't support new APIs. >> >> >> >> >> >> +1; I am OK with adding a new bpf_link-based way to attach TC programs, >> >> >> but we still need to support the netlink API in libbpf. >> >> >> >> >> >> > So I think we'd have to support legacy TC APIs, but I agree with >> >> >> > Alexei and Daniel that we should keep it to the simplest and most >> >> >> > straightforward API of supporting direction-action attachments and >> >> >> > setting up qdisc transparently (if I'm getting all the terminology >> >> >> > right, after reading Quentin's blog post). That coincidentally should >> >> >> > probably match how bpf_link-based TC API will look like, so all that >> >> >> > can be abstracted behind a single bpf_link__attach_tc() API as well, >> >> >> > right? That's the plan for dealing with kprobe right now, btw. Libbpf >> >> >> > will detect the best available API and transparently fall back (maybe >> >> >> > with some warning for awareness, due to inherent downsides of legacy >> >> >> > APIs: no auto-cleanup being the most prominent one). >> >> >> >> >> >> Yup, SGTM: Expose both in the low-level API (in bpf.c), and make the >> >> >> high-level API auto-detect. That way users can also still use the >> >> >> netlink attach function if they don't want the fd-based auto-close >> >> >> behaviour of bpf_link. >> >> > >> >> > So I thought a bit more about this, and it feels like the right move >> >> > would be to expose only higher-level TC BPF API behind bpf_link. It >> >> > will keep the API complexity and amount of APIs that libbpf will have >> >> > to support to the minimum, and will keep the API itself simple: >> >> > direct-attach with the minimum amount of input arguments. By not >> >> > exposing low-level APIs we also table the whole bpf_tc_cls_attach_id >> >> > design discussion, as we now can keep as much info as needed inside >> >> > bpf_link_tc (which will embed bpf_link internally as well) to support >> >> > detachment and possibly some additional querying, if needed. >> >> >> >> But then there would be no way for the caller to explicitly select a >> >> mechanism? I.e., if I write a BPF program using this mechanism targeting >> >> a 5.12 kernel, I'll get netlink attachment, which can stick around when >> >> I do bpf_link__disconnect(). But then if the kernel gets upgraded to >> >> support bpf_link for TC programs I'll suddenly transparently get >> >> bpf_link and the attachments will go away unless I pin them. This >> >> seems... less than ideal? >> > >> > That's what we are doing with bpf_program__attach_kprobe(), though. >> > And so far I've only seen people (privately) saying how good it would >> > be to have bpf_link-based TC APIs, doesn't seem like anyone with a >> > realistic use case prefers the current APIs. So I suspect it's not >> > going to be a problem in practice. But at least I'd start there and >> > see how people are using it and if they need anything else. >> >> *sigh* - I really wish you would stop arbitrarily declaring your own use >> cases "realistic" and mine (implied) "unrealistic". Makes it really hard >> to have a productive discussion... > > Well (sigh?..), this wasn't my intention, sorry you read it this way. > But we had similar discussions when I was adding bpf_link-based XDP > attach APIs. Great, thank you! And yeah, we did discuss exactly this before, which is where my mental sigh came from - I feel like we already covered this ground and that I'm just being dismissed with "that is not a real use case". But OK, I'll give it another shot, see below. > And guess what, now I see that samples/bpf/whatever_xdp is switched to > bpf_link-based XDP, because that makes everything simpler and more > reliable. What I also know is that in production we ran into multiple > issues with anything that doesn't auto-detach on process exit/crash > (unless pinned explicitly, of course). And that people that are trying > to use TC right now are saying how having bpf_link-based TC APIs would > make everything *simpler* and *safer*. So I don't know... I understand > it might be convenient in some cases to not care about a lifetime of > BPF programs you are attaching, but then there are usually explicit > and intentional ways to achieve at least similar behavior with safety > by default. > > So I guess call me unconvinced (yet? still?). Give it another shot, > though. I'm not arguing against adding bpf_link support, and I'm even fine with making it the default. As you say, there are plenty of use cases where the bpf_link semantics make sense, and the XDP programs in samples all fall in this category. So sure, let's add this support and make this convenient to use. But there are also use cases where the BPF program lifetime absolutely shouldn't follow that of the userspace application. This includes both applications that don't have a long-running daemon at all (like a firewall that just loads a ruleset at boot; xdp-filter is such an application in the BPF world, but I'm sure there are others). And daemons that use BPF as a data path and want the packets to keep flowing even when they restart, like Cilium as Daniel mentioned. So the latter category of applications need their BPF programs to be permanently attached to the interface. And sure, this can sorta be done by pinning the bpf_link; but not really, because then: - You incur a new dependency on bpffs, so you have to make sure that is mounted and that you can get at the particular fs instance you're using; the latter is especially painful if you switch namespaces. - Your BPF program lifetime is no longer tied to the interface, so you have to deal with garbage collecting your pinned files somehow. This is especially painful if you don't have a daemon. Together, these issues make bpf_link a much less compelling proposition, to the point where it's no longer the better API for these use cases, IMO. And I know that because I had to work around just these issues with bpf_link for xdp-tools. But I'm not even asking for the netlink API to be the default, I'm fine with bpf_link being the default and encouraged API. I'm just asking for a way to make it *possible* to select which attach mode I want. Either by a flag to bpf_program__attach_tc(), or by exposing the low-level bpf_tc_cls_*() netlink functions, like we do for XDP. >> >> If we expose the low-level API I can elect to just use this if I know I >> >> want netlink behaviour, but if bpf_program__attach_tc() is the only API >> >> available it would at least need a flag to enforce one mode or the other >> >> (I can see someone wanting to enforce kernel bpf_link semantics as well, >> >> so a flag for either mode seems reasonable?). >> > >> > Sophisticated enough users can also do feature detection to know if >> > it's going to work or not. >> >> Sure, but that won't help if there's no API to pick the attach mode they >> want. > > I'm not intending to allow legacy kprobe APIs to be "chosen", for > instance. Because I'm convinced it's a bad API that no one should use > if they can use an FD-based one. I'd tend to agree with you for the tracing APIs, actually. But a BPF-based data plane is different, as I tried to explain above. > It might be a different case for TC, who knows. I'd just start with > safer APIs and then evaluate whether there is a real demand for less > safe ones. It's just some minor refactoring and exposing more APIs, > when/if we need them. There you go again with the "real demand" argument. How can I read this in any other way than that you don't consider my use case "real" (as you just assured me above was not the case)? What do you consider "real demand"? -Toke
On Thu, Apr 15, 2021 at 8:57 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Wed, Apr 14, 2021 at 3:51 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> > >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >> > >> > On Wed, Apr 14, 2021 at 3:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> >> > >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >> >> > >> >> > On Tue, Apr 6, 2021 at 3:06 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> >> >> > >> >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >> >> >> > >> >> >> > On Sat, Apr 3, 2021 at 10:47 AM Alexei Starovoitov > >> >> >> > <alexei.starovoitov@gmail.com> wrote: > >> >> >> >> > >> >> >> >> On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi wrote: > >> >> >> >> > On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: > >> >> >> >> > > On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > >> >> >> >> > > > [...] > >> >> >> >> > > > >> >> >> >> > > All of these things are messy because of tc legacy. bpf tried to follow tc style > >> >> >> >> > > with cls and act distinction and it didn't quite work. cls with > >> >> >> >> > > direct-action is the only > >> >> >> >> > > thing that became mainstream while tc style attach wasn't really addressed. > >> >> >> >> > > There were several incidents where tc had tens of thousands of progs attached > >> >> >> >> > > because of this attach/query/index weirdness described above. > >> >> >> >> > > I think the only way to address this properly is to introduce bpf_link style of > >> >> >> >> > > attaching to tc. Such bpf_link would support ingress/egress only. > >> >> >> >> > > direction-action will be implied. There won't be any index and query > >> >> >> >> > > will be obvious. > >> >> >> >> > > >> >> >> >> > Note that we already have bpf_link support working (without support for pinning > >> >> >> >> > ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle, > >> >> >> >> > chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link > >> >> >> >> > and are able to operate on the exact filter during release. > >> >> >> >> > >> >> >> >> Except they're not unique. The library can stash them, but something else > >> >> >> >> doing detach via iproute2 or their own netlink calls will detach the prog. > >> >> >> >> This other app can attach to the same spot a different prog and now > >> >> >> >> bpf_link__destroy will be detaching somebody else prog. > >> >> >> >> > >> >> >> >> > > So I would like to propose to take this patch set a step further from > >> >> >> >> > > what Daniel said: > >> >> >> >> > > int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): > >> >> >> >> > > and make this proposed api to return FD. > >> >> >> >> > > To detach from tc ingress/egress just close(fd). > >> >> >> >> > > >> >> >> >> > You mean adding an fd-based TC API to the kernel? > >> >> >> >> > >> >> >> >> yes. > >> >> >> > > >> >> >> > I'm totally for bpf_link-based TC attachment. > >> >> >> > > >> >> >> > But I think *also* having "legacy" netlink-based APIs will allow > >> >> >> > applications to handle older kernels in a much nicer way without extra > >> >> >> > dependency on iproute2. We have a similar situation with kprobe, where > >> >> >> > currently libbpf only supports "modern" fd-based attachment, but users > >> >> >> > periodically ask questions and struggle to figure out issues on older > >> >> >> > kernels that don't support new APIs. > >> >> >> > >> >> >> +1; I am OK with adding a new bpf_link-based way to attach TC programs, > >> >> >> but we still need to support the netlink API in libbpf. > >> >> >> > >> >> >> > So I think we'd have to support legacy TC APIs, but I agree with > >> >> >> > Alexei and Daniel that we should keep it to the simplest and most > >> >> >> > straightforward API of supporting direction-action attachments and > >> >> >> > setting up qdisc transparently (if I'm getting all the terminology > >> >> >> > right, after reading Quentin's blog post). That coincidentally should > >> >> >> > probably match how bpf_link-based TC API will look like, so all that > >> >> >> > can be abstracted behind a single bpf_link__attach_tc() API as well, > >> >> >> > right? That's the plan for dealing with kprobe right now, btw. Libbpf > >> >> >> > will detect the best available API and transparently fall back (maybe > >> >> >> > with some warning for awareness, due to inherent downsides of legacy > >> >> >> > APIs: no auto-cleanup being the most prominent one). > >> >> >> > >> >> >> Yup, SGTM: Expose both in the low-level API (in bpf.c), and make the > >> >> >> high-level API auto-detect. That way users can also still use the > >> >> >> netlink attach function if they don't want the fd-based auto-close > >> >> >> behaviour of bpf_link. > >> >> > > >> >> > So I thought a bit more about this, and it feels like the right move > >> >> > would be to expose only higher-level TC BPF API behind bpf_link. It > >> >> > will keep the API complexity and amount of APIs that libbpf will have > >> >> > to support to the minimum, and will keep the API itself simple: > >> >> > direct-attach with the minimum amount of input arguments. By not > >> >> > exposing low-level APIs we also table the whole bpf_tc_cls_attach_id > >> >> > design discussion, as we now can keep as much info as needed inside > >> >> > bpf_link_tc (which will embed bpf_link internally as well) to support > >> >> > detachment and possibly some additional querying, if needed. > >> >> > >> >> But then there would be no way for the caller to explicitly select a > >> >> mechanism? I.e., if I write a BPF program using this mechanism targeting > >> >> a 5.12 kernel, I'll get netlink attachment, which can stick around when > >> >> I do bpf_link__disconnect(). But then if the kernel gets upgraded to > >> >> support bpf_link for TC programs I'll suddenly transparently get > >> >> bpf_link and the attachments will go away unless I pin them. This > >> >> seems... less than ideal? > >> > > >> > That's what we are doing with bpf_program__attach_kprobe(), though. > >> > And so far I've only seen people (privately) saying how good it would > >> > be to have bpf_link-based TC APIs, doesn't seem like anyone with a > >> > realistic use case prefers the current APIs. So I suspect it's not > >> > going to be a problem in practice. But at least I'd start there and > >> > see how people are using it and if they need anything else. > >> > >> *sigh* - I really wish you would stop arbitrarily declaring your own use > >> cases "realistic" and mine (implied) "unrealistic". Makes it really hard > >> to have a productive discussion... > > > > Well (sigh?..), this wasn't my intention, sorry you read it this way. > > But we had similar discussions when I was adding bpf_link-based XDP > > attach APIs. > > Great, thank you! And yeah, we did discuss exactly this before, which is > where my mental sigh came from - I feel like we already covered this > ground and that I'm just being dismissed with "that is not a real use > case". But OK, I'll give it another shot, see below. > > > And guess what, now I see that samples/bpf/whatever_xdp is switched to > > bpf_link-based XDP, because that makes everything simpler and more > > reliable. What I also know is that in production we ran into multiple > > issues with anything that doesn't auto-detach on process exit/crash > > (unless pinned explicitly, of course). And that people that are trying > > to use TC right now are saying how having bpf_link-based TC APIs would > > make everything *simpler* and *safer*. So I don't know... I understand > > it might be convenient in some cases to not care about a lifetime of > > BPF programs you are attaching, but then there are usually explicit > > and intentional ways to achieve at least similar behavior with safety > > by default. > > > > So I guess call me unconvinced (yet? still?). Give it another shot, > > though. > > I'm not arguing against adding bpf_link support, and I'm even fine with > making it the default. As you say, there are plenty of use cases where > the bpf_link semantics make sense, and the XDP programs in samples all > fall in this category. So sure, let's add this support and make this > convenient to use. > > But there are also use cases where the BPF program lifetime absolutely > shouldn't follow that of the userspace application. This includes both > applications that don't have a long-running daemon at all (like a > firewall that just loads a ruleset at boot; xdp-filter is such an > application in the BPF world, but I'm sure there are others). And > daemons that use BPF as a data path and want the packets to keep flowing > even when they restart, like Cilium as Daniel mentioned. > > So the latter category of applications need their BPF programs to be > permanently attached to the interface. And sure, this can sorta be done > by pinning the bpf_link; but not really, because then: > > - You incur a new dependency on bpffs, so you have to make sure that is > mounted and that you can get at the particular fs instance you're > using; the latter is especially painful if you switch namespaces. So I understand that it's more painful than current TC and legacy XDP APIs for these specific use cases. But for other types of BPF programs that want to persist across user-space process exit, all those needs to be addressed and designed around anyway. And that doesn't prevent others from doing it, otherwise we wouldn't even implement BPFFS. > > - Your BPF program lifetime is no longer tied to the interface, so you > have to deal with garbage collecting your pinned files somehow. This > is especially painful if you don't have a daemon. With bpf_link auto-detach, you'll still get the underlying BPF program freed, only a small bpf_link shell will persist. If you churn through interface going up/down, whichever application is responsible for re-attaching BPF programs would deal with clean up. But yes, I'm not oblivious to the need to change how you design your applications and new inconveniences that would cause. But think about this from a slightly different point of view. If we had to choose between bpf_link model and current TC API, which one would we choose? I'd argue we should choose bpf_link, because: 1) it allows to do things more safely (auto-cleanup) by default; 2) it still allows for having BPF program/link persistence, even if in a different and slightly more inconvenient way. So it's a more generic and powerful approach, it's just not as perfectly aligned with the way it's used for cases you mentioned. > > Together, these issues make bpf_link a much less compelling proposition, > to the point where it's no longer the better API for these use cases, > IMO. And I know that because I had to work around just these issues with > bpf_link for xdp-tools. > > But I'm not even asking for the netlink API to be the default, I'm > fine with bpf_link being the default and encouraged API. I'm just asking > for a way to make it *possible* to select which attach mode I want. > Either by a flag to bpf_program__attach_tc(), or by exposing the > low-level bpf_tc_cls_*() netlink functions, like we do for XDP. Given significant semantic differences between bpf_link and current TC APIs, I'm not sure anymore if it's a good idea to hide current API behind bpf_link abstraction. For tracing it's a good fit, so I still think falling back to legacy kprobe API makes sense there. For TC, given your and Daniel's replies, I'd rather have low-level APIs exposed directly instead of having some options switch in bpf_program__attach_tc() API. > > >> >> If we expose the low-level API I can elect to just use this if I know I > >> >> want netlink behaviour, but if bpf_program__attach_tc() is the only API > >> >> available it would at least need a flag to enforce one mode or the other > >> >> (I can see someone wanting to enforce kernel bpf_link semantics as well, > >> >> so a flag for either mode seems reasonable?). > >> > > >> > Sophisticated enough users can also do feature detection to know if > >> > it's going to work or not. > >> > >> Sure, but that won't help if there's no API to pick the attach mode they > >> want. > > > > I'm not intending to allow legacy kprobe APIs to be "chosen", for > > instance. Because I'm convinced it's a bad API that no one should use > > if they can use an FD-based one. > > I'd tend to agree with you for the tracing APIs, actually. But a > BPF-based data plane is different, as I tried to explain above. Right, see above. > > > It might be a different case for TC, who knows. I'd just start with > > safer APIs and then evaluate whether there is a real demand for less > > safe ones. It's just some minor refactoring and exposing more APIs, > > when/if we need them. > > There you go again with the "real demand" argument. How can I read this > in any other way than that you don't consider my use case "real" (as you > just assured me above was not the case)? What do you consider "real > demand"? Please try not to read more in my words than there really is. By real demand I meant at least few *different* use cases coming from different parties. That says nothing about whether your use case is real or not, just that it's a one case (or at least coming from just one party). > > -Toke >
On 4/15/21 1:58 AM, Andrii Nakryiko wrote: > On Wed, Apr 14, 2021 at 4:32 PM Daniel Borkmann <daniel@iogearbox.net> wrote: >> On 4/15/21 1:19 AM, Andrii Nakryiko wrote: >>> On Wed, Apr 14, 2021 at 3:51 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >>>>> On Wed, Apr 14, 2021 at 3:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >>>>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >>>>>>> On Tue, Apr 6, 2021 at 3:06 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >>>>>>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >>>>>>>>> On Sat, Apr 3, 2021 at 10:47 AM Alexei Starovoitov >>>>>>>>> <alexei.starovoitov@gmail.com> wrote: >>>>>>>>>> On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi wrote: >>>>>>>>>>> On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: >>>>>>>>>>>> On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: >>>>>>>>>>>>> [...] >>>>>>>>>>>> >>>>>>>>>>>> All of these things are messy because of tc legacy. bpf tried to follow tc style >>>>>>>>>>>> with cls and act distinction and it didn't quite work. cls with >>>>>>>>>>>> direct-action is the only >>>>>>>>>>>> thing that became mainstream while tc style attach wasn't really addressed. >>>>>>>>>>>> There were several incidents where tc had tens of thousands of progs attached >>>>>>>>>>>> because of this attach/query/index weirdness described above. >>>>>>>>>>>> I think the only way to address this properly is to introduce bpf_link style of >>>>>>>>>>>> attaching to tc. Such bpf_link would support ingress/egress only. >>>>>>>>>>>> direction-action will be implied. There won't be any index and query >>>>>>>>>>>> will be obvious. >>>>>>>>>>> >>>>>>>>>>> Note that we already have bpf_link support working (without support for pinning >>>>>>>>>>> ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle, >>>>>>>>>>> chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link >>>>>>>>>>> and are able to operate on the exact filter during release. >>>>>>>>>> >>>>>>>>>> Except they're not unique. The library can stash them, but something else >>>>>>>>>> doing detach via iproute2 or their own netlink calls will detach the prog. >>>>>>>>>> This other app can attach to the same spot a different prog and now >>>>>>>>>> bpf_link__destroy will be detaching somebody else prog. >>>>>>>>>> >>>>>>>>>>>> So I would like to propose to take this patch set a step further from >>>>>>>>>>>> what Daniel said: >>>>>>>>>>>> int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): >>>>>>>>>>>> and make this proposed api to return FD. >>>>>>>>>>>> To detach from tc ingress/egress just close(fd). >>>>>>>>>>> >>>>>>>>>>> You mean adding an fd-based TC API to the kernel? >>>>>>>>>> >>>>>>>>>> yes. >>>>>>>>> >>>>>>>>> I'm totally for bpf_link-based TC attachment. >>>>>>>>> >>>>>>>>> But I think *also* having "legacy" netlink-based APIs will allow >>>>>>>>> applications to handle older kernels in a much nicer way without extra >>>>>>>>> dependency on iproute2. We have a similar situation with kprobe, where >>>>>>>>> currently libbpf only supports "modern" fd-based attachment, but users >>>>>>>>> periodically ask questions and struggle to figure out issues on older >>>>>>>>> kernels that don't support new APIs. >>>>>>>> >>>>>>>> +1; I am OK with adding a new bpf_link-based way to attach TC programs, >>>>>>>> but we still need to support the netlink API in libbpf. >>>>>>>> >>>>>>>>> So I think we'd have to support legacy TC APIs, but I agree with >>>>>>>>> Alexei and Daniel that we should keep it to the simplest and most >>>>>>>>> straightforward API of supporting direction-action attachments and >>>>>>>>> setting up qdisc transparently (if I'm getting all the terminology >>>>>>>>> right, after reading Quentin's blog post). That coincidentally should >>>>>>>>> probably match how bpf_link-based TC API will look like, so all that >>>>>>>>> can be abstracted behind a single bpf_link__attach_tc() API as well, >>>>>>>>> right? That's the plan for dealing with kprobe right now, btw. Libbpf >>>>>>>>> will detect the best available API and transparently fall back (maybe >>>>>>>>> with some warning for awareness, due to inherent downsides of legacy >>>>>>>>> APIs: no auto-cleanup being the most prominent one). >>>>>>>> >>>>>>>> Yup, SGTM: Expose both in the low-level API (in bpf.c), and make the >>>>>>>> high-level API auto-detect. That way users can also still use the >>>>>>>> netlink attach function if they don't want the fd-based auto-close >>>>>>>> behaviour of bpf_link. >>>>>>> >>>>>>> So I thought a bit more about this, and it feels like the right move >>>>>>> would be to expose only higher-level TC BPF API behind bpf_link. It >>>>>>> will keep the API complexity and amount of APIs that libbpf will have >>>>>>> to support to the minimum, and will keep the API itself simple: >>>>>>> direct-attach with the minimum amount of input arguments. By not >>>>>>> exposing low-level APIs we also table the whole bpf_tc_cls_attach_id >>>>>>> design discussion, as we now can keep as much info as needed inside >>>>>>> bpf_link_tc (which will embed bpf_link internally as well) to support >>>>>>> detachment and possibly some additional querying, if needed. >>>>>> >>>>>> But then there would be no way for the caller to explicitly select a >>>>>> mechanism? I.e., if I write a BPF program using this mechanism targeting >>>>>> a 5.12 kernel, I'll get netlink attachment, which can stick around when >>>>>> I do bpf_link__disconnect(). But then if the kernel gets upgraded to >>>>>> support bpf_link for TC programs I'll suddenly transparently get >>>>>> bpf_link and the attachments will go away unless I pin them. This >>>>>> seems... less than ideal? >>>>> >>>>> That's what we are doing with bpf_program__attach_kprobe(), though. >>>>> And so far I've only seen people (privately) saying how good it would >>>>> be to have bpf_link-based TC APIs, doesn't seem like anyone with a >>>>> realistic use case prefers the current APIs. So I suspect it's not >>>>> going to be a problem in practice. But at least I'd start there and >>>>> see how people are using it and if they need anything else. >>>> >>>> *sigh* - I really wish you would stop arbitrarily declaring your own use >>>> cases "realistic" and mine (implied) "unrealistic". Makes it really hard >>>> to have a productive discussion... >>> >>> Well (sigh?..), this wasn't my intention, sorry you read it this way. >>> But we had similar discussions when I was adding bpf_link-based XDP >>> attach APIs. And guess what, now I see that samples/bpf/whatever_xdp >>> is switched to bpf_link-based XDP, because that makes everything >>> simpler and more reliable. What I also know is that in production we >>> ran into multiple issues with anything that doesn't auto-detach on >>> process exit/crash (unless pinned explicitly, of course). And that >>> people that are trying to use TC right now are saying how having >>> bpf_link-based TC APIs would make everything *simpler* and *safer*. So >>> I don't know... I understand it might be convenient in some cases to >>> not care about a lifetime of BPF programs you are attaching, but then >>> there are usually explicit and intentional ways to achieve at least >>> similar behavior with safety by default. >> >> [...] >> >> >>> There are many ways to skin this cat. I'd prioritize bpf_link-based TC >> >>> APIs to be added with legacy TC API as a fallback. >> >> I think the problem here is though that this would need to be deterministic >> when upgrading from one kernel version to another where we don't use the >> fallback anymore, e.g. in case of Cilium we always want to keep the progs >> attached to allow headless updates on the agent, meaning, traffic keeps >> flowing through the BPF datapath while in user space, our agent restarts >> after upgrade, and atomically replaces the BPF progs once up and running >> (we're doing this for the whole range of 4.9 to 5.x kernels that we support). >> While we use the 'simple' api that is discussed here internally in Cilium, >> this attach behavior would have to be consistent, so transparent fallback >> inside libbpf on link vs non-link availability won't work (at least in our >> case). > > What about pinning? It's not exactly the same, but bpf_link could > actually pin a BPF program, if using legacy TC, and pin bpf_link, if > using bpf_link-based APIs. Of course before switching from iproute2 to > libbpf APIs you'd need to design your applications to use pinning > instead of relying implicitly on permanently attached BPF program. All the progs we load from Cilium in a K8s setting w/ Pods, we could have easily over 100 loaded at the same time on a node, and we template the per Pod ones, so the complexity of managing those pinned lifecycles from the agent and dealing with the semantic/fallback differences between kernels feels probably not worth the gain. So if there would be a libbpf tc simplified attach API, I'd for the time being stick to the existing aka legacy means. Thanks, Daniel
On Thu, Apr 15, 2021 at 3:10 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 4/15/21 1:58 AM, Andrii Nakryiko wrote: > > On Wed, Apr 14, 2021 at 4:32 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > >> On 4/15/21 1:19 AM, Andrii Nakryiko wrote: > >>> On Wed, Apr 14, 2021 at 3:51 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >>>>> On Wed, Apr 14, 2021 at 3:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >>>>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >>>>>>> On Tue, Apr 6, 2021 at 3:06 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >>>>>>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >>>>>>>>> On Sat, Apr 3, 2021 at 10:47 AM Alexei Starovoitov > >>>>>>>>> <alexei.starovoitov@gmail.com> wrote: > >>>>>>>>>> On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi wrote: > >>>>>>>>>>> On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: > >>>>>>>>>>>> On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > >>>>>>>>>>>>> [...] > >>>>>>>>>>>> > >>>>>>>>>>>> All of these things are messy because of tc legacy. bpf tried to follow tc style > >>>>>>>>>>>> with cls and act distinction and it didn't quite work. cls with > >>>>>>>>>>>> direct-action is the only > >>>>>>>>>>>> thing that became mainstream while tc style attach wasn't really addressed. > >>>>>>>>>>>> There were several incidents where tc had tens of thousands of progs attached > >>>>>>>>>>>> because of this attach/query/index weirdness described above. > >>>>>>>>>>>> I think the only way to address this properly is to introduce bpf_link style of > >>>>>>>>>>>> attaching to tc. Such bpf_link would support ingress/egress only. > >>>>>>>>>>>> direction-action will be implied. There won't be any index and query > >>>>>>>>>>>> will be obvious. > >>>>>>>>>>> > >>>>>>>>>>> Note that we already have bpf_link support working (without support for pinning > >>>>>>>>>>> ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle, > >>>>>>>>>>> chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link > >>>>>>>>>>> and are able to operate on the exact filter during release. > >>>>>>>>>> > >>>>>>>>>> Except they're not unique. The library can stash them, but something else > >>>>>>>>>> doing detach via iproute2 or their own netlink calls will detach the prog. > >>>>>>>>>> This other app can attach to the same spot a different prog and now > >>>>>>>>>> bpf_link__destroy will be detaching somebody else prog. > >>>>>>>>>> > >>>>>>>>>>>> So I would like to propose to take this patch set a step further from > >>>>>>>>>>>> what Daniel said: > >>>>>>>>>>>> int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): > >>>>>>>>>>>> and make this proposed api to return FD. > >>>>>>>>>>>> To detach from tc ingress/egress just close(fd). > >>>>>>>>>>> > >>>>>>>>>>> You mean adding an fd-based TC API to the kernel? > >>>>>>>>>> > >>>>>>>>>> yes. > >>>>>>>>> > >>>>>>>>> I'm totally for bpf_link-based TC attachment. > >>>>>>>>> > >>>>>>>>> But I think *also* having "legacy" netlink-based APIs will allow > >>>>>>>>> applications to handle older kernels in a much nicer way without extra > >>>>>>>>> dependency on iproute2. We have a similar situation with kprobe, where > >>>>>>>>> currently libbpf only supports "modern" fd-based attachment, but users > >>>>>>>>> periodically ask questions and struggle to figure out issues on older > >>>>>>>>> kernels that don't support new APIs. > >>>>>>>> > >>>>>>>> +1; I am OK with adding a new bpf_link-based way to attach TC programs, > >>>>>>>> but we still need to support the netlink API in libbpf. > >>>>>>>> > >>>>>>>>> So I think we'd have to support legacy TC APIs, but I agree with > >>>>>>>>> Alexei and Daniel that we should keep it to the simplest and most > >>>>>>>>> straightforward API of supporting direction-action attachments and > >>>>>>>>> setting up qdisc transparently (if I'm getting all the terminology > >>>>>>>>> right, after reading Quentin's blog post). That coincidentally should > >>>>>>>>> probably match how bpf_link-based TC API will look like, so all that > >>>>>>>>> can be abstracted behind a single bpf_link__attach_tc() API as well, > >>>>>>>>> right? That's the plan for dealing with kprobe right now, btw. Libbpf > >>>>>>>>> will detect the best available API and transparently fall back (maybe > >>>>>>>>> with some warning for awareness, due to inherent downsides of legacy > >>>>>>>>> APIs: no auto-cleanup being the most prominent one). > >>>>>>>> > >>>>>>>> Yup, SGTM: Expose both in the low-level API (in bpf.c), and make the > >>>>>>>> high-level API auto-detect. That way users can also still use the > >>>>>>>> netlink attach function if they don't want the fd-based auto-close > >>>>>>>> behaviour of bpf_link. > >>>>>>> > >>>>>>> So I thought a bit more about this, and it feels like the right move > >>>>>>> would be to expose only higher-level TC BPF API behind bpf_link. It > >>>>>>> will keep the API complexity and amount of APIs that libbpf will have > >>>>>>> to support to the minimum, and will keep the API itself simple: > >>>>>>> direct-attach with the minimum amount of input arguments. By not > >>>>>>> exposing low-level APIs we also table the whole bpf_tc_cls_attach_id > >>>>>>> design discussion, as we now can keep as much info as needed inside > >>>>>>> bpf_link_tc (which will embed bpf_link internally as well) to support > >>>>>>> detachment and possibly some additional querying, if needed. > >>>>>> > >>>>>> But then there would be no way for the caller to explicitly select a > >>>>>> mechanism? I.e., if I write a BPF program using this mechanism targeting > >>>>>> a 5.12 kernel, I'll get netlink attachment, which can stick around when > >>>>>> I do bpf_link__disconnect(). But then if the kernel gets upgraded to > >>>>>> support bpf_link for TC programs I'll suddenly transparently get > >>>>>> bpf_link and the attachments will go away unless I pin them. This > >>>>>> seems... less than ideal? > >>>>> > >>>>> That's what we are doing with bpf_program__attach_kprobe(), though. > >>>>> And so far I've only seen people (privately) saying how good it would > >>>>> be to have bpf_link-based TC APIs, doesn't seem like anyone with a > >>>>> realistic use case prefers the current APIs. So I suspect it's not > >>>>> going to be a problem in practice. But at least I'd start there and > >>>>> see how people are using it and if they need anything else. > >>>> > >>>> *sigh* - I really wish you would stop arbitrarily declaring your own use > >>>> cases "realistic" and mine (implied) "unrealistic". Makes it really hard > >>>> to have a productive discussion... > >>> > >>> Well (sigh?..), this wasn't my intention, sorry you read it this way. > >>> But we had similar discussions when I was adding bpf_link-based XDP > >>> attach APIs. And guess what, now I see that samples/bpf/whatever_xdp > >>> is switched to bpf_link-based XDP, because that makes everything > >>> simpler and more reliable. What I also know is that in production we > >>> ran into multiple issues with anything that doesn't auto-detach on > >>> process exit/crash (unless pinned explicitly, of course). And that > >>> people that are trying to use TC right now are saying how having > >>> bpf_link-based TC APIs would make everything *simpler* and *safer*. So > >>> I don't know... I understand it might be convenient in some cases to > >>> not care about a lifetime of BPF programs you are attaching, but then > >>> there are usually explicit and intentional ways to achieve at least > >>> similar behavior with safety by default. > >> > >> [...] > >> > >> >>> There are many ways to skin this cat. I'd prioritize bpf_link-based TC > >> >>> APIs to be added with legacy TC API as a fallback. > >> > >> I think the problem here is though that this would need to be deterministic > >> when upgrading from one kernel version to another where we don't use the > >> fallback anymore, e.g. in case of Cilium we always want to keep the progs > >> attached to allow headless updates on the agent, meaning, traffic keeps > >> flowing through the BPF datapath while in user space, our agent restarts > >> after upgrade, and atomically replaces the BPF progs once up and running > >> (we're doing this for the whole range of 4.9 to 5.x kernels that we support). > >> While we use the 'simple' api that is discussed here internally in Cilium, > >> this attach behavior would have to be consistent, so transparent fallback > >> inside libbpf on link vs non-link availability won't work (at least in our > >> case). > > > > What about pinning? It's not exactly the same, but bpf_link could > > actually pin a BPF program, if using legacy TC, and pin bpf_link, if > > using bpf_link-based APIs. Of course before switching from iproute2 to > > libbpf APIs you'd need to design your applications to use pinning > > instead of relying implicitly on permanently attached BPF program. > > All the progs we load from Cilium in a K8s setting w/ Pods, we could have easily > over 100 loaded at the same time on a node, and we template the per Pod ones, so > the complexity of managing those pinned lifecycles from the agent and dealing with > the semantic/fallback differences between kernels feels probably not worth the > gain. So if there would be a libbpf tc simplified attach API, I'd for the time > being stick to the existing aka legacy means. Sure. Then what do you think about keeping only low-level TC APIs, and in the future add bpf_program__attach_tc(), which will use bpf_link-based one. It seems like it's not worth it to pretend we have bpf_link-based semantics with "legacy" current TC APIs. Similarly how we have a low-level XDP attach API, and bpf_link-based (only) bpf_program__attach_xdp(). > > Thanks, > Daniel
On 4/16/21 12:22 AM, Andrii Nakryiko wrote: > On Thu, Apr 15, 2021 at 3:10 PM Daniel Borkmann <daniel@iogearbox.net> wrote: >> On 4/15/21 1:58 AM, Andrii Nakryiko wrote: >>> On Wed, Apr 14, 2021 at 4:32 PM Daniel Borkmann <daniel@iogearbox.net> wrote: >>>> On 4/15/21 1:19 AM, Andrii Nakryiko wrote: >>>>> On Wed, Apr 14, 2021 at 3:51 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >>>>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >>>>>>> On Wed, Apr 14, 2021 at 3:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >>>>>>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >>>>>>>>> On Tue, Apr 6, 2021 at 3:06 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >>>>>>>>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >>>>>>>>>>> On Sat, Apr 3, 2021 at 10:47 AM Alexei Starovoitov >>>>>>>>>>> <alexei.starovoitov@gmail.com> wrote: >>>>>>>>>>>> On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi wrote: >>>>>>>>>>>>> On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: >>>>>>>>>>>>>> On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: >>>>>>>>>>>>>>> [...] >>>>>>>>>>>>>> >>>>>>>>>>>>>> All of these things are messy because of tc legacy. bpf tried to follow tc style >>>>>>>>>>>>>> with cls and act distinction and it didn't quite work. cls with >>>>>>>>>>>>>> direct-action is the only >>>>>>>>>>>>>> thing that became mainstream while tc style attach wasn't really addressed. >>>>>>>>>>>>>> There were several incidents where tc had tens of thousands of progs attached >>>>>>>>>>>>>> because of this attach/query/index weirdness described above. >>>>>>>>>>>>>> I think the only way to address this properly is to introduce bpf_link style of >>>>>>>>>>>>>> attaching to tc. Such bpf_link would support ingress/egress only. >>>>>>>>>>>>>> direction-action will be implied. There won't be any index and query >>>>>>>>>>>>>> will be obvious. >>>>>>>>>>>>> >>>>>>>>>>>>> Note that we already have bpf_link support working (without support for pinning >>>>>>>>>>>>> ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle, >>>>>>>>>>>>> chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link >>>>>>>>>>>>> and are able to operate on the exact filter during release. >>>>>>>>>>>> >>>>>>>>>>>> Except they're not unique. The library can stash them, but something else >>>>>>>>>>>> doing detach via iproute2 or their own netlink calls will detach the prog. >>>>>>>>>>>> This other app can attach to the same spot a different prog and now >>>>>>>>>>>> bpf_link__destroy will be detaching somebody else prog. >>>>>>>>>>>> >>>>>>>>>>>>>> So I would like to propose to take this patch set a step further from >>>>>>>>>>>>>> what Daniel said: >>>>>>>>>>>>>> int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): >>>>>>>>>>>>>> and make this proposed api to return FD. >>>>>>>>>>>>>> To detach from tc ingress/egress just close(fd). >>>>>>>>>>>>> >>>>>>>>>>>>> You mean adding an fd-based TC API to the kernel? >>>>>>>>>>>> >>>>>>>>>>>> yes. >>>>>>>>>>> >>>>>>>>>>> I'm totally for bpf_link-based TC attachment. >>>>>>>>>>> >>>>>>>>>>> But I think *also* having "legacy" netlink-based APIs will allow >>>>>>>>>>> applications to handle older kernels in a much nicer way without extra >>>>>>>>>>> dependency on iproute2. We have a similar situation with kprobe, where >>>>>>>>>>> currently libbpf only supports "modern" fd-based attachment, but users >>>>>>>>>>> periodically ask questions and struggle to figure out issues on older >>>>>>>>>>> kernels that don't support new APIs. >>>>>>>>>> >>>>>>>>>> +1; I am OK with adding a new bpf_link-based way to attach TC programs, >>>>>>>>>> but we still need to support the netlink API in libbpf. >>>>>>>>>> >>>>>>>>>>> So I think we'd have to support legacy TC APIs, but I agree with >>>>>>>>>>> Alexei and Daniel that we should keep it to the simplest and most >>>>>>>>>>> straightforward API of supporting direction-action attachments and >>>>>>>>>>> setting up qdisc transparently (if I'm getting all the terminology >>>>>>>>>>> right, after reading Quentin's blog post). That coincidentally should >>>>>>>>>>> probably match how bpf_link-based TC API will look like, so all that >>>>>>>>>>> can be abstracted behind a single bpf_link__attach_tc() API as well, >>>>>>>>>>> right? That's the plan for dealing with kprobe right now, btw. Libbpf >>>>>>>>>>> will detect the best available API and transparently fall back (maybe >>>>>>>>>>> with some warning for awareness, due to inherent downsides of legacy >>>>>>>>>>> APIs: no auto-cleanup being the most prominent one). >>>>>>>>>> >>>>>>>>>> Yup, SGTM: Expose both in the low-level API (in bpf.c), and make the >>>>>>>>>> high-level API auto-detect. That way users can also still use the >>>>>>>>>> netlink attach function if they don't want the fd-based auto-close >>>>>>>>>> behaviour of bpf_link. >>>>>>>>> >>>>>>>>> So I thought a bit more about this, and it feels like the right move >>>>>>>>> would be to expose only higher-level TC BPF API behind bpf_link. It >>>>>>>>> will keep the API complexity and amount of APIs that libbpf will have >>>>>>>>> to support to the minimum, and will keep the API itself simple: >>>>>>>>> direct-attach with the minimum amount of input arguments. By not >>>>>>>>> exposing low-level APIs we also table the whole bpf_tc_cls_attach_id >>>>>>>>> design discussion, as we now can keep as much info as needed inside >>>>>>>>> bpf_link_tc (which will embed bpf_link internally as well) to support >>>>>>>>> detachment and possibly some additional querying, if needed. >>>>>>>> >>>>>>>> But then there would be no way for the caller to explicitly select a >>>>>>>> mechanism? I.e., if I write a BPF program using this mechanism targeting >>>>>>>> a 5.12 kernel, I'll get netlink attachment, which can stick around when >>>>>>>> I do bpf_link__disconnect(). But then if the kernel gets upgraded to >>>>>>>> support bpf_link for TC programs I'll suddenly transparently get >>>>>>>> bpf_link and the attachments will go away unless I pin them. This >>>>>>>> seems... less than ideal? >>>>>>> >>>>>>> That's what we are doing with bpf_program__attach_kprobe(), though. >>>>>>> And so far I've only seen people (privately) saying how good it would >>>>>>> be to have bpf_link-based TC APIs, doesn't seem like anyone with a >>>>>>> realistic use case prefers the current APIs. So I suspect it's not >>>>>>> going to be a problem in practice. But at least I'd start there and >>>>>>> see how people are using it and if they need anything else. >>>>>> >>>>>> *sigh* - I really wish you would stop arbitrarily declaring your own use >>>>>> cases "realistic" and mine (implied) "unrealistic". Makes it really hard >>>>>> to have a productive discussion... >>>>> >>>>> Well (sigh?..), this wasn't my intention, sorry you read it this way. >>>>> But we had similar discussions when I was adding bpf_link-based XDP >>>>> attach APIs. And guess what, now I see that samples/bpf/whatever_xdp >>>>> is switched to bpf_link-based XDP, because that makes everything >>>>> simpler and more reliable. What I also know is that in production we >>>>> ran into multiple issues with anything that doesn't auto-detach on >>>>> process exit/crash (unless pinned explicitly, of course). And that >>>>> people that are trying to use TC right now are saying how having >>>>> bpf_link-based TC APIs would make everything *simpler* and *safer*. So >>>>> I don't know... I understand it might be convenient in some cases to >>>>> not care about a lifetime of BPF programs you are attaching, but then >>>>> there are usually explicit and intentional ways to achieve at least >>>>> similar behavior with safety by default. >>>> >>>> [...] >>>> >>>> >>> There are many ways to skin this cat. I'd prioritize bpf_link-based TC >>>> >>> APIs to be added with legacy TC API as a fallback. >>>> >>>> I think the problem here is though that this would need to be deterministic >>>> when upgrading from one kernel version to another where we don't use the >>>> fallback anymore, e.g. in case of Cilium we always want to keep the progs >>>> attached to allow headless updates on the agent, meaning, traffic keeps >>>> flowing through the BPF datapath while in user space, our agent restarts >>>> after upgrade, and atomically replaces the BPF progs once up and running >>>> (we're doing this for the whole range of 4.9 to 5.x kernels that we support). >>>> While we use the 'simple' api that is discussed here internally in Cilium, >>>> this attach behavior would have to be consistent, so transparent fallback >>>> inside libbpf on link vs non-link availability won't work (at least in our >>>> case). >>> >>> What about pinning? It's not exactly the same, but bpf_link could >>> actually pin a BPF program, if using legacy TC, and pin bpf_link, if >>> using bpf_link-based APIs. Of course before switching from iproute2 to >>> libbpf APIs you'd need to design your applications to use pinning >>> instead of relying implicitly on permanently attached BPF program. >> >> All the progs we load from Cilium in a K8s setting w/ Pods, we could have easily >> over 100 loaded at the same time on a node, and we template the per Pod ones, so >> the complexity of managing those pinned lifecycles from the agent and dealing with >> the semantic/fallback differences between kernels feels probably not worth the >> gain. So if there would be a libbpf tc simplified attach API, I'd for the time >> being stick to the existing aka legacy means. > > Sure. Then what do you think about keeping only low-level TC APIs, and > in the future add bpf_program__attach_tc(), which will use > bpf_link-based one. It seems like it's not worth it to pretend we have > bpf_link-based semantics with "legacy" current TC APIs. Similarly how > we have a low-level XDP attach API, and bpf_link-based (only) > bpf_program__attach_xdp(). I think that's okay. I guess question is what do we define as initial scope for the low-level TC API. cls_bpf w/ fixed direct-action mode + fixed eth_p_all, allowing to flexibly specify handle / priority or a block_index feels reasonable.
Daniel Borkmann <daniel@iogearbox.net> writes: > On 4/16/21 12:22 AM, Andrii Nakryiko wrote: >> On Thu, Apr 15, 2021 at 3:10 PM Daniel Borkmann <daniel@iogearbox.net> wrote: >>> On 4/15/21 1:58 AM, Andrii Nakryiko wrote: >>>> On Wed, Apr 14, 2021 at 4:32 PM Daniel Borkmann <daniel@iogearbox.net> wrote: >>>>> On 4/15/21 1:19 AM, Andrii Nakryiko wrote: >>>>>> On Wed, Apr 14, 2021 at 3:51 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >>>>>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >>>>>>>> On Wed, Apr 14, 2021 at 3:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >>>>>>>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >>>>>>>>>> On Tue, Apr 6, 2021 at 3:06 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >>>>>>>>>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >>>>>>>>>>>> On Sat, Apr 3, 2021 at 10:47 AM Alexei Starovoitov >>>>>>>>>>>> <alexei.starovoitov@gmail.com> wrote: >>>>>>>>>>>>> On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi wrote: >>>>>>>>>>>>>> On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote: >>>>>>>>>>>>>>> On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: >>>>>>>>>>>>>>>> [...] >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> All of these things are messy because of tc legacy. bpf tried to follow tc style >>>>>>>>>>>>>>> with cls and act distinction and it didn't quite work. cls with >>>>>>>>>>>>>>> direct-action is the only >>>>>>>>>>>>>>> thing that became mainstream while tc style attach wasn't really addressed. >>>>>>>>>>>>>>> There were several incidents where tc had tens of thousands of progs attached >>>>>>>>>>>>>>> because of this attach/query/index weirdness described above. >>>>>>>>>>>>>>> I think the only way to address this properly is to introduce bpf_link style of >>>>>>>>>>>>>>> attaching to tc. Such bpf_link would support ingress/egress only. >>>>>>>>>>>>>>> direction-action will be implied. There won't be any index and query >>>>>>>>>>>>>>> will be obvious. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Note that we already have bpf_link support working (without support for pinning >>>>>>>>>>>>>> ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle, >>>>>>>>>>>>>> chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link >>>>>>>>>>>>>> and are able to operate on the exact filter during release. >>>>>>>>>>>>> >>>>>>>>>>>>> Except they're not unique. The library can stash them, but something else >>>>>>>>>>>>> doing detach via iproute2 or their own netlink calls will detach the prog. >>>>>>>>>>>>> This other app can attach to the same spot a different prog and now >>>>>>>>>>>>> bpf_link__destroy will be detaching somebody else prog. >>>>>>>>>>>>> >>>>>>>>>>>>>>> So I would like to propose to take this patch set a step further from >>>>>>>>>>>>>>> what Daniel said: >>>>>>>>>>>>>>> int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}): >>>>>>>>>>>>>>> and make this proposed api to return FD. >>>>>>>>>>>>>>> To detach from tc ingress/egress just close(fd). >>>>>>>>>>>>>> >>>>>>>>>>>>>> You mean adding an fd-based TC API to the kernel? >>>>>>>>>>>>> >>>>>>>>>>>>> yes. >>>>>>>>>>>> >>>>>>>>>>>> I'm totally for bpf_link-based TC attachment. >>>>>>>>>>>> >>>>>>>>>>>> But I think *also* having "legacy" netlink-based APIs will allow >>>>>>>>>>>> applications to handle older kernels in a much nicer way without extra >>>>>>>>>>>> dependency on iproute2. We have a similar situation with kprobe, where >>>>>>>>>>>> currently libbpf only supports "modern" fd-based attachment, but users >>>>>>>>>>>> periodically ask questions and struggle to figure out issues on older >>>>>>>>>>>> kernels that don't support new APIs. >>>>>>>>>>> >>>>>>>>>>> +1; I am OK with adding a new bpf_link-based way to attach TC programs, >>>>>>>>>>> but we still need to support the netlink API in libbpf. >>>>>>>>>>> >>>>>>>>>>>> So I think we'd have to support legacy TC APIs, but I agree with >>>>>>>>>>>> Alexei and Daniel that we should keep it to the simplest and most >>>>>>>>>>>> straightforward API of supporting direction-action attachments and >>>>>>>>>>>> setting up qdisc transparently (if I'm getting all the terminology >>>>>>>>>>>> right, after reading Quentin's blog post). That coincidentally should >>>>>>>>>>>> probably match how bpf_link-based TC API will look like, so all that >>>>>>>>>>>> can be abstracted behind a single bpf_link__attach_tc() API as well, >>>>>>>>>>>> right? That's the plan for dealing with kprobe right now, btw. Libbpf >>>>>>>>>>>> will detect the best available API and transparently fall back (maybe >>>>>>>>>>>> with some warning for awareness, due to inherent downsides of legacy >>>>>>>>>>>> APIs: no auto-cleanup being the most prominent one). >>>>>>>>>>> >>>>>>>>>>> Yup, SGTM: Expose both in the low-level API (in bpf.c), and make the >>>>>>>>>>> high-level API auto-detect. That way users can also still use the >>>>>>>>>>> netlink attach function if they don't want the fd-based auto-close >>>>>>>>>>> behaviour of bpf_link. >>>>>>>>>> >>>>>>>>>> So I thought a bit more about this, and it feels like the right move >>>>>>>>>> would be to expose only higher-level TC BPF API behind bpf_link. It >>>>>>>>>> will keep the API complexity and amount of APIs that libbpf will have >>>>>>>>>> to support to the minimum, and will keep the API itself simple: >>>>>>>>>> direct-attach with the minimum amount of input arguments. By not >>>>>>>>>> exposing low-level APIs we also table the whole bpf_tc_cls_attach_id >>>>>>>>>> design discussion, as we now can keep as much info as needed inside >>>>>>>>>> bpf_link_tc (which will embed bpf_link internally as well) to support >>>>>>>>>> detachment and possibly some additional querying, if needed. >>>>>>>>> >>>>>>>>> But then there would be no way for the caller to explicitly select a >>>>>>>>> mechanism? I.e., if I write a BPF program using this mechanism targeting >>>>>>>>> a 5.12 kernel, I'll get netlink attachment, which can stick around when >>>>>>>>> I do bpf_link__disconnect(). But then if the kernel gets upgraded to >>>>>>>>> support bpf_link for TC programs I'll suddenly transparently get >>>>>>>>> bpf_link and the attachments will go away unless I pin them. This >>>>>>>>> seems... less than ideal? >>>>>>>> >>>>>>>> That's what we are doing with bpf_program__attach_kprobe(), though. >>>>>>>> And so far I've only seen people (privately) saying how good it would >>>>>>>> be to have bpf_link-based TC APIs, doesn't seem like anyone with a >>>>>>>> realistic use case prefers the current APIs. So I suspect it's not >>>>>>>> going to be a problem in practice. But at least I'd start there and >>>>>>>> see how people are using it and if they need anything else. >>>>>>> >>>>>>> *sigh* - I really wish you would stop arbitrarily declaring your own use >>>>>>> cases "realistic" and mine (implied) "unrealistic". Makes it really hard >>>>>>> to have a productive discussion... >>>>>> >>>>>> Well (sigh?..), this wasn't my intention, sorry you read it this way. >>>>>> But we had similar discussions when I was adding bpf_link-based XDP >>>>>> attach APIs. And guess what, now I see that samples/bpf/whatever_xdp >>>>>> is switched to bpf_link-based XDP, because that makes everything >>>>>> simpler and more reliable. What I also know is that in production we >>>>>> ran into multiple issues with anything that doesn't auto-detach on >>>>>> process exit/crash (unless pinned explicitly, of course). And that >>>>>> people that are trying to use TC right now are saying how having >>>>>> bpf_link-based TC APIs would make everything *simpler* and *safer*. So >>>>>> I don't know... I understand it might be convenient in some cases to >>>>>> not care about a lifetime of BPF programs you are attaching, but then >>>>>> there are usually explicit and intentional ways to achieve at least >>>>>> similar behavior with safety by default. >>>>> >>>>> [...] >>>>> >>>>> >>> There are many ways to skin this cat. I'd prioritize bpf_link-based TC >>>>> >>> APIs to be added with legacy TC API as a fallback. >>>>> >>>>> I think the problem here is though that this would need to be deterministic >>>>> when upgrading from one kernel version to another where we don't use the >>>>> fallback anymore, e.g. in case of Cilium we always want to keep the progs >>>>> attached to allow headless updates on the agent, meaning, traffic keeps >>>>> flowing through the BPF datapath while in user space, our agent restarts >>>>> after upgrade, and atomically replaces the BPF progs once up and running >>>>> (we're doing this for the whole range of 4.9 to 5.x kernels that we support). >>>>> While we use the 'simple' api that is discussed here internally in Cilium, >>>>> this attach behavior would have to be consistent, so transparent fallback >>>>> inside libbpf on link vs non-link availability won't work (at least in our >>>>> case). >>>> >>>> What about pinning? It's not exactly the same, but bpf_link could >>>> actually pin a BPF program, if using legacy TC, and pin bpf_link, if >>>> using bpf_link-based APIs. Of course before switching from iproute2 to >>>> libbpf APIs you'd need to design your applications to use pinning >>>> instead of relying implicitly on permanently attached BPF program. >>> >>> All the progs we load from Cilium in a K8s setting w/ Pods, we could have easily >>> over 100 loaded at the same time on a node, and we template the per Pod ones, so >>> the complexity of managing those pinned lifecycles from the agent and dealing with >>> the semantic/fallback differences between kernels feels probably not worth the >>> gain. So if there would be a libbpf tc simplified attach API, I'd for the time >>> being stick to the existing aka legacy means. >> >> Sure. Then what do you think about keeping only low-level TC APIs, and >> in the future add bpf_program__attach_tc(), which will use >> bpf_link-based one. It seems like it's not worth it to pretend we have >> bpf_link-based semantics with "legacy" current TC APIs. Similarly how >> we have a low-level XDP attach API, and bpf_link-based (only) >> bpf_program__attach_xdp(). > > I think that's okay. I guess question is what do we define as initial scope for > the low-level TC API. cls_bpf w/ fixed direct-action mode + fixed eth_p_all, > allowing to flexibly specify handle / priority or a block_index feels reasonable. Sounds reasonable to me, with the addition of 'parent' to the things you can specify. So snipping a few bits from Kumar's patch and paring it down a bit, we'd end up with something like this? +struct bpf_tc_cls_opts { + size_t sz; + __u32 chain_index; + __u32 handle; + __u32 priority; + __u32 class_id; +}; +#define bpf_tc_cls_opts__last_field class_id + +/* Acts as a handle for an attached filter */ +struct bpf_tc_cls_attach_id { + __u32 ifindex; + union { + __u32 block_index; + __u32 parent_id; + }; + __u32 protocol; + __u32 chain_index; + __u32 handle; + __u32 priority; +}; + +struct bpf_tc_cls_info { + struct bpf_tc_cls_attach_id id; + __u32 class_id; + __u32 bpf_flags; + __u32 bpf_flags_gen; +}; + +LIBBPF_API int bpf_tc_cls_attach_dev(int fd, __u32 ifindex, __u32 parent_id, + const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_attach_id *id); +LIBBPF_API int bpf_tc_cls_detach_dev(const struct bpf_tc_cls_attach_id *id); +LIBBPF_API int bpf_tc_cls_get_info_dev(int fd, __u32 ifindex, __u32 parent_id, + const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_info *info); What about change and replace? I guess we could do without those, right? -Toke
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index a1a424b9b8ff..63baef6045b1 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -16,6 +16,9 @@ #include <stdbool.h> #include <sys/types.h> // for size_t #include <linux/bpf.h> +#include <linux/pkt_cls.h> +#include <linux/pkt_sched.h> +#include <linux/tc_act/tc_bpf.h> #include "libbpf_common.h" @@ -773,6 +776,121 @@ LIBBPF_API int bpf_linker__add_file(struct bpf_linker *linker, const char *filen LIBBPF_API int bpf_linker__finalize(struct bpf_linker *linker); LIBBPF_API void bpf_linker__free(struct bpf_linker *linker); +/* + * Requirements: + * If choosing hw offload mode (skip_sw = true), ifindex during prog load must be set. + */ + +/* Convenience macros for the clsact attach hooks */ +#define BPF_TC_CLSACT_INGRESS TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_INGRESS) +#define BPF_TC_CLSACT_EGRESS TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_EGRESS) + +struct bpf_tc_cls_opts { + size_t sz; + __u32 chain_index; + __u32 handle; + __u32 priority; + __u32 class_id; + bool direct_action; + bool skip_sw; + bool skip_hw; + size_t :0; +}; + +#define bpf_tc_cls_opts__last_field skip_hw + +/* Acts as a handle for an attached filter */ +struct bpf_tc_cls_attach_id { + __u32 ifindex; + union { + __u32 block_index; + __u32 parent_id; + }; + __u32 protocol; + __u32 chain_index; + __u32 handle; + __u32 priority; +}; + +struct bpf_tc_cls_info { + struct bpf_tc_cls_attach_id id; + __u32 class_id; + __u32 bpf_flags; + __u32 bpf_flags_gen; +}; + +/* id is out parameter that will be written to, it must not be NULL */ +LIBBPF_API int bpf_tc_cls_attach_dev(int fd, __u32 ifindex, __u32 parent_id, + __u32 protocol, + const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_attach_id *id); +LIBBPF_API int bpf_tc_cls_change_dev(int fd, __u32 ifindex, __u32 parent_id, + __u32 protocol, + const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_attach_id *id); +/* This replaces an existing filter with the same attributes, so the arguments + * can be filled in from an existing attach_id when replacing, and otherwise be + * used like bpf_tc_cls_attach_dev. + */ +LIBBPF_API int bpf_tc_cls_replace_dev(int fd, __u32 ifindex, __u32 parent_id, + __u32 protocol, + const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_attach_id *id); +LIBBPF_API int bpf_tc_cls_detach_dev(const struct bpf_tc_cls_attach_id *id); +LIBBPF_API int bpf_tc_cls_get_info_dev(int fd, __u32 ifindex, __u32 parent_id, + __u32 protocol, + const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_info *info); + +/* id is out parameter that will be written to, it must not be NULL */ +LIBBPF_API int bpf_tc_cls_attach_block(int fd, __u32 block_index, + __u32 protocol, + const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_attach_id *id); +LIBBPF_API int bpf_tc_cls_change_block(int fd, __u32 block_index, + __u32 protocol, + const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_attach_id *id); +/* This replaces an existing filter with the same attributes, so the arguments + * can be filled in from an existing attach_id when replacing, and otherwise be + * used like bpf_tc_cls_attach_block. + */ +LIBBPF_API int bpf_tc_cls_replace_block(int fd, __u32 block_index, + __u32 protocol, + const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_attach_id *id); +LIBBPF_API int bpf_tc_cls_detach_block(const struct bpf_tc_cls_attach_id *id); +LIBBPF_API int bpf_tc_cls_get_info_block(int fd, __u32 block_index, + __u32 protocol, + const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_info *info); + +struct bpf_tc_act_opts { + size_t sz; + __u32 index; + int action; + void *cookie; + size_t cookie_len; + __u8 hw_stats_type; + bool no_percpu; + size_t :0; +}; + +#define bpf_tc_act_opts__last_field no_percpu + +struct bpf_tc_act_info { + __u32 index; + __u32 capab; + int action; + int refcnt; + int bindcnt; +}; + +LIBBPF_API int bpf_tc_act_attach(int fd, const struct bpf_tc_act_opts *opts, __u32 *index); +LIBBPF_API int bpf_tc_act_replace(int fd, const struct bpf_tc_act_opts *opts, __u32 *index); +LIBBPF_API int bpf_tc_act_detach(__u32 index); +LIBBPF_API int bpf_tc_act_get_info(int fd, struct bpf_tc_act_info *info); + #ifdef __cplusplus } /* extern "C" */ #endif diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index 279ae861f568..72022b45a8b9 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -359,4 +359,18 @@ LIBBPF_0.4.0 { bpf_linker__finalize; bpf_linker__free; bpf_linker__new; + bpf_tc_act_attach; + bpf_tc_act_replace; + bpf_tc_act_detach; + bpf_tc_act_get_info; + bpf_tc_cls_attach_block; + bpf_tc_cls_attach_dev; + bpf_tc_cls_change_block; + bpf_tc_cls_change_dev; + bpf_tc_cls_detach_block; + bpf_tc_cls_detach_dev; + bpf_tc_cls_replace_block; + bpf_tc_cls_replace_dev; + bpf_tc_cls_get_info_dev; + bpf_tc_cls_get_info_block; } LIBBPF_0.3.0; diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c index f448c29de76d..bd196d184341 100644 --- a/tools/lib/bpf/netlink.c +++ b/tools/lib/bpf/netlink.c @@ -4,8 +4,13 @@ #include <stdlib.h> #include <memory.h> #include <unistd.h> +#include <inttypes.h> +#include <arpa/inet.h> #include <linux/bpf.h> +#include <linux/atm.h> +#include <linux/pkt_cls.h> #include <linux/rtnetlink.h> +#include <linux/tc_act/tc_bpf.h> #include <sys/socket.h> #include <errno.h> #include <time.h> @@ -344,6 +349,20 @@ int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags) return ret; } +static int bpf_nl_get_ext(struct nlmsghdr *nh, int sock, unsigned int nl_pid, + __dump_nlmsg_t dump_link_nlmsg_p, + libbpf_dump_nlmsg_t dump_link_nlmsg, void *cookie) +{ + int seq = time(NULL); + + nh->nlmsg_seq = seq; + if (send(sock, nh, nh->nlmsg_len, 0) < 0) + return -errno; + + return bpf_netlink_recv(sock, nl_pid, seq, dump_link_nlmsg_p, + dump_link_nlmsg, cookie); +} + int libbpf_nl_get_link(int sock, unsigned int nl_pid, libbpf_dump_nlmsg_t dump_link_nlmsg, void *cookie) { @@ -356,12 +375,696 @@ int libbpf_nl_get_link(int sock, unsigned int nl_pid, .nlh.nlmsg_flags = NLM_F_DUMP | NLM_F_REQUEST, .ifm.ifi_family = AF_PACKET, }; - int seq = time(NULL); - req.nlh.nlmsg_seq = seq; - if (send(sock, &req, req.nlh.nlmsg_len, 0) < 0) - return -errno; + return bpf_nl_get_ext(&req.nlh, sock, nl_pid, __dump_link_nlmsg, + dump_link_nlmsg, cookie); +} - return bpf_netlink_recv(sock, nl_pid, seq, __dump_link_nlmsg, - dump_link_nlmsg, cookie); +static int tc_bpf_add_fd_and_name(struct nlmsghdr *nh, size_t maxsz, int fd, + enum bpf_prog_type type) +{ + int len, ret, bpf_fd_type, bpf_name_type; + struct bpf_prog_info info = {}; + __u32 info_len = sizeof(info); + char name[64] = {}; + + switch (type) { + case BPF_PROG_TYPE_SCHED_CLS: + bpf_fd_type = TCA_BPF_FD; + bpf_name_type = TCA_BPF_NAME; + break; + case BPF_PROG_TYPE_SCHED_ACT: + bpf_fd_type = TCA_ACT_BPF_FD; + bpf_name_type = TCA_ACT_BPF_NAME; + break; + default: + return -EINVAL; + } + + ret = bpf_obj_get_info_by_fd(fd, &info, &info_len); + if (ret < 0 || type != info.type) + return ret; + + ret = add_nlattr(nh, maxsz, bpf_fd_type, &fd, sizeof(fd)); + if (ret < 0) + return ret; + + len = snprintf(name, sizeof(name), "%s:[%" PRIu32 "]", info.name, + info.id); + if (len < 0 || len >= sizeof(name)) + return len < 0 ? -EINVAL : -ENAMETOOLONG; + + return add_nlattr(nh, maxsz, bpf_name_type, name, len + 1); +} + +struct pass_info { + void *info; + __u32 prog_id; +}; + +static int cls_get_info(struct nlmsghdr *nh, libbpf_dump_nlmsg_t fn, + void *cookie); + +static int tc_cls_bpf_modify(int fd, int cmd, unsigned int flags, __u32 ifindex, + __u32 parent_id, __u32 protocol, + const struct bpf_tc_cls_opts *opts, + __dump_nlmsg_t fn, struct bpf_tc_cls_attach_id *id) +{ + unsigned int bpf_flags = 0, bpf_flags_gen = 0; + struct bpf_tc_cls_info info = {}; + int sock, seq = 0, ret; + struct nlattr *nla; + __u32 nl_pid = 0; + struct { + struct nlmsghdr nh; + struct tcmsg t; + char buf[256]; + } req; + + if (OPTS_GET(opts, priority, 0) > 0xFFFF) + return -EINVAL; + + sock = libbpf_netlink_open(&nl_pid); + if (sock < 0) + return sock; + + memset(&req, 0, sizeof(req)); + req.nh.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcmsg)); + req.nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK | flags; + req.nh.nlmsg_type = cmd; + req.nh.nlmsg_pid = 0; + req.nh.nlmsg_seq = ++seq; + req.t.tcm_family = AF_UNSPEC; + req.t.tcm_handle = OPTS_GET(opts, handle, 0); + req.t.tcm_parent = parent_id; + req.t.tcm_ifindex = ifindex; + req.t.tcm_info = + TC_H_MAKE(OPTS_GET(opts, priority, 0UL) << 16, htons(protocol)); + + if (OPTS_HAS(opts, chain_index)) { + ret = add_nlattr(&req.nh, sizeof(req), TCA_CHAIN, + &opts->chain_index, sizeof(opts->chain_index)); + if (ret < 0) + goto end; + } + + ret = add_nlattr(&req.nh, sizeof(req), TCA_KIND, "bpf", sizeof("bpf")); + if (ret < 0) + goto end; + + nla = begin_nlattr_nested(&req.nh, sizeof(req), TCA_OPTIONS); + if (!nla) { + ret = -EMSGSIZE; + goto end; + } + + if (OPTS_GET(opts, class_id, TC_H_UNSPEC)) { + ret = add_nlattr(&req.nh, sizeof(req), TCA_BPF_CLASSID, + &opts->class_id, sizeof(opts->class_id)); + if (ret < 0) + goto end; + } + + if (cmd != RTM_DELTFILTER) { + ret = tc_bpf_add_fd_and_name(&req.nh, sizeof(req), fd, + BPF_PROG_TYPE_SCHED_CLS); + if (ret < 0) + goto end; + + if (OPTS_GET(opts, skip_hw, false)) + bpf_flags_gen |= TCA_CLS_FLAGS_SKIP_HW; + if (OPTS_GET(opts, skip_sw, false)) + bpf_flags_gen |= TCA_CLS_FLAGS_SKIP_SW; + if (OPTS_GET(opts, direct_action, false)) + bpf_flags |= TCA_BPF_FLAG_ACT_DIRECT; + + if (bpf_flags_gen) { + ret = add_nlattr(&req.nh, sizeof(req), + TCA_BPF_FLAGS_GEN, &bpf_flags_gen, + sizeof(bpf_flags_gen)); + if (ret < 0) + goto end; + } + + if (bpf_flags) { + ret = add_nlattr(&req.nh, sizeof(req), TCA_BPF_FLAGS, + &bpf_flags, sizeof(bpf_flags)); + if (ret < 0) + goto end; + } + } + + end_nlattr_nested(&req.nh, nla); + + ret = send(sock, &req.nh, req.nh.nlmsg_len, 0); + if (ret < 0) + goto end; + + ret = bpf_netlink_recv(sock, nl_pid, seq, fn, NULL, + &(struct pass_info){ &info, 0 }); + + if (fn) + *id = info.id; + +end: + close(sock); + return ret; +} + +int bpf_tc_cls_attach_dev(int fd, __u32 ifindex, __u32 parent_id, + __u32 protocol, const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_attach_id *id) +{ + if (fd < 1 || !OPTS_VALID(opts, bpf_tc_cls_opts) || !id) + return -EINVAL; + + return tc_cls_bpf_modify(fd, RTM_NEWTFILTER, + NLM_F_ECHO | NLM_F_EXCL | NLM_F_CREATE, + ifindex, parent_id, protocol, opts, + cls_get_info, id); +} + +int bpf_tc_cls_change_dev(int fd, __u32 ifindex, __u32 parent_id, + __u32 protocol, const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_attach_id *id) +{ + if (fd < 1 || !OPTS_VALID(opts, bpf_tc_cls_opts) || !id) + return -EINVAL; + + return tc_cls_bpf_modify(fd, RTM_NEWTFILTER, NLM_F_ECHO, ifindex, + parent_id, protocol, opts, cls_get_info, id); +} + +int bpf_tc_cls_replace_dev(int fd, __u32 ifindex, __u32 parent_id, + __u32 protocol, const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_attach_id *id) +{ + if (fd < 1 || !OPTS_VALID(opts, bpf_tc_cls_opts) || !id) + return -EINVAL; + + return tc_cls_bpf_modify(fd, RTM_NEWTFILTER, NLM_F_ECHO | NLM_F_CREATE, + ifindex, parent_id, protocol, opts, + cls_get_info, id); +} + +int bpf_tc_cls_detach_dev(const struct bpf_tc_cls_attach_id *id) +{ + DECLARE_LIBBPF_OPTS(bpf_tc_cls_opts, opts, 0); + + if (!id) + return -EINVAL; + + opts.chain_index = id->chain_index; + opts.handle = id->handle; + opts.priority = id->priority; + + return tc_cls_bpf_modify(-1, RTM_DELTFILTER, 0, id->ifindex, + id->parent_id, id->protocol, &opts, NULL, + NULL); +} + +int bpf_tc_cls_attach_block(int fd, __u32 block_index, __u32 protocol, + const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_attach_id *id) +{ + return bpf_tc_cls_attach_dev(fd, TCM_IFINDEX_MAGIC_BLOCK, block_index, + protocol, opts, id); +} + +int bpf_tc_cls_change_block(int fd, __u32 block_index, __u32 protocol, + const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_attach_id *id) +{ + return bpf_tc_cls_attach_dev(fd, TCM_IFINDEX_MAGIC_BLOCK, block_index, + protocol, opts, id); +} + +int bpf_tc_cls_replace_block(int fd, __u32 block_index, __u32 protocol, + const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_attach_id *id) +{ + return bpf_tc_cls_attach_dev(fd, TCM_IFINDEX_MAGIC_BLOCK, block_index, + protocol, opts, id); +} + +int bpf_tc_cls_detach_block(const struct bpf_tc_cls_attach_id *id) +{ + return bpf_tc_cls_detach_dev(id); +} + +static int __cls_get_info(void *cookie, void *msg, struct nlattr **tb) +{ + struct nlattr *tbb[TCA_BPF_MAX + 1]; + struct pass_info *cinfo = cookie; + struct bpf_tc_cls_info *info; + struct tcmsg *t = msg; + __u32 prog_id; + + info = cinfo->info; + + if (!tb[TCA_OPTIONS]) + return 0; + + libbpf_nla_parse_nested(tbb, TCA_BPF_MAX, tb[TCA_OPTIONS], NULL); + if (!tbb[TCA_BPF_ID]) + return 0; + + prog_id = libbpf_nla_getattr_u32(tbb[TCA_BPF_ID]); + if (cinfo->prog_id && cinfo->prog_id != prog_id) + return 0; + + info->id.parent_id = t->tcm_parent; + info->id.ifindex = t->tcm_ifindex; + info->id.protocol = ntohs(TC_H_MIN(t->tcm_info)); + info->id.priority = TC_H_MAJ(t->tcm_info) >> 16; + info->id.handle = t->tcm_handle; + + if (tb[TCA_CHAIN]) + info->id.chain_index = libbpf_nla_getattr_u32(tb[TCA_CHAIN]); + else + info->id.chain_index = 0; + + if (tbb[TCA_BPF_FLAGS]) + info->bpf_flags = libbpf_nla_getattr_u32(tbb[TCA_BPF_FLAGS]); + + if (tbb[TCA_BPF_FLAGS_GEN]) + info->bpf_flags_gen = + libbpf_nla_getattr_u32(tbb[TCA_BPF_FLAGS_GEN]); + + if (tbb[TCA_BPF_CLASSID]) + info->class_id = libbpf_nla_getattr_u32(tbb[TCA_BPF_CLASSID]); + + return 1; +} + +static int cls_get_info(struct nlmsghdr *nh, libbpf_dump_nlmsg_t fn, + void *cookie) +{ + struct tcmsg *t = NLMSG_DATA(nh); + struct nlattr *tb[TCA_MAX + 1]; + + libbpf_nla_parse(tb, TCA_MAX, + (struct nlattr *)((char *)t + NLMSG_ALIGN(sizeof(*t))), + NLMSG_PAYLOAD(nh, sizeof(*t)), NULL); + if (!tb[TCA_KIND]) + return -EINVAL; + + return __cls_get_info(cookie, t, tb); +} + +static int tc_cls_get_info(int fd, __u32 ifindex, __u32 parent_id, + __u32 protocol, const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_info *info) +{ + __u32 nl_pid, info_len = sizeof(struct bpf_prog_info); + struct bpf_prog_info prog_info = {}; + int sock, ret; + struct { + struct nlmsghdr nh; + struct tcmsg t; + char buf[256]; + } req = { + .nh.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcmsg)), + .nh.nlmsg_type = RTM_GETTFILTER, + .nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_DUMP, + .t.tcm_family = AF_UNSPEC, + }; + + if (!OPTS_VALID(opts, bpf_tc_cls_opts)) + return -EINVAL; + + req.t.tcm_parent = parent_id; + req.t.tcm_ifindex = ifindex; + req.t.tcm_handle = OPTS_GET(opts, handle, 0); + req.t.tcm_info = + TC_H_MAKE(OPTS_GET(opts, priority, 0UL) << 16, htons(protocol)); + + ret = bpf_obj_get_info_by_fd(fd, &prog_info, &info_len); + if (ret < 0) + return ret; + + sock = libbpf_netlink_open(&nl_pid); + if (sock < 0) + return sock; + + ret = add_nlattr(&req.nh, sizeof(req), TCA_KIND, "bpf", sizeof("bpf")); + if (ret < 0) + goto end; + + if (OPTS_HAS(opts, chain_index)) { + ret = add_nlattr(&req.nh, sizeof(req), TCA_CHAIN, + &opts->chain_index, sizeof(opts->chain_index)); + if (ret < 0) + goto end; + } + + req.nh.nlmsg_seq = time(NULL); + + ret = bpf_nl_get_ext(&req.nh, sock, nl_pid, cls_get_info, NULL, + &(struct pass_info){ info, prog_info.id }); + if (ret < 0) + goto end; + /* 1 denotes a match */ + ret = ret == 1 ? 0 : -ESRCH; +end: + close(sock); + return ret; +} + +int bpf_tc_cls_get_info_dev(int fd, __u32 ifindex, __u32 parent_id, + __u32 protocol, const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_info *info) +{ + return tc_cls_get_info(fd, ifindex, parent_id, protocol, opts, info); +} + +int bpf_tc_cls_get_info_block(int fd, __u32 block_index, __u32 protocol, + const struct bpf_tc_cls_opts *opts, + struct bpf_tc_cls_info *info) +{ + return bpf_tc_cls_get_info_dev(fd, TCM_IFINDEX_MAGIC_BLOCK, block_index, + protocol, opts, info); +} + +static int tc_act_add_action(struct nlmsghdr *nh, size_t maxsz, int type, + int fd, const struct bpf_tc_act_opts *opts) +{ + struct nlattr *nla, *nla_opt, *nla_subopt; + struct tc_act_bpf param = {}; + int ret; + + nla = begin_nlattr_nested(nh, maxsz, type); + if (!nla) + return -EMSGSIZE; + + nla_opt = begin_nlattr_nested(nh, maxsz, 1); + if (!nla_opt) + return -EMSGSIZE; + + ret = add_nlattr(nh, maxsz, TCA_ACT_KIND, "bpf", sizeof("bpf")); + if (ret < 0) + return ret; + + ret = add_nlattr(nh, maxsz, TCA_ACT_INDEX, + OPTS_HAS(opts, index) ? &opts->index : &(__u32){ 0 }, + sizeof(opts->index)); + + if (ret < 0) + return ret; + + nla_subopt = begin_nlattr_nested(nh, maxsz, TCA_ACT_OPTIONS); + if (!nla) + return -EMSGSIZE; + + if (fd > 0) { + ret = tc_bpf_add_fd_and_name(nh, maxsz, fd, + BPF_PROG_TYPE_SCHED_ACT); + if (ret < 0) + return ret; + } + + param.index = OPTS_GET(opts, index, 0); + param.action = OPTS_GET(opts, action, TC_ACT_UNSPEC); + + ret = add_nlattr(nh, maxsz, TCA_ACT_BPF_PARMS, ¶m, sizeof(param)); + if (ret < 0) + return ret; + + if (OPTS_GET(opts, cookie, NULL) && OPTS_GET(opts, cookie_len, 0)) { + if (opts->cookie_len > TC_COOKIE_MAX_SIZE) + return -E2BIG; + + ret = add_nlattr(nh, maxsz, TCA_ACT_COOKIE, opts->cookie, + opts->cookie_len); + if (ret < 0) + return ret; + } + + if (OPTS_GET(opts, hw_stats_type, 0)) { + struct nla_bitfield32 hw_stats_bf = { + .value = opts->hw_stats_type, + .selector = opts->hw_stats_type, + }; + + ret = add_nlattr(nh, maxsz, TCA_ACT_HW_STATS, &hw_stats_bf, + sizeof(hw_stats_bf)); + if (ret < 0) + return ret; + } + + if (OPTS_GET(opts, no_percpu, false)) { + struct nla_bitfield32 flags = { + TCA_ACT_FLAGS_NO_PERCPU_STATS, + TCA_ACT_FLAGS_NO_PERCPU_STATS, + }; + + ret = add_nlattr(nh, maxsz, TCA_ACT_FLAGS, &flags, + sizeof(flags)); + if (ret < 0) + return ret; + } + + end_nlattr_nested(nh, nla_subopt); + end_nlattr_nested(nh, nla_opt); + end_nlattr_nested(nh, nla); + + return 0; +} + +static int tc_act_modify(int cmd, unsigned int flags, int fd, int action, + const struct bpf_tc_act_opts *opts, __dump_nlmsg_t fn, + __u32 *index) +{ + struct bpf_tc_act_info info = {}; + int sock, seq = 0, ret; + __u32 nl_pid = 0; + struct { + struct nlmsghdr nh; + struct tcamsg t; + char buf[256]; + } req; + + sock = libbpf_netlink_open(&nl_pid); + if (sock < 0) + return sock; + + memset(&req, 0, sizeof(req)); + req.nh.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcamsg)); + req.nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK | flags; + req.nh.nlmsg_type = cmd; + req.nh.nlmsg_pid = 0; + req.nh.nlmsg_seq = ++seq; + req.t.tca_family = AF_UNSPEC; + + /* gcc complains when using req.nh here */ + ret = tc_act_add_action((struct nlmsghdr *)&req, sizeof(req), + TCA_ACT_TAB, fd, opts); + if (ret < 0) + goto end; + + ret = send(sock, &req.nh, req.nh.nlmsg_len, 0); + if (ret < 0) + goto end; + + ret = bpf_netlink_recv(sock, nl_pid, seq, fn, NULL, + &(struct pass_info){ &info, 0 }); + if (ret < 0) + goto end; + + if (fn) { + if (info.index) { + *index = info.index; + ret = 0; + } else + ret = -ESRCH; + } + +end: + close(sock); + return ret; +} + +static int get_act_info(struct nlmsghdr *nh, libbpf_dump_nlmsg_t fn, + void *cookie); + +int bpf_tc_act_attach(int fd, const struct bpf_tc_act_opts *opts, __u32 *index) +{ + if (fd < 1 || !OPTS_VALID(opts, bpf_tc_act_opts) || !index) + return -EINVAL; + + return tc_act_modify(RTM_NEWACTION, NLM_F_ECHO | NLM_F_EXCL, fd, + OPTS_GET(opts, action, TCA_ACT_UNSPEC), opts, + get_act_info, index); +} + +int bpf_tc_act_replace(int fd, const struct bpf_tc_act_opts *opts, __u32 *index) +{ + if (fd < 1 || !OPTS_VALID(opts, bpf_tc_act_opts) || !index) + return -EINVAL; + + return tc_act_modify(RTM_NEWACTION, NLM_F_ECHO | NLM_F_REPLACE, fd, + OPTS_GET(opts, action, TCA_ACT_UNSPEC), opts, + get_act_info, index); +} + +int bpf_tc_act_detach(__u32 index) +{ + DECLARE_LIBBPF_OPTS(bpf_tc_act_opts, opts, .index = index); + + return tc_act_modify(RTM_DELACTION, index ? 0 : NLM_F_ROOT, -1, + TC_ACT_UNSPEC, &opts, NULL, NULL); +} + +static int __get_act_info(void *cookie, void *msg, struct nlattr *nla) +{ + struct nlattr *tbb[TCA_ACT_BPF_MAX + 1]; + struct pass_info *ainfo = cookie; + struct bpf_tc_act_info *info; + struct tc_act_bpf parm; + __u32 prog_id; + + info = ainfo->info; + + if (!nla) + return -EINVAL; + + libbpf_nla_parse_nested(tbb, TCA_ACT_BPF_MAX, nla, NULL); + + if (!tbb[TCA_ACT_BPF_PARMS] || !tbb[TCA_ACT_BPF_ID]) + return -ESRCH; + + prog_id = libbpf_nla_getattr_u32(tbb[TCA_ACT_BPF_ID]); + if (ainfo->prog_id && ainfo->prog_id != prog_id) + return 0; + + /* Found a match */ + memcpy(&parm, libbpf_nla_data(tbb[TCA_ACT_BPF_PARMS]), + sizeof(parm)); + + info->index = parm.index; + info->capab = parm.capab; + info->action = parm.action; + info->refcnt = parm.refcnt; + info->bindcnt = parm.bindcnt; + + return 1; +} + +static int get_act_info_msg(struct nlmsghdr *nh, libbpf_dump_nlmsg_t fn, + void *cookie, __u32 total, struct nlattr *nla) +{ + struct nlattr *tbb[TCA_ACT_MAX + 1]; + struct tcamsg *t = NLMSG_DATA(nh); + struct nlattr *tb[total + 1]; + int ret; + + libbpf_nla_parse_nested(tb, total, nla, NULL); + + for (int i = 0; i <= total; i++) { + if (tb[i]) { + nla = tb[i]; + libbpf_nla_parse_nested(tbb, TCA_ACT_MAX, nla, NULL); + + if (!tbb[TCA_ACT_KIND]) + return -EINVAL; + + ret = __get_act_info(cookie, t, tbb[TCA_ACT_OPTIONS]); + if (ret < 0) + return ret; + + if (ret > 0) + return 1; + } + } + + return 0; +} + +static int get_act_info(struct nlmsghdr *nh, libbpf_dump_nlmsg_t fn, + void *cookie) +{ + struct nlattr *nla, *tb[TCA_ROOT_MAX + 1]; + __u32 total = 0; + + nla = NLMSG_DATA(nh) + NLMSG_ALIGN(sizeof(struct tcamsg)); + libbpf_nla_parse(tb, TCA_ROOT_MAX, nla, + NLMSG_PAYLOAD(nh, sizeof(struct tcamsg)), NULL); + + if (tb[TCA_ROOT_COUNT]) + total = libbpf_nla_getattr_u32(tb[TCA_ROOT_COUNT]); + + total = total ?: TCA_ACT_MAX_PRIO; + + return get_act_info_msg(nh, fn, cookie, total, tb[TCA_ACT_TAB]); +} + +static int tc_act_get_info(int sock, unsigned int nl_pid, int fd, + struct bpf_tc_act_info *info) +{ + struct bpf_prog_info prog_info = {}; + __u32 info_len = sizeof(prog_info); + struct nlattr *nla, *nla_opt; + struct { + struct nlmsghdr nh; + struct tcamsg t; + char buf[256]; + } req = { + .nh.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcamsg)), + .nh.nlmsg_type = RTM_GETACTION, + .nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_DUMP, + .t.tca_family = AF_UNSPEC, + }; + int ret; + + if (fd < 1) + return -EINVAL; + + ret = bpf_obj_get_info_by_fd(fd, &prog_info, &info_len); + if (ret < 0) + return ret; + + nla = begin_nlattr_nested(&req.nh, sizeof(req), TCA_ACT_TAB); + if (!nla) + return -EMSGSIZE; + + nla_opt = begin_nlattr_nested(&req.nh, sizeof(req), 1); + if (!nla_opt) + return -EMSGSIZE; + + ret = add_nlattr(&req.nh, sizeof(req), TCA_ACT_KIND, "bpf", + sizeof("bpf")); + if (ret < 0) + return ret; + + end_nlattr_nested(&req.nh, nla_opt); + end_nlattr_nested(&req.nh, nla); + + req.nh.nlmsg_seq = time(NULL); + + /* Pass prog id the info is to be returned for */ + return bpf_nl_get_ext(&req.nh, sock, nl_pid, get_act_info, NULL, + &(struct pass_info){ info, prog_info.id }); +} + +int bpf_tc_act_get_info(int fd, struct bpf_tc_act_info *info) +{ + int sock, ret; + __u32 nl_pid; + + if (fd < 1 || !info) + return -EINVAL; + + sock = libbpf_netlink_open(&nl_pid); + if (sock < 0) + return sock; + + ret = tc_act_get_info(sock, nl_pid, fd, info); + if (ret < 0) + goto end; + + if (!info->index) + ret = -ESRCH; +end: + close(sock); + return ret; }
This adds functions that wrap the netlink API used for adding, manipulating, and removing filters and actions. These functions operate directly on the loaded prog's fd, and return a handle to the filter and action using an out parameter (id for tc_cls, and index for tc_act). The basic featureset is covered to allow for attaching, manipulation of properties, and removal of filters and actions. Some additional features like TCA_BPF_POLICE and TCA_RATE for tc_cls have been omitted. These can added on top later by extending the bpf_tc_cls_opts struct. Support for binding actions directly to a classifier by passing them in during filter creation has also been omitted for now. These actions have an auto clean up property because their lifetime is bound to the filter they are attached to. This can be added later, but was omitted for now as direct action mode is a better alternative to it. An API summary: The BPF TC-CLS API bpf_tc_act_{attach, change, replace}_{dev, block} may be used to attach, change, and replace SCHED_CLS bpf classifiers. Separate set of functions are provided for network interfaces and shared filter blocks. bpf_tc_cls_detach_{dev, block} may be used to detach existing SCHED_CLS filter. The bpf_tc_cls_attach_id object filled in during attach, change, or replace must be passed in to the detach functions for them to remove the filter and its attached classififer correctly. bpf_tc_cls_get_info is a helper that can be used to obtain attributes for the filter and classififer. The opts structure may be used to choose the granularity of search, such that info for a specific filter corresponding to the same loaded bpf program can be obtained. By default, the first match is returned to the user. Examples: struct bpf_tc_cls_attach_id id = {}; struct bpf_object *obj; struct bpf_program *p; int fd, r; obj = bpf_object_open("foo.o"); if (IS_ERR_OR_NULL(obj)) return PTR_ERR(obj); p = bpf_object__find_program_by_title(obj, "classifier"); if (IS_ERR_OR_NULL(p)) return PTR_ERR(p); if (bpf_object__load(obj) < 0) return -1; fd = bpf_program__fd(p); r = bpf_tc_cls_attach_dev(fd, if_nametoindex("lo"), BPF_TC_CLSACT_INGRESS, ETH_P_IP, NULL, &id); if (r < 0) return r; ... which is roughly equivalent to (after clsact qdisc setup): # tc filter add dev lo ingress bpf obj /home/kkd/foo.o sec classifier If a user wishes to modify existing options on an attached filter, the bpf_tc_cls_change_{dev, block} API may be used. Parameters like chain_index, priority, and handle are ignored in the bpf_tc_cls_opts struct as they cannot be modified after attaching a filter. Example: /* Optional parameters necessary to select the right filter */ DECLARE_LIBBPF_OPTS(bpf_tc_cls_opts, opts, .handle = id.handle, .priority = id.priority, .chain_index = id.chain_index) /* Turn on direct action mode */ opts.direct_action = true; r = bpf_tc_cls_change_dev(fd, id.ifindex, id.parent_id, id.protocol, &opts, &id); if (r < 0) return r; /* Verify that the direct action mode has been set */ struct bpf_tc_cls_info info = {}; r = bpf_tc_cls_get_info_dev(fd, id.ifindex, id.parent_id, id.protocol, &opts, &info); if (r < 0) return r; assert(info.bpf_flags & TCA_BPF_FLAG_ACT_DIRECT); This would be roughly equivalent to doing: # tc filter change dev lo egress prio <p> handle <h> bpf obj /home/kkd/foo.o section classifier da ... except a new bpf program will be loaded and replace existing one. If a user wishes to either replace an existing filter, or create a new one with the same properties, they can use bpf_tc_cls_replace_dev. The benefit of bpf_tc_cls_change is that it fails if no matching filter exists. The BPF TC-ACT API bpf_tc_act_{attach, replace} may be used to attach and replace already attached SCHED_ACT actions. Passing an index of 0 has special meaning, in that an index will be automatically chosen by the kernel. The index chosen by the kernel is the return value of these functions in case of success. bpf_tc_act_detach may be used to detach a SCHED_ACT action prog identified by the index parameter. The index 0 again has a special meaning, in that passing it will flush all existing SCHED_ACT actions loaded using the ACT API. bpf_tc_act_get_info is a helper to get the required attributes of a loaded program to be able to manipulate it futher, by passing them into the aforementioned functions. Example: struct bpf_object *obj; struct bpf_program *p; __u32 index; int fd, r; obj = bpf_object_open("foo.o"); if (IS_ERR_OR_NULL(obj)) return PTR_ERR(obj); p = bpf_object__find_program_by_title(obj, "action"); if (IS_ERR_OR_NULL(p)) return PTR_ERR(p); if (bpf_object__load(obj) < 0) return -1; fd = bpf_program__fd(p); r = bpf_tc_act_attach(fd, NULL, &index); if (r < 0) return r; if (bpf_tc_act_detach(index)) return -1; ... which is equivalent to the following sequence: tc action add action bpf obj /home/kkd/foo.o sec action tc action del action bpf index <idx> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- tools/lib/bpf/libbpf.h | 118 +++++++ tools/lib/bpf/libbpf.map | 14 + tools/lib/bpf/netlink.c | 715 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 841 insertions(+), 6 deletions(-)