netdev - Re: [PATCH net-next] bpf: add missing rcu protection when releasing programs from prog

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: <5568F4C3.5010007@plumgrid.com>
Date:	Fri, 29 May 2015 16:22:43 -0700
From:	Alexei Starovoitov <ast@...mgrid.com>
To:	Daniel Borkmann <daniel@...earbox.net>,
	"David S. Miller" <davem@...emloft.net>
CC:	Michael Holzheu <holzheu@...ux.vnet.ibm.com>,
	Martin Schwidefsky <schwidefsky@...ibm.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next] bpf: add missing rcu protection when releasing
 programs from prog_array

On 5/29/15 2:10 AM, Daniel Borkmann wrote:
>>
>> +static void __prog_put_rcu(struct rcu_head *rcu)
>> +{
>> +    struct bpf_prog_aux *aux = container_of(rcu, struct bpf_prog_aux,
>> rcu);
>> +
>> +    free_used_maps(aux);
>> +    bpf_prog_free(aux->prog);
>
> Not sure if it's worth it to move these two into a common helper shared
> with bpf_prog_put()? Probably only in case that code should get further
> extended.

I though about it too, but my recent re-reading of net/core/filter.c
taught me otherwise. We have too many tiny helper functions that
are hiding meaning instead of helping.
Like instead of having two pieces of the code:
do1(); do2(); do3(); and do1(); do2();
if we introduce a helper foo() { do1(); do2(); } and the code will do:
foo(), do3() and foo()
when the helper is close enough to invocation it's still easy to read,
but overtime the whole thing, imo, will become a mess. For example,
we have prog_release, prog_free, filter_release and all combinations
with and without __ prefix and _rcu suffix.
I think some of this stuff should be 'unhelpered'.
Like __sk_filter_release() and __bpf_prog_release() should be removed.
Of course, it's a grey line when to introduce a helper and when not to,
but just because two lines are close enough between two functions it
doesn't mean that helper is warranted. In this bpf_prog_put() case
I think helper is not needed _today_. If it grows, we'll reconsider.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html