On 05/27, Jakub Sitnicki wrote:
On Wed, May 27, 2020 at 07:40 PM CEST, sdf@xxxxxxxxxx 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@xxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx>
