Re: [PATCH bpf-next 3/8] net: Introduce netns_bpf for BPF programs attached to netns

On 05/27, Jakub Sitnicki wrote:

On Wed, May 27, 2020 at 07:40 PM CEST, sdf@google.com wrote:

quoted

On 05/27, Jakub Sitnicki wrote:

quoted

In order to:

quoted

  (1) attach more than one BPF program type to netns, or
  (2) support attaching BPF programs to netns with bpf_link, or
  (3) support multi-prog attach points for netns

quoted

we will need to keep more state per netns than a single pointer like we
have now for BPF flow dissector program.

quoted

Prepare for the above by extracting netns_bpf that is part of struct  

net,

quoted

quoted

for storing all state related to BPF programs attached to netns.

quoted

Turn flow dissector callbacks for querying/attaching/detaching a  

program

quoted

quoted

into generic ones that operate on netns_bpf. Next patch will move the
generic callbacks into their own module.

quoted

This is similar to how it is organized for cgroup with cgroup_bpf.

quoted

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
  include/linux/bpf-netns.h   | 56 ++++++++++++++++++++++
  include/linux/skbuff.h      | 26 ----------
  include/net/net_namespace.h |  4 +-
  include/net/netns/bpf.h     | 17 +++++++
  kernel/bpf/syscall.c        |  7 +--
  net/core/flow_dissector.c   | 96  

++++++++++++++++++++++++-------------

quoted

quoted

  6 files changed, 143 insertions(+), 63 deletions(-)
  create mode 100644 include/linux/bpf-netns.h
  create mode 100644 include/net/netns/bpf.h

quoted

diff --git a/include/linux/bpf-netns.h b/include/linux/bpf-netns.h
new file mode 100644
index 000000000000..f3aec3d79824
--- /dev/null
+++ b/include/linux/bpf-netns.h

@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _BPF_NETNS_H
+#define _BPF_NETNS_H
+
+#include <linux/mutex.h>
+#include <uapi/linux/bpf.h>
+
+enum netns_bpf_attach_type {
+	NETNS_BPF_INVALID = -1,
+	NETNS_BPF_FLOW_DISSECTOR = 0,
+	MAX_NETNS_BPF_ATTACH_TYPE
+};
+
+static inline enum netns_bpf_attach_type
+to_netns_bpf_attach_type(enum bpf_attach_type attach_type)
+{
+	switch (attach_type) {
+	case BPF_FLOW_DISSECTOR:
+		return NETNS_BPF_FLOW_DISSECTOR;
+	default:
+		return NETNS_BPF_INVALID;
+	}
+}
+
+/* Protects updates to netns_bpf */
+extern struct mutex netns_bpf_mutex;

I wonder whether it's a good time to make this mutex per-netns, WDYT?

The only problem I see is that it might complicate the global
mode of flow dissector where we go over every ns to make sure no
progs are attached. That will be racy with per-ns mutex unless
we do something about it...

It crossed my mind. I stuck with a global mutex for a couple of
reasons. Different that one you bring up, which I forgot about.

1. Don't know if it has potential to be a bottleneck.

cgroup BPF uses a global mutex too. Even one that serializes access to
more data than just BPF programs attached to a cgroup.

Also, we grab the netns_bpf_mutex only on prog attach/detach, and link
create/update/release. Netns teardown is not grabbing it. So if you're
not using netns BPF you're not going to "feel" contention.

2. Makes locking on nets bpf_link release trickier

In bpf_netns_link_release (patch 5), we deref pointer from link to
struct net under RCU read lock, in case the net is being destroyed
simulatneously.

However, we're also grabbing the netns_bpf_mutex, in case of another
possible scenario, when struct net is alive and well (refcnt > 0), but
we're racing with a prog attach/detach to access net->bpf.{links,progs}.

Making the mutex part of net->bpf means I first need to somehow ensure
netns stays alive if I go to sleep waiting for the lock. Or it would
have to be a spinlock, or some better (simpler?) locking scheme.

The above two convinced me that I should start with a global mutex, and
go for more pain if there's contention.

Thanks for giving the series a review.

Yeah, everything makes sense, agreed, feel free to slap:
Reviewed-by: Stanislav Fomichev <redacted>