netdev - Re: [PATCH bpf] flow_dissector: Drop BPF flow dissector prog ref on netns cleanup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200521104214.6a1a4f9c@toad>
Date:   Thu, 21 May 2020 10:42:14 +0200
From:   Jakub Sitnicki <jakub@...udflare.com>
To:     sdf@...gle.com
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        kernel-team@...udflare.com, Petar Penkov <ppenkov@...gle.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Subject: Re: [PATCH bpf] flow_dissector: Drop BPF flow dissector prog ref on
 netns cleanup

On Wed, 20 May 2020 10:40:00 -0700
sdf@...gle.com wrote:

> > +static void __net_exit flow_dissector_pernet_pre_exit(struct net *net)
> > +{
> > +	struct bpf_prog *attached;
> > +
> > +	/* We don't lock the update-side because there are no
> > +	 * references left to this netns when we get called. Hence
> > +	 * there can be no attach/detach in progress.
> > +	 */
> > +	rcu_read_lock();
> > +	attached = rcu_dereference(net->flow_dissector_prog);
> > +	if (attached) {
> > +		RCU_INIT_POINTER(net->flow_dissector_prog, NULL);
> > +		bpf_prog_put(attached);
> > +	}
> > +	rcu_read_unlock();
> > +}  
> I wonder, should we instead refactor existing
> skb_flow_dissector_bpf_prog_detach to accept netns (instead of attr)
> can call that here? Instead of reimplementing it (I don't think we
> care about mutex lock/unlock efficiency here?). Thoughts?

I wanted to be nice to container-heavy workloads where network
namespaces get torn down frequently and in parallel and avoid
locking a global mutex. OTOH we already do it today, for instance in
devlink pre_exit callback.

In our case I think there is a way to have the cake and it eat too:

https://lore.kernel.org/bpf/20200521083435.560256-1-jakub@cloudflare.com/

Thanks for reviewing it,
-jkbs