Remove backing qdisc on cleanup

[pcolladosoto]

Hi! I've been playing around with Go's bindings for libbpf (i.e. aquasecurity/libbpfgo) and found that their example of how to use BPF-based TC filters did not clean the backing qdisc after the cleanup.

Even though this behaviour was not causing any issues, I decided to dive into libbpf's implementation and found that, indeed, a backing qdisc is only removed if the attach point of the hook is BPF_TC_INGRESS|BPF_TC_EGRESS which is usually not the case. In that way, libbpf's API is 'forcing' the user to explicitly request the clearing of the qdisc. This behaviour is documented in the associated patch series.

Anyway, this PR simply forces the removal of the backing qdisc by setting tc_hook.attach_point = BPF_TC_INGRESS|BPF_TC_EGRESS; just before the call to bpf_tc_hook_destroy. Given the example is rather self-contained I suppose that makes sense? The rationale behind opening up the PR is that it took me several hours to get to the patch series and I was hoping the next person wouldn't have to do the same... Maybe this can be left as a comment in tc.c even if one decides not to remove the qdisc?

In a real-world approach I suppose one could use the return value of bpf_tc_hook_create to decide whether to remove the qdisc or not so that the backing qdisc is only removed if the return value is -EEXIST? Judging by the comment above the call to bpf_tc_hook_create I suppose this behaviour was known by the original author of tc.c, but I feel like a clarification would be great for people using these examples as scaffolding for larger projects.

Thanks a ton for all this work! It's really helped me up to this moment and I'm sure it'll continue to do so for quite a while!

[kkdwivedi]

While the change is correct (in terms of correctly removing a qdisc this program created), in general why we don't do that by default is because between the creation and destruction, other processes using this same API could come in and attach their own TC-BPF filters to ingress and egress. So it's inherently racy unless you know you "own" the qdisc somehow.

It's a fundamental limitation of the way the netlink API works in the kernel, which this libbpf functions use underneath. Attempts made to alleviate such problems for the netlink API went nowhere upstream.

You could have another program that sees hook_created as false, and install its filters and go away, and then later since you have hook_created as true, you destroy the qdisc disrupting the filters the other program attached.

I guess all of this does not matter too much in the context of this example but it is something to keep in mind when exposing this as part of a generic library, i.e. not to do this by default, and only when explicitly requested by the user, with a fat warning on top about the above.

The "correct" way would be to use the new tcx API which has bpf_link support (https://lwn.net/Articles/938632) which does not require a clsact qdisc to be installed on the net device.

[pcolladosoto]

Hi @kkdwivedi! Thanks a ton for the clarification. After going through the references on LWN I see how the netlink API is indeed not flexible enough to accommodate this user case in a straightforward way. I'll look into migrating to the new tcx API for my project.

Given my current use case I can safely assume my program 'owns' the qdisc I attach the eBPF program to in the sense that no other processes make use of it (i.e. I'm not working in the context of virtualisation in any way, shape or form).

However, as you correctly point out this might not (and certainly won't) be the case and anybody wanting to remove the qdisc should want it enough to do all this digging. With that in mind, maybe adding a bit of documentation (maybe in the shape of a comment?) would be a compromise? That way there's no qdisc deletion at all but users are pointed both in the right direction for deleting the qdisc and maybe also the new tcx API.

I completely understand that you might just prefer to leave things as they are. In that case feel free to close the PR.

Anyway, thanks a ton for the reference to the patch series: I learned a lot thanks to it!

All the best :)