netdev - Re: [PATCH bpf-next 2/3] bpf, netns: Keep attached programs in bpf_prog

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: <87tuz2m4wh.fsf@cloudflare.com>
Date:   Tue, 23 Jun 2020 12:51:26 +0200
From:   Jakub Sitnicki <jakub@...udflare.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        kernel-team@...udflare.com
Subject: Re: [PATCH bpf-next 2/3] bpf, netns: Keep attached programs in bpf_prog_array

On Tue, Jun 23, 2020 at 08:23 AM CEST, Andrii Nakryiko wrote:
> On Mon, Jun 22, 2020 at 9:04 AM Jakub Sitnicki <jakub@...udflare.com> wrote:
>>
>> Prepare for having multi-prog attachments for new netns attach types by
>> storing programs to run in a bpf_prog_array, which is well suited for
>> iterating over programs and running them in sequence.
>>
>> Because bpf_prog_array is dynamically resized, after this change a
>> potentially blocking memory allocation in bpf(PROG_QUERY) callback can
>> happen, in order to collect program IDs before copying the values to
>> user-space supplied buffer. This forces us to adapt how we protect access
>> to the attached program in the callback. As bpf_prog_array_copy_to_user()
>> helper can sleep, we switch from an RCU read lock to holding a mutex that
>> serializes updaters.
>>
>> To handle bpf(PROG_ATTACH) scenario when we are replacing an already
>> attached program, we introduce a new bpf_prog_array helper called
>> bpf_prog_array_replace_item that will exchange the old program with a new
>> one. bpf-cgroup does away with such helper by computing an index into the
>> array based on program position in an external list of attached
>> programs/links. Such approach seems fragile, however, when dummy progs can
>> be left in the array after a memory allocation failure on link release.
>
> bpf-cgroup can have the same BPF program present multiple times in the
> effective prog array due to inheritance. It also has strict
> guarantee/requirement about relative order of programs in parent
> cgroup vs child cgroups. For such cases, replacing a BPF program based
> on its pointer is not going to work correctly.

Thanks for the explanation. That did not occur to me. Incorporated it
into the description in v2.

>
> We do need to make sure that cgroup detachment never fails by falling
> back to replacing BPF prog with dummy prog, though. If you are
> interested in a challenge, you are very welcome to do that! :)

I keep a list of tasks for a slow day.

[...]