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

[sdf@google.com]

On 05/27, Jakub Sitnicki wrote:
> On Wed, May 27, 2020 at 07:40 PM CEST, sdf@google.com wrote:
> > On 05/27, Jakub Sitnicki wrote:
> >> In order to:
> >
> >>   (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
> >
> >> we will need to keep more state per netns than a single pointer like we
> >> have now for BPF flow dissector program.
> >
> >> Prepare for the above by extracting netns_bpf that is part of struct  
> net,
> >> for storing all state related to BPF programs attached to netns.
> >
> >> Turn flow dissector callbacks for querying/attaching/detaching a  
> program
> >> into generic ones that operate on netns_bpf. Next patch will move the
> >> generic callbacks into their own module.
> >
> >> This is similar to how it is organized for cgroup with cgroup_bpf.
> >
> >> 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  
> ++++++++++++++++++++++++-------------
> >>   6 files changed, 143 insertions(+), 63 deletions(-)
> >>   create mode 100644 include/linux/bpf-netns.h
> >>   create mode 100644 include/net/netns/bpf.h
> >
> >> 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 <sdf@google.com>