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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAEf4BzYY8NcmprF-V3SxBgiF0mqNpK-qrymt=wvz6iCON=geiw@mail.gmail.com>
Date:   Mon, 22 Jun 2020 23:23:30 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jakub Sitnicki <jakub@...udflare.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 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.

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! :)

>
> No functional changes intended.
>
> Signed-off-by: Jakub Sitnicki <jakub@...udflare.com>
> ---

LGTM.

Acked-by: Andrii Nakryiko <andriin@...com>

>  include/linux/bpf.h        |   3 +
>  include/net/netns/bpf.h    |   5 +-
>  kernel/bpf/core.c          |  19 +++--
>  kernel/bpf/net_namespace.c | 137 +++++++++++++++++++++++++++----------
>  net/core/flow_dissector.c  |  21 +++---
>  5 files changed, 131 insertions(+), 54 deletions(-)
>

[...]

> +
> +void bpf_prog_array_delete_safe(struct bpf_prog_array *array,
> +                               struct bpf_prog *old_prog)
> +{
> +       bpf_prog_array_replace_item(array, old_prog, &dummy_bpf_prog.prog);

nit: (void) cast to show we don't care about return code?

[...]