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

[Jakub Sitnicki <jakub@cloudflare.com>]

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.

-jkbs