Re: [PATCH bpf-next 0/3] Fixes for TC-BPF series

‫בתאריך יום ב׳, 14 ביוני 2021 ב-13:20 מאת ‪Kumar Kartikeya Dwivedi‬‏
<‪memxor@gmail.com‬‏>:‬

On Mon, Jun 14, 2021 at 03:02:07PM IST, Yaniv Agman wrote:

quoted

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@gmail.com/ (local),
"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.

quoted

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