netdev - Re: [PATCH RFC bpf-next v1 0/6] Introduce unstable CT lookup helpers

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:   Tue, 2 Nov 2021 16:16:42 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc:     bpf@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        Maxim Mikityanskiy <maximmi@...dia.com>,
        Florian Westphal <fw@...len.de>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        netdev@...r.kernel.org, netfilter-devel@...r.kernel.org
Subject: Re: [PATCH RFC bpf-next v1 0/6] Introduce unstable CT lookup helpers

On Sat, Oct 30, 2021 at 08:16:03PM +0530, Kumar Kartikeya Dwivedi wrote:
> This series adds unstable conntrack lookup helpers using BPF kfunc support.  The
> patch adding the lookup helper is based off of Maxim's recent patch to aid in
> rebasing their series on top of this, all adjusted to work with kfunc support
> [0].
> 
> This is an RFC series, as I'm unsure whether the reference tracking for
> PTR_TO_BTF_ID will be accepted.

Yes. The patches look good overall.
Please don't do __BPF_RET_TYPE_MAX signalling. It's an ambiguous name.
_MAX is typically used for a different purpose. Just give it an explicit name.
I don't fully understand why that skip is needed though.
Why it's not one of existing RET_*. Duplication of return and
being lazy to propagate the correct ret value into get_kfunc_return_type ?

> If not, we can go back to doing it the typical
> way with PTR_TO_NF_CONN type, guarded with #if IS_ENABLED(CONFIG_NF_CONNTRACK).

Please don't. We already have a ton of special and custom types in the verifier.
refcnted PTR_TO_BTF_ID sounds as good way to scale it.

> Also, I want to understand whether it would make sense to introduce
> check_helper_call style bpf_func_proto based argument checking for kfuncs, or
> continue with how it is right now, since it doesn't seem correct that PTR_TO_MEM
> can be passed where PTR_TO_BTF_ID may be expected. Only PTR_TO_CTX is enforced.

Do we really allow to pass PTR_TO_MEM argument into a function that expects PTR_TO_BTF_ID ?
That sounds like a bug that we need to fix.