On Fri, Feb 19, 2021 at 07:46 PM CET, Cong Wang wrote:

On Fri, Feb 19, 2021 at 10:25 AM Jakub Sitnicki [off-list ref] wrote:

quoted

On Tue, Feb 16, 2021 at 07:42 AM CET, Cong Wang wrote:

quoted

From: Cong Wang <redacted>

As suggested by John, clean up sockmap related Kconfigs:

Reduce the scope of CONFIG_BPF_STREAM_PARSER down to TCP stream
parser, to reflect its name.

Make the rest sockmap code simply depend on CONFIG_BPF_SYSCALL.
And leave CONFIG_NET_SOCK_MSG untouched, as it is used by
non-sockmap cases.

Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Sitnicki <jakub@cloudflare.com>
Reviewed-by: Lorenz Bauer <redacted>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <redacted>
---

Sorry for the delay. There's a lot happening here. Took me a while to
dig through it.

I have a couple of nit-picks, which easily can be addressed as
follow-ups, and one comment.

No problem, it is not merged, so V5 is definitely not a problem.

quoted

sock_map_prog_update and sk_psock_done_strp are only used in
net/core/sock_map.c and can be static.

1. This seems to be unrelated to this patch? But I am still happy to
address it.

Completely unrelated. Just a nit-pick. Feel free to ignore.

2. sk_psock_done_strp() is in skmsg.c, hence why it is non-static.
And I believe it fits in skmsg.c better than in sock_map.c, because
it operates on psock rather than sock_map itself.

I wrote that sk_psock_done_strp is used only in net/core/sock_map.c,
while I should have written that it's used only in net/core/skmsg.c.

Sorry, a mistake when copying from my notes. Also, feel free to ignore.

So, I can make sock_map_prog_update() static in a separate patch
and carry it in V5.

Completely up to you. I don't insist.

quoted

[...]

quoted

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index bc7d2a586e18..b2c4865eb39b 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c

@@ -229,7 +229,6 @@ int tcp_bpf_sendmsg_redir(struct sock *sk, struct sk_msg *msg,
 }
 EXPORT_SYMBOL_GPL(tcp_bpf_sendmsg_redir);

-#ifdef CONFIG_BPF_STREAM_PARSER
 static bool tcp_bpf_stream_read(const struct sock *sk)
 {
      struct sk_psock *psock;

@@ -561,8 +560,10 @@ static void tcp_bpf_rebuild_protos(struct proto prot[TCP_BPF_NUM_CFGS],
                                 struct proto *base)
 {
      prot[TCP_BPF_BASE]                      = *base;
+#if defined(CONFIG_BPF_SYSCALL)
      prot[TCP_BPF_BASE].unhash               = sock_map_unhash;
      prot[TCP_BPF_BASE].close                = sock_map_close;
+#endif
      prot[TCP_BPF_BASE].recvmsg              = tcp_bpf_recvmsg;
      prot[TCP_BPF_BASE].stream_memory_read   = tcp_bpf_stream_read;

@@ -629,4 +630,3 @@ void tcp_bpf_clone(const struct sock *sk, struct sock *newsk)
      if (prot == &tcp_bpf_prots[family][TCP_BPF_BASE])
              newsk->sk_prot = sk->sk_prot_creator;
 }
-#endif /* CONFIG_BPF_STREAM_PARSER */

net/core/sock_map.o now is built only when CONFIG_BPF_SYSCALL is set.
While tcp_bpf_get_proto is only called from net/core/sock_map.o.

Seems there is no sense in compiling tcp_bpf_get_proto, and everything
it depends on which was enclosed by CONFIG_BPF_STREAM_PARSER check, when
CONFIG_BPF_SYSCALL is unset.

I can try but I am definitely not sure whether kTLS is happy about
it, clearly kTLS at least uses __tcp_bpf_recvmsg() and
tcp_bpf_sendmsg_redir().

I think kTLS will be fine because that's the situation today.

__tcp_bpf_recvmsg and tcp_bpf_sendmsg_redir need to be left out,
naturally, is it is now.

(Although I think they could event be stubbed out. But that would be
unrelated to this change.)

After all how would we end up on code path in kTLS that utilizes sockmap
API, if sockmap can't be created when CONFIG_BPF_SYSCALL is unset.

quoted

quoted

diff --git a/net/ipv4/udp_bpf.c b/net/ipv4/udp_bpf.c
index 7a94791efc1a..e635ccc175ca 100644
--- a/net/ipv4/udp_bpf.c
+++ b/net/ipv4/udp_bpf.c

@@ -18,8 +18,10 @@ static struct proto udp_bpf_prots[UDP_BPF_NUM_PROTS];
 static void udp_bpf_rebuild_protos(struct proto *prot, const struct proto *base)
 {
      *prot        = *base;
+#if defined(CONFIG_BPF_SYSCALL)
      prot->unhash = sock_map_unhash;
      prot->close  = sock_map_close;
+#endif
 }

 static void udp_bpf_check_v6_needs_rebuild(struct proto *ops)

Same situation here but for udp_bpf_get_proto.

UDP is different, as kTLS certainly doesn't and won't use it. I think
udp_bpf.c can be just put under CONFIG_BPF_SYSCALL.

Thanks.