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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 14 Jun 2021 15:48:44 +0530
From:   Kumar Kartikeya Dwivedi <memxor@...il.com>
To:     Yaniv Agman <yanivagman@...il.com>
Cc:     andrii@...nel.org, ast@...nel.org, bpf@...r.kernel.org,
        daniel@...earbox.net, netdev@...r.kernel.org, toke@...hat.com
Subject: Re: [PATCH bpf-next 0/3] Fixes for TC-BPF series

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