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

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

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