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]

Date:   Wed, 24 Aug 2016 17:54:51 -0400
From:   Tejun Heo <htejun@...com>
To:     Daniel Mack <daniel@...que.org>
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

Hello, Daniel.

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().

> +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]));
> +
> +	rcu_read_unlock();
> +}
...
> +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.

> +	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.

> +	effective = (!prog && parent) ?
> +		rcu_dereference(parent->bpf.prog_effective[type]) : prog;

If this is what's triggering rcu warnings, there's an accessor to use
in these situations.

> +	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.

Thanks.

-- 
tejun