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

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

[Yaniv Agman]

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);

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?

Yaniv

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

[Kumar Kartikeya Dwivedi]

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

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

[Yaniv Agman]

‫בתאריך יום ב׳, 14 ביוני 2021 ב-13:20 מאת ‪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

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

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

[Kumar Kartikeya Dwivedi]

These are a few simple cleanups. Two of these hopefully silence the coverity
warnings. Even though there is no real bug here, the report is valid as per
language rules, and overall it does make the code a bit simpler. There's one
other patch to add the forgotten NLM_F_EXCL that I spotted while doing this.

Andrii, would you be able to tell whether this silences the warnings? I wasn't
able to figure out how to run the Coverity suite locally.

Kumar Kartikeya Dwivedi (3):
  libbpf: remove unneeded check for flags during detach
  libbpf: set NLM_F_EXCL when creating qdisc
  libbpf: add request buffer type for netlink messages

 tools/lib/bpf/netlink.c | 111 +++++++++++++++-------------------------
 tools/lib/bpf/nlattr.h  |  37 +++++++++-----
 2 files changed, 66 insertions(+), 82 deletions(-)

-- 
2.31.1