netdev - Re: [PATCH v2 2/6] cgroup: add support for eBPF programs

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: <616bcf67-314a-c943-8177-7e04021e89bb@zonque.org>
Date:   Thu, 25 Aug 2016 00:30:47 +0200
From:   Daniel Mack <daniel@...que.org>
To:     Tejun Heo <htejun@...com>
Cc:     daniel@...earbox.net, ast@...com, davem@...emloft.net,
        kafai@...com, fw@...len.de, pablo@...filter.org, harald@...hat.com,
        netdev@...r.kernel.org, sargun@...gun.me
Subject: Re: [PATCH v2 2/6] cgroup: add support for eBPF programs

Hi Tejun,

On 08/24/2016 11:54 PM, Tejun Heo wrote:
> On Wed, Aug 24, 2016 at 10:24:19PM +0200, Daniel Mack wrote:
>> +void cgroup_bpf_free(struct cgroup *cgrp)
>> +{
>> +	unsigned int type;
>> +
>> +	rcu_read_lock();
>> +
>> +	for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) {
>> +		if (!cgrp->bpf.prog[type])
>> +			continue;
>> +
>> +		bpf_prog_put(cgrp->bpf.prog[type]);
>> +		static_branch_dec(&cgroup_bpf_enabled_key);
>> +	}
>> +
>> +	rcu_read_unlock();
> 
> These rcu locking seem suspicious to me.  RCU locking on writer side
> is usually bogus.  We sometimes do it to work around locking
> assertions in accessors but it's a better idea to make the assertions
> better in those cases - e.g. sth like assert_mylock_or_rcu_locked().

Right, in this case, it is unnecessary, as the bpf.prog[] is not under RCU.

>> +void cgroup_bpf_inherit(struct cgroup *cgrp, struct cgroup *parent)
>> +{
>> +	unsigned int type;
>> +
>> +	rcu_read_lock();
> 
> Ditto.
> 
>> +	for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++)
>> +		rcu_assign_pointer(cgrp->bpf.prog_effective[type],
>> +			rcu_dereference(parent->bpf.prog_effective[type]));

Okay, yes. We're under cgroup_mutex write-path protection here, so
that's unnecessary too.

>> +void __cgroup_bpf_update(struct cgroup *cgrp,
>> +			 struct cgroup *parent,
>> +			 struct bpf_prog *prog,
>> +			 enum bpf_attach_type type)
>> +{
>> +	struct bpf_prog *old_prog, *effective;
>> +	struct cgroup_subsys_state *pos;
>> +
>> +	rcu_read_lock();
> 
> Ditto.

Yes, agreed, as above.

>> +	old_prog = xchg(cgrp->bpf.prog + type, prog);
>> +	if (old_prog) {
>> +		bpf_prog_put(old_prog);
>> +		static_branch_dec(&cgroup_bpf_enabled_key);
>> +	}
>> +
>> +	if (prog)
>> +		static_branch_inc(&cgroup_bpf_enabled_key);
> 
> Minor but probably better to inc first and then dec so that you can
> avoid unnecessary enabled -> disabled -> enabled sequence.

Good point. Will fix.

>> +	rcu_read_unlock();
>> +
>> +	css_for_each_descendant_pre(pos, &cgrp->self) {
> 
> On the other hand, this walk actually requires rcu read locking unless
> you're holding cgroup_mutex.

I am - this function is always called with cgroup_mutex held through the
wrapper in kernel/cgroup.c.

Thanks a lot - will put all that changes in v3.


Daniel