netdev - Re: [PATCH net-next 1/8] bpf: multi program support for cgroup+bpf

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: <f5d34464-42a9-b51b-edc4-06c44b3c4845@fb.com>
Date:   Mon, 2 Oct 2017 21:21:23 -0700
From:   Alexei Starovoitov <ast@...com>
To:     David Ahern <dsa@...ulusnetworks.com>,
        "David S . Miller" <davem@...emloft.net>
CC:     Daniel Borkmann <daniel@...earbox.net>, Tejun Heo <tj@...nel.org>,
        <netdev@...r.kernel.org>, <kernel-team@...com>
Subject: Re: [PATCH net-next 1/8] bpf: multi program support for cgroup+bpf

On 10/2/17 8:54 PM, David Ahern wrote:
> On 10/2/17 4:48 PM, Alexei Starovoitov wrote:
>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>> index 546113430049..70f679a94804 100644
>> --- a/kernel/bpf/cgroup.c
>> +++ b/kernel/bpf/cgroup.c
>> @@ -27,129 +27,361 @@ void cgroup_bpf_put(struct cgroup *cgrp)
>>  {
>>  	unsigned int type;
>>
>> -	for (type = 0; type < ARRAY_SIZE(cgrp->bpf.prog); type++) {
>> -		struct bpf_prog *prog = cgrp->bpf.prog[type];
>> -
>> -		if (prog) {
>> -			bpf_prog_put(prog);
>> +	for (type = 0; type < ARRAY_SIZE(cgrp->bpf.progs); type++) {
>> +		struct list_head *progs = &cgrp->bpf.progs[type];
>> +		struct bpf_prog_list *pl, *tmp;
>> +
>> +		list_for_each_entry_safe(pl, tmp, progs, node) {
>> +			list_del(&pl->node);
>> +			bpf_prog_put(pl->prog);
>> +			kfree(pl);
>>  			static_branch_dec(&cgroup_bpf_enabled_key);
>>  		}
>> +		bpf_prog_array_free(cgrp->bpf.effective[type]);
>> +	}
>> +}
>> +
>
> ...
>
>>
>> -	if (prog)
>> -		static_branch_inc(&cgroup_bpf_enabled_key);
>> +	/* all allocations were successful. Activate all prog arrays */
>> +	css_for_each_descendant_pre(css, &cgrp->self) {
>> +		struct cgroup *desc = container_of(css, struct cgroup, self);
>>
>> +		activate_effective_progs(desc, type, desc->bpf.inactive);
>> +		desc->bpf.inactive = NULL;
>> +	}
>> +
>> +	static_branch_inc(&cgroup_bpf_enabled_key);
>>  	if (old_prog) {
>>  		bpf_prog_put(old_prog);
>>  		static_branch_dec(&cgroup_bpf_enabled_key);
>>  	}
>>  	return 0;
>
> It's not clear to me that the static_branch_inc and static_branch_dec's
> are equal since the dec is in the loop over each program in the list,
> but the inc is not in a loop.

i'm not sure what you're trying to say.
The first loop quoted above is inside cgroup_bpf_put()
which is called when cgroup is destroyed. At this point
we're detaching and prog_put all attached programs.
While there is only one static_branch_inc() in __cgroup_bpf_attach()
that is called every time the prog is attached to a cgroup.
So what's the concern?
Note we're doing branch_dec only for progs in prog_list.
Just like we do branch_inc only for progs in prog_list.
Computing prog_array doesn't involve manipulations with prog's refcnt
and no branch_inc/dec either.