netdev - Re: [PATCH v3] cgroup/bpf: fast path skb BPF filtering

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:   Mon, 24 Jan 2022 15:46:33 +0000
From:   Pavel Begunkov <asml.silence@...il.com>
To:     Stanislav Fomichev <sdf@...gle.com>,
        Martin KaFai Lau <kafai@...com>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Song Liu <songliubraving@...com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] cgroup/bpf: fast path skb BPF filtering

On 12/16/21 18:24, Stanislav Fomichev wrote:
> On Thu, Dec 16, 2021 at 10:14 AM Martin KaFai Lau <kafai@...com> wrote:
>> On Thu, Dec 16, 2021 at 01:21:26PM +0000, Pavel Begunkov wrote:
>>> On 12/15/21 22:07, Stanislav Fomichev wrote:
>>>>> I'm skeptical I'll be able to measure inlining one function,
>>>>> variability between boots/runs is usually greater and would hide it.
>>>>
>>>> Right, that's why I suggested to mirror what we do in set/getsockopt
>>>> instead of the new extra CGROUP_BPF_TYPE_ENABLED. But I'll leave it up
>>>> to you, Martin and the rest.
>> I also suggested to try to stay with one way for fullsock context in v2
>> but it is for code readability reason.
>>
>> How about calling CGROUP_BPF_TYPE_ENABLED() just next to cgroup_bpf_enabled()
>> in BPF_CGROUP_RUN_PROG_*SOCKOPT_*() instead ?
> 
> SG!
> 
>> It is because both cgroup_bpf_enabled() and CGROUP_BPF_TYPE_ENABLED()
>> want to check if there is bpf to run before proceeding everything else
>> and then I don't need to jump to the non-inline function itself to see
>> if there is other prog array empty check.
>>
>> Stan, do you have concern on an extra inlined sock_cgroup_ptr()
>> when there is bpf prog to run for set/getsockopt()?  I think
>> it should be mostly noise from looking at
>> __cgroup_bpf_run_filter_*sockopt()?
> 
> Yeah, my concern is also mostly about readability/consistency. Either
> __cgroup_bpf_prog_array_is_empty everywhere or this new
> CGROUP_BPF_TYPE_ENABLED everywhere. I'm slightly leaning towards
> __cgroup_bpf_prog_array_is_empty because I don't believe direct
> function calls add any visible overhead and macros are ugly :-) But
> either way is fine as long as it looks consistent.

Martin, Stanislav, do you think it's good to go? Any other concerns?
It feels it might end with bikeshedding and would be great to finally
get it done, especially since I find the issue to be pretty simple.

-- 
Pavel Begunkov