[bpf-next,v5,0/6] Support direct writes to nf_conn:mark

Message

Daniel Xu Sept. 7, 2022, 4:40 p.m. UTC

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.

Past discussion:
- v4: https://lore.kernel.org/bpf/[e-mail]/
- v3: https://lore.kernel.org/bpf/[e-mail]/
- v2: https://lore.kernel.org/bpf/[e-mail]/T/
- v1: https://lore.kernel.org/bpf/[e-mail]/

Changes since v4:
- Use exported function pointer + mutex to handle CONFIG_NF_CONNTRACK=m
  case

Changes since v3:
- Use a mutex to protect module load/unload critical section

Changes since v2:
- Remove use of NOT_INIT for btf_struct_access write path
- Disallow nf_conn writing when nf_conntrack module not loaded
- Support writing to nf_conn___init:mark

Changes since v1:
- Add unimplemented stub for when !CONFIG_BPF_SYSCALL


Daniel Xu (6):
  bpf: Remove duplicate PTR_TO_BTF_ID RO check
  bpf: Add stub for btf_struct_access()
  bpf: Use 0 instead of NOT_INIT for btf_struct_access() writes
  bpf: Export btf_type_by_id() and bpf_log()
  bpf: Add support for writing to nf_conn:mark
  selftests/bpf: Add tests for writing to nf_conn:mark

 include/linux/bpf.h                           |  9 +++
 include/net/netfilter/nf_conntrack_bpf.h      | 23 +++++++
 kernel/bpf/btf.c                              |  1 +
 kernel/bpf/verifier.c                         |  4 +-
 net/core/filter.c                             | 54 +++++++++++++++
 net/ipv4/bpf_tcp_ca.c                         |  2 +-
 net/netfilter/nf_conntrack_bpf.c              | 66 ++++++++++++++++++-
 net/netfilter/nf_conntrack_core.c             |  1 +
 .../testing/selftests/bpf/prog_tests/bpf_nf.c |  2 +
 .../testing/selftests/bpf/progs/test_bpf_nf.c |  9 ++-
 .../selftests/bpf/progs/test_bpf_nf_fail.c    | 14 ++++
 11 files changed, 178 insertions(+), 7 deletions(-)

Comments

Kumar Kartikeya Dwivedi Sept. 10, 2022, 12:27 a.m. UTC | #1

On Wed, 7 Sept 2022 at 18:41, Daniel Xu <[e-mail]> 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.
>

There are a couple of compile time warnings when conntrack is disabled,

../net/core/filter.c:8608:1: warning: symbol 'nf_conn_btf_access_lock'
was not declared. Should it be static?
../net/core/filter.c:8611:5: warning: symbol 'nfct_bsa' was not
declared. Should it be static?

Most likely because extern declaration is guarded by ifdefs. So just
moving those out of ifdef should work.
I guess you can send that as a follow up fix, or roll it in if you end
up respinning.

Otherwise, for the series:
Acked-by: Kumar Kartikeya Dwivedi <[e-mail]>

[e-mail] Sept. 11, 2022, 12:40 a.m. UTC | #2

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <[e-mail]>:

On Wed,  7 Sep 2022 10:40:35 -0600 you 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.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v5,1/6] bpf: Remove duplicate PTR_TO_BTF_ID RO check
    https://git.kernel.org/bpf/bpf-next/c/65269888c695
  - [bpf-next,v5,2/6] bpf: Add stub for btf_struct_access()
    https://git.kernel.org/bpf/bpf-next/c/d4f7bdb2ed7b
  - [bpf-next,v5,3/6] bpf: Use 0 instead of NOT_INIT for btf_struct_access() writes
    https://git.kernel.org/bpf/bpf-next/c/896f07c07da0
  - [bpf-next,v5,4/6] bpf: Export btf_type_by_id() and bpf_log()
    https://git.kernel.org/bpf/bpf-next/c/84c6ac417cea
  - [bpf-next,v5,5/6] bpf: Add support for writing to nf_conn:mark
    https://git.kernel.org/bpf/bpf-next/c/864b656f82cc
  - [bpf-next,v5,6/6] selftests/bpf: Add tests for writing to nf_conn:mark
    https://git.kernel.org/bpf/bpf-next/c/e2d75e954c0a

You are awesome, thank you!

Daniel Xu Sept. 11, 2022, 5:26 p.m. UTC | #3

Hi Kumar,

On Sat, Sep 10, 2022 at 02:27:38AM +0200, Kumar Kartikeya Dwivedi wrote:
> On Wed, 7 Sept 2022 at 18:41, Daniel Xu <[e-mail]> 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.
> >
> 
> There are a couple of compile time warnings when conntrack is disabled,
> 
> ../net/core/filter.c:8608:1: warning: symbol 'nf_conn_btf_access_lock'
> was not declared. Should it be static?
> ../net/core/filter.c:8611:5: warning: symbol 'nfct_bsa' was not
> declared. Should it be static?
> 
> Most likely because extern declaration is guarded by ifdefs. So just
> moving those out of ifdef should work.
> I guess you can send that as a follow up fix, or roll it in if you end
> up respinning.

Hmm, I don't see how filter.c ever #include's nf_conntrack_bpf.h. So
you'd think that the warning would always be present regardless of
CONFIG_NF_CONNTRACK setting.

FWIW I can't reproduce the warning even with CONFIG_NF_CONNTRACK=n.

Maybe the extern declarations should be in include/linux/filter.h
anyways? Might be cleaner. WDYT?

> Otherwise, for the series:
> Acked-by: Kumar Kartikeya Dwivedi <[e-mail]>

Thanks!

Daniel