netdev - Re: [PATCH bpf-next v2 3/4] 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:   Thu, 18 Aug 2022 16:10:32 -0600
From:   Daniel Xu <dxu@...uu.xyz>
To:     Toke Høiland-Jørgensen <toke@...nel.org>
Cc:     bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        andrii@...nel.org, memxor@...il.com, pablo@...filter.org,
        fw@...len.de, netfilter-devel@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 3/4] bpf: Add support for writing to
 nf_conn:mark

Hi Toke,

On Thu, Aug 18, 2022 at 09:52:08PM +0200, Toke Høiland-Jørgensen wrote:
> Daniel Xu <dxu@...uu.xyz> writes:
> 
> > 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.
> 
> Looking closer at the nf_conn definition, the mark field (and possibly
> secmark) seems to be the only field that is likely to be feasible to
> support direct writes to, as everything else either requires special
> handling (like status and timeout), or they are composite field that
> will require helpers anyway to use correctly.
> 
> Which means we're in the process of creating an API where users have to
> call helpers to fill in all fields *except* this one field that happens
> to be directly writable. That seems like a really confusing and
> inconsistent API, so IMO it strengthens the case for just making a
> helper for this field as well, even though it adds a bit of overhead
> (and then solving the overhead issue in a more generic way such as by
> supporting clever inlining).
> 
> -Toke

I don't particularly have a strong opinion here. But to play devil's
advocate:

* It may be confusing now, but over time I expect to see more direct
  write support via BTF, especially b/c there is support for unstable
  helpers now. So perhaps in the future it will seem more sensible.

* The unstable helpers do not have external documentation. Nor should
  they in my opinion as their unstableness + stale docs may lead to
  undesirable outcomes. So users of the unstable API already have to
  splunk through kernel code and/or selftests to figure out how to wield
  the APIs. All this to say there may not be an argument for
  discoverability.

* Direct writes are slightly more ergnomic than using a helper.

Thanks,
Daniel