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] [day] [month] [year] [list]

Message-ID: <CAMy7=ZULTCoSCcjxw=MdhaKmNM9DXKc=t7QScf9smKKUB+L_fQ@mail.gmail.com>
Date:   Mon, 14 Jun 2021 14:08:51 +0300
From:   Yaniv Agman <yanivagman@...il.com>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc:     andrii@...nel.org, ast@...nel.org, bpf <bpf@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        toke@...hat.com
Subject: Re: [PATCH bpf-next 0/3] Fixes for TC-BPF series

‫בתאריך יום ב׳, 14 ביוני 2021 ב-13:20 מאת ‪Kumar Kartikeya Dwivedi‬‏
<‪memxor@...il.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