בתאריך יום ב׳, 14 ביוני 2021 ב-13:20 מאת Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>: > > On Mon, Jun 14, 2021 at 03:02:07PM IST, Yaniv Agman wrote: > > Hi Kartikeya, > > > > I recently started experimenting with the new tc-bpf API (which is > > great, many thanks!) and I wanted to share a potential problem I > > found. > > I'm using this "Fixes for TC-BPF series" thread to write about it, but > > it is not directly related to this patch set. > > > > According to the API summary given in > > https://lore.kernel.org/bpf/20210512103451.989420-3-memxor@xxxxxxxxx/, > > "It is advised that if the qdisc is operated on by many programs, > > then the program at least check that there are no other existing > > filters before deleting the clsact qdisc." > > In the example given, one should: > > > > /* set opts as NULL, as we're not really interested in > > * getting any info for a particular filter, but just > > * detecting its presence. > > */ > > r = bpf_tc_query(&hook, NULL); > > > > Yes, at some revision this worked, but then we changed it to not allow passing > opts as NULL and I forgot to remove the snippet from the commit message. Sorry > for that, but now it's buried in the git history forever :/. Mea Culpa. > > > However, following in this summary, where bpf_tc_query is described, > > it is written that the opts argument cannot be NULL. > > And indeed, when I tried to use the example above, an error (EINVAL) > > was returned (as expected?) > > > > Am I missing something? > > > > You are correct. We could do a few thing things: > > 1. Add a separate documentation file that correctly describes things (everything > minus that para). > 2. Support passing NULL to just detect presence of filters at a hook. > 3. Add a multi query API that dumps all filters. > > Regardless of what we choose here, it will still be racy to clean up the qdisc a > program installs itself, as there is a small race (but a race nonetheless) > between checking of installed filters and removing the qdisc. > > I will discuss this today in the TC meeting to find some proper solution instead > of the current hack. For now it would probably be best to leave it around I > guess, though that does entail a small performance impact (due to enabling the > sch_handle_{ingress,egress} static key). > > -- > Kartikeya Got it, thanks. Another option (that will require an API change) can be to delete the qdisc only if there are no other filters installed on it. I'm not really familiar with the netlink API and if that's even possible, but providing such an API can reduce the chances of having a race condition. For example, what I have in mind is adding a new flag to tc_bpf_flags called something like BPF_TC_Q_DELETE, and also adding an "opts" argument to the bpf_tc_hook_destroy() api so we can pass this flag WDYT? Is that even an option? Yaniv
