netdev - Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf

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, 16 Aug 2022 12:43:32 +0200
From:   Toke Høiland-Jørgensen <toke@...nel.org>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Florian Westphal <fw@...len.de>
Cc:     Daniel Xu <dxu@...uu.xyz>, bpf <bpf@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Kumar Kartikeya Dwivedi <memxor@...il.com>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        netfilter-devel <netfilter-devel@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark

Alexei Starovoitov <alexei.starovoitov@...il.com> writes:

> On Mon, Aug 15, 2022 at 3:40 PM Florian Westphal <fw@...len.de> wrote:
>>
>> Toke Høiland-Jørgensen <toke@...nel.org> wrote:
>> > > Support direct writes to nf_conn:mark from TC and XDP prog types. This
>> > > is useful when applications want to store per-connection metadata. This
>> > > is also particularly useful for applications that run both bpf and
>> > > iptables/nftables because the latter can trivially access this metadata.
>> > >
>> > > One example use case would be if a bpf prog is responsible for advanced
>> > > packet classification and iptables/nftables is later used for routing
>> > > due to pre-existing/legacy code.
>> > >
>> > > Signed-off-by: Daniel Xu <dxu@...uu.xyz>
>> >
>> > Didn't we agree the last time around that all field access should be
>> > using helper kfuncs instead of allowing direct writes to struct nf_conn?
>>
>> I don't see why ct->mark needs special handling.
>>
>> It might be possible we need to change accesses on nf/tc side to use
>> READ/WRITE_ONCE though.
>
> +1
> I don't think we need to have a hard rule.
> If fields is safe to access directly than it's faster
> to let bpf prog read/write it.
> There are no backward compat concerns. If conntrack side decides
> to make that field special we can disallow direct writes in
> the same kernel version.

Right, I was under the impression we wanted all fields to be wrapper by
helpers so that the struct owner could change their semantics without
affecting users (and solve the performance issue by figuring out a
generic way to inline those helpers). I guess there could also be an API
consistency argument for doing this.

However, I don't have a strong opinion on this, so if y'all prefer
keeping these as direct field writes, that's OK with me.

> These accesses, just like kfuncs, are unstable.

Well, it will be interesting to see how that plays out the first time
an application relying on one of these breaks on a kernel upgrade :)

-Toke