Re: [PATCH bpf-next v3 2/4] bpf, netns: Keep attached programs in bpf_prog_array

On Fri, Jun 26, 2020 at 3:13 PM Andrii Nakryiko
[off-list ref] wrote:

On Fri, Jun 26, 2020 at 2:45 AM Jakub Sitnicki [off-list ref] wrote:

quoted

On Thu, Jun 25, 2020 at 10:50 PM CEST, Andrii Nakryiko wrote:

quoted

On Thu, Jun 25, 2020 at 7:17 AM Jakub Sitnicki [off-list ref] wrote:

quoted

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.

After this change bpf(PROG_QUERY) may block to allocate memory in
bpf_prog_array_copy_to_user() for collected program IDs. This forces a
change in how we protect access to the attached program in the query
callback. Because bpf_prog_array_copy_to_user() can sleep, we switch from
an RCU read lock to holding a mutex that serializes updaters.

Because we allow only one BPF flow_dissector program to be attached to
netns at all times, the bpf_prog_array pointed by net->bpf.run_array is
always either detached (null) or one element long.

No functional changes intended.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---

I wonder if instead of NULL prog_array, it's better to just use a
dummy empty (but allocated) array. Might help eliminate some of the
IFs, maybe even in the hot path.

That was my initial approach, which I abandoned seeing that it leads to
replacing NULL prog_array checks in flow_dissector with
bpf_prog_array_is_empty() checks to determine which netns has a BPF
program attached. So no IFs gone there.

While on the hot path, where we run the program, we probably would still
be left with an IF checking for empty prog_array to avoid building the
context if no progs will RUN.

The checks I'm referring to are on attach path, in
flow_dissector_bpf_prog_attach_check(), and hot-path,
__skb_flow_dissect().

Fair enough.

Acked-by: Andrii Nakryiko <redacted>