* [PATCH bpf-next v2 0/6] Add SO_REUSEPORT support for TC bpf_sk_assign
@ 2023-06-13 10:14 Lorenz Bauer
2023-06-13 10:14 ` [PATCH bpf-next v2 1/6] net: export inet_lookup_reuseport and inet6_lookup_reuseport Lorenz Bauer
` (5 more replies)
0 siblings, 6 replies; 22+ messages in thread
From: Lorenz Bauer @ 2023-06-13 10:14 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
David Ahern, Willem de Bruijn, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Joe Stringer, Mykola Lysenko, Shuah Khan,
Kuniyuki Iwashima
Cc: Hemanth Malla, netdev, linux-kernel, bpf, linux-kselftest,
Lorenz Bauer, Joe Stringer
We want to replace iptables TPROXY with a BPF program at TC ingress.
To make this work in all cases we need to assign a SO_REUSEPORT socket
to an skb, which is currently prohibited. This series adds support for
such sockets to bpf_sk_assing. See patch 5 for details.
I did some refactoring to cut down on the amount of duplicate code. The
key to this is to use INDIRECT_CALL in the reuseport helpers. To show
that this approach is not just beneficial to TC sk_assign I removed
duplicate code for bpf_sk_lookup as well.
Changes from v1:
- Correct commit abbrev length (Kuniyuki)
- Reduce duplication (Kuniyuki)
- Add checks on sk_state (Martin)
- Split exporting inet[6]_lookup_reuseport into separate patch (Eric)
Joint work with Daniel Borkmann.
Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
---
Daniel Borkmann (1):
selftests/bpf: Test that SO_REUSEPORT can be used with sk_assign helper
Lorenz Bauer (5):
net: export inet_lookup_reuseport and inet6_lookup_reuseport
net: document inet[6]_lookup_reuseport sk_state requirements
net: remove duplicate reuseport_lookup functions
net: remove duplicate sk_lookup helpers
bpf, net: Support SO_REUSEPORT sockets with bpf_sk_assign
include/net/inet6_hashtables.h | 84 ++++++++-
include/net/inet_hashtables.h | 77 +++++++-
include/net/sock.h | 7 +-
include/uapi/linux/bpf.h | 3 -
net/core/filter.c | 2 -
net/ipv4/inet_hashtables.c | 69 +++++---
net/ipv4/udp.c | 73 +++-----
net/ipv6/inet6_hashtables.c | 71 +++++---
net/ipv6/udp.c | 85 +++------
tools/include/uapi/linux/bpf.h | 3 -
tools/testing/selftests/bpf/network_helpers.c | 3 +
.../selftests/bpf/prog_tests/assign_reuse.c | 197 +++++++++++++++++++++
.../selftests/bpf/progs/test_assign_reuse.c | 142 +++++++++++++++
13 files changed, 637 insertions(+), 179 deletions(-)
---
base-commit: 25085b4e9251c77758964a8e8651338972353642
change-id: 20230613-so-reuseport-e92c526173ee
Best regards,
--
Lorenz Bauer <lmb@isovalent.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH bpf-next v2 1/6] net: export inet_lookup_reuseport and inet6_lookup_reuseport
2023-06-13 10:14 [PATCH bpf-next v2 0/6] Add SO_REUSEPORT support for TC bpf_sk_assign Lorenz Bauer
@ 2023-06-13 10:14 ` Lorenz Bauer
2023-06-13 10:14 ` [PATCH bpf-next v2 2/6] net: document inet[6]_lookup_reuseport sk_state requirements Lorenz Bauer
` (4 subsequent siblings)
5 siblings, 0 replies; 22+ messages in thread
From: Lorenz Bauer @ 2023-06-13 10:14 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
David Ahern, Willem de Bruijn, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Joe Stringer, Mykola Lysenko, Shuah Khan,
Kuniyuki Iwashima
Cc: Hemanth Malla, netdev, linux-kernel, bpf, linux-kselftest, Lorenz Bauer
Rename the existing reuseport helpers for IPv4 and IPv6 so that they
can be invoked in the follow up commit. Export them so that DCCP which
may be built as a module can access them.
No change in functionality.
Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
---
include/net/inet6_hashtables.h | 7 +++++++
include/net/inet_hashtables.h | 5 +++++
net/ipv4/inet_hashtables.c | 15 ++++++++-------
net/ipv6/inet6_hashtables.c | 19 ++++++++++---------
4 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/include/net/inet6_hashtables.h b/include/net/inet6_hashtables.h
index 56f1286583d3..032ddab48f8f 100644
--- a/include/net/inet6_hashtables.h
+++ b/include/net/inet6_hashtables.h
@@ -48,6 +48,13 @@ struct sock *__inet6_lookup_established(struct net *net,
const u16 hnum, const int dif,
const int sdif);
+struct sock *inet6_lookup_reuseport(struct net *net, struct sock *sk,
+ struct sk_buff *skb, int doff,
+ const struct in6_addr *saddr,
+ __be16 sport,
+ const struct in6_addr *daddr,
+ unsigned short hnum);
+
struct sock *inet6_lookup_listener(struct net *net,
struct inet_hashinfo *hashinfo,
struct sk_buff *skb, int doff,
diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index 99bd823e97f6..8734f3488f5d 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -379,6 +379,11 @@ struct sock *__inet_lookup_established(struct net *net,
const __be32 daddr, const u16 hnum,
const int dif, const int sdif);
+struct sock *inet_lookup_reuseport(struct net *net, struct sock *sk,
+ struct sk_buff *skb, int doff,
+ __be32 saddr, __be16 sport,
+ __be32 daddr, unsigned short hnum);
+
static inline struct sock *
inet_lookup_established(struct net *net, struct inet_hashinfo *hashinfo,
const __be32 saddr, const __be16 sport,
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index e7391bf310a7..920131e4a65d 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -332,10 +332,10 @@ static inline int compute_score(struct sock *sk, struct net *net,
return score;
}
-static inline struct sock *lookup_reuseport(struct net *net, struct sock *sk,
- struct sk_buff *skb, int doff,
- __be32 saddr, __be16 sport,
- __be32 daddr, unsigned short hnum)
+struct sock *inet_lookup_reuseport(struct net *net, struct sock *sk,
+ struct sk_buff *skb, int doff,
+ __be32 saddr, __be16 sport,
+ __be32 daddr, unsigned short hnum)
{
struct sock *reuse_sk = NULL;
u32 phash;
@@ -346,6 +346,7 @@ static inline struct sock *lookup_reuseport(struct net *net, struct sock *sk,
}
return reuse_sk;
}
+EXPORT_SYMBOL_GPL(inet_lookup_reuseport);
/*
* Here are some nice properties to exploit here. The BSD API
@@ -369,8 +370,8 @@ static struct sock *inet_lhash2_lookup(struct net *net,
sk_nulls_for_each_rcu(sk, node, &ilb2->nulls_head) {
score = compute_score(sk, net, hnum, daddr, dif, sdif);
if (score > hiscore) {
- result = lookup_reuseport(net, sk, skb, doff,
- saddr, sport, daddr, hnum);
+ result = inet_lookup_reuseport(net, sk, skb, doff,
+ saddr, sport, daddr, hnum);
if (result)
return result;
@@ -399,7 +400,7 @@ static inline struct sock *inet_lookup_run_bpf(struct net *net,
if (no_reuseport || IS_ERR_OR_NULL(sk))
return sk;
- reuse_sk = lookup_reuseport(net, sk, skb, doff, saddr, sport, daddr, hnum);
+ reuse_sk = inet_lookup_reuseport(net, sk, skb, doff, saddr, sport, daddr, hnum);
if (reuse_sk)
sk = reuse_sk;
return sk;
diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
index b64b49012655..b7c56867314e 100644
--- a/net/ipv6/inet6_hashtables.c
+++ b/net/ipv6/inet6_hashtables.c
@@ -111,12 +111,12 @@ static inline int compute_score(struct sock *sk, struct net *net,
return score;
}
-static inline struct sock *lookup_reuseport(struct net *net, struct sock *sk,
- struct sk_buff *skb, int doff,
- const struct in6_addr *saddr,
- __be16 sport,
- const struct in6_addr *daddr,
- unsigned short hnum)
+struct sock *inet6_lookup_reuseport(struct net *net, struct sock *sk,
+ struct sk_buff *skb, int doff,
+ const struct in6_addr *saddr,
+ __be16 sport,
+ const struct in6_addr *daddr,
+ unsigned short hnum)
{
struct sock *reuse_sk = NULL;
u32 phash;
@@ -127,6 +127,7 @@ static inline struct sock *lookup_reuseport(struct net *net, struct sock *sk,
}
return reuse_sk;
}
+EXPORT_SYMBOL_GPL(inet6_lookup_reuseport);
/* called with rcu_read_lock() */
static struct sock *inet6_lhash2_lookup(struct net *net,
@@ -143,8 +144,8 @@ static struct sock *inet6_lhash2_lookup(struct net *net,
sk_nulls_for_each_rcu(sk, node, &ilb2->nulls_head) {
score = compute_score(sk, net, hnum, daddr, dif, sdif);
if (score > hiscore) {
- result = lookup_reuseport(net, sk, skb, doff,
- saddr, sport, daddr, hnum);
+ result = inet6_lookup_reuseport(net, sk, skb, doff,
+ saddr, sport, daddr, hnum);
if (result)
return result;
@@ -175,7 +176,7 @@ static inline struct sock *inet6_lookup_run_bpf(struct net *net,
if (no_reuseport || IS_ERR_OR_NULL(sk))
return sk;
- reuse_sk = lookup_reuseport(net, sk, skb, doff, saddr, sport, daddr, hnum);
+ reuse_sk = inet6_lookup_reuseport(net, sk, skb, doff, saddr, sport, daddr, hnum);
if (reuse_sk)
sk = reuse_sk;
return sk;
--
2.40.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH bpf-next v2 2/6] net: document inet[6]_lookup_reuseport sk_state requirements
2023-06-13 10:14 [PATCH bpf-next v2 0/6] Add SO_REUSEPORT support for TC bpf_sk_assign Lorenz Bauer
2023-06-13 10:14 ` [PATCH bpf-next v2 1/6] net: export inet_lookup_reuseport and inet6_lookup_reuseport Lorenz Bauer
@ 2023-06-13 10:14 ` Lorenz Bauer
2023-06-13 10:14 ` [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions Lorenz Bauer
` (3 subsequent siblings)
5 siblings, 0 replies; 22+ messages in thread
From: Lorenz Bauer @ 2023-06-13 10:14 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
David Ahern, Willem de Bruijn, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Joe Stringer, Mykola Lysenko, Shuah Khan,
Kuniyuki Iwashima
Cc: Hemanth Malla, netdev, linux-kernel, bpf, linux-kselftest, Lorenz Bauer
The current implementation was extracted from inet[6]_lhash2_lookup
in commit 80b373f74f9e ("inet: Extract helper for selecting socket
from reuseport group") and commit 5df6531292b5 ("inet6: Extract helper
for selecting socket from reuseport group"). In the original context,
sk is always in TCP_LISTEN state and so did not have a separate check.
Add documentation that specifies which sk_state are valid to pass to
the function.
Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
---
net/ipv4/inet_hashtables.c | 14 ++++++++++++++
net/ipv6/inet6_hashtables.c | 14 ++++++++++++++
2 files changed, 28 insertions(+)
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 920131e4a65d..91f9210d4e83 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -332,6 +332,20 @@ static inline int compute_score(struct sock *sk, struct net *net,
return score;
}
+/**
+ * inet_lookup_reuseport() - execute reuseport logic on AF_INET socket if necessary.
+ * @net: network namespace.
+ * @sk: AF_INET socket, must be in TCP_LISTEN state for TCP or TCP_CLOSE for UDP.
+ * @skb: context for a potential SK_REUSEPORT program.
+ * @doff: header offset.
+ * @saddr: source address.
+ * @sport: source port.
+ * @daddr: destination address.
+ * @hnum: destination port in host byte order.
+ *
+ * Return: NULL if sk doesn't have SO_REUSEPORT set, otherwise a pointer to
+ * the selected sock or an error.
+ */
struct sock *inet_lookup_reuseport(struct net *net, struct sock *sk,
struct sk_buff *skb, int doff,
__be32 saddr, __be16 sport,
diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
index b7c56867314e..208998694ae3 100644
--- a/net/ipv6/inet6_hashtables.c
+++ b/net/ipv6/inet6_hashtables.c
@@ -111,6 +111,20 @@ static inline int compute_score(struct sock *sk, struct net *net,
return score;
}
+/**
+ * inet6_lookup_reuseport() - execute reuseport logic on AF_INET6 socket if necessary.
+ * @net: network namespace.
+ * @sk: AF_INET6 socket, must be in TCP_LISTEN state for TCP or TCP_CLOSE for UDP.
+ * @skb: context for a potential SK_REUSEPORT program.
+ * @doff: header offset.
+ * @saddr: source address.
+ * @sport: source port.
+ * @daddr: destination address.
+ * @hnum: destination port in host byte order.
+ *
+ * Return: NULL if sk doesn't have SO_REUSEPORT set, otherwise a pointer to
+ * the selected sock or an error.
+ */
struct sock *inet6_lookup_reuseport(struct net *net, struct sock *sk,
struct sk_buff *skb, int doff,
const struct in6_addr *saddr,
--
2.40.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions
2023-06-13 10:14 [PATCH bpf-next v2 0/6] Add SO_REUSEPORT support for TC bpf_sk_assign Lorenz Bauer
2023-06-13 10:14 ` [PATCH bpf-next v2 1/6] net: export inet_lookup_reuseport and inet6_lookup_reuseport Lorenz Bauer
2023-06-13 10:14 ` [PATCH bpf-next v2 2/6] net: document inet[6]_lookup_reuseport sk_state requirements Lorenz Bauer
@ 2023-06-13 10:14 ` Lorenz Bauer
2023-06-13 15:32 ` Simon Horman
` (2 more replies)
2023-06-13 10:14 ` [PATCH bpf-next v2 4/6] net: remove duplicate sk_lookup helpers Lorenz Bauer
` (2 subsequent siblings)
5 siblings, 3 replies; 22+ messages in thread
From: Lorenz Bauer @ 2023-06-13 10:14 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
David Ahern, Willem de Bruijn, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Joe Stringer, Mykola Lysenko, Shuah Khan,
Kuniyuki Iwashima
Cc: Hemanth Malla, netdev, linux-kernel, bpf, linux-kselftest, Lorenz Bauer
There are currently four copies of reuseport_lookup: one each for
(TCP, UDP)x(IPv4, IPv6). This forces us to duplicate all callers of
those functions as well. This is already the case for sk_lookup
helpers (inet,inet6,udp4,udp6)_lookup_run_bpf.
The only difference between the reuseport_lookup helpers is calling
a different hash function. Cut down the number of reuseport_lookup
functions to one per IP version by using the INDIRECT_CALL
infrastructure.
Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
---
include/net/inet6_hashtables.h | 11 ++++++++++-
include/net/inet_hashtables.h | 15 +++++++++-----
net/ipv4/inet_hashtables.c | 22 ++++++++++++++-------
net/ipv4/udp.c | 37 +++++++++++-----------------------
net/ipv6/inet6_hashtables.c | 16 +++++++++++----
net/ipv6/udp.c | 45 +++++++++++++++---------------------------
6 files changed, 75 insertions(+), 71 deletions(-)
diff --git a/include/net/inet6_hashtables.h b/include/net/inet6_hashtables.h
index 032ddab48f8f..49d586454287 100644
--- a/include/net/inet6_hashtables.h
+++ b/include/net/inet6_hashtables.h
@@ -48,12 +48,21 @@ struct sock *__inet6_lookup_established(struct net *net,
const u16 hnum, const int dif,
const int sdif);
+typedef u32 (*inet6_ehashfn_t)(const struct net *net,
+ const struct in6_addr *laddr, const u16 lport,
+ const struct in6_addr *faddr, const __be16 fport);
+
+u32 inet6_ehashfn(const struct net *net,
+ const struct in6_addr *laddr, const u16 lport,
+ const struct in6_addr *faddr, const __be16 fport);
+
struct sock *inet6_lookup_reuseport(struct net *net, struct sock *sk,
struct sk_buff *skb, int doff,
const struct in6_addr *saddr,
__be16 sport,
const struct in6_addr *daddr,
- unsigned short hnum);
+ unsigned short hnum,
+ inet6_ehashfn_t ehashfn);
struct sock *inet6_lookup_listener(struct net *net,
struct inet_hashinfo *hashinfo,
diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index 8734f3488f5d..51ab6a1a3601 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -379,10 +379,19 @@ struct sock *__inet_lookup_established(struct net *net,
const __be32 daddr, const u16 hnum,
const int dif, const int sdif);
+typedef u32 (*inet_ehashfn_t)(const struct net *net,
+ const __be32 laddr, const __u16 lport,
+ const __be32 faddr, const __be16 fport);
+
+u32 inet_ehashfn(const struct net *net,
+ const __be32 laddr, const __u16 lport,
+ const __be32 faddr, const __be16 fport);
+
struct sock *inet_lookup_reuseport(struct net *net, struct sock *sk,
struct sk_buff *skb, int doff,
__be32 saddr, __be16 sport,
- __be32 daddr, unsigned short hnum);
+ __be32 daddr, unsigned short hnum,
+ inet_ehashfn_t ehashfn);
static inline struct sock *
inet_lookup_established(struct net *net, struct inet_hashinfo *hashinfo,
@@ -453,10 +462,6 @@ static inline struct sock *__inet_lookup_skb(struct inet_hashinfo *hashinfo,
refcounted);
}
-u32 inet6_ehashfn(const struct net *net,
- const struct in6_addr *laddr, const u16 lport,
- const struct in6_addr *faddr, const __be16 fport);
-
static inline void sk_daddr_set(struct sock *sk, __be32 addr)
{
sk->sk_daddr = addr; /* alias of inet_daddr */
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 91f9210d4e83..1ec895fd9905 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -28,9 +28,9 @@
#include <net/tcp.h>
#include <net/sock_reuseport.h>
-static u32 inet_ehashfn(const struct net *net, const __be32 laddr,
- const __u16 lport, const __be32 faddr,
- const __be16 fport)
+u32 inet_ehashfn(const struct net *net, const __be32 laddr,
+ const __u16 lport, const __be32 faddr,
+ const __be16 fport)
{
static u32 inet_ehash_secret __read_mostly;
@@ -332,6 +332,10 @@ static inline int compute_score(struct sock *sk, struct net *net,
return score;
}
+INDIRECT_CALLABLE_DECLARE(u32 udp_ehashfn(const struct net *,
+ const __be32, const __u16,
+ const __be32, const __be16));
+
/**
* inet_lookup_reuseport() - execute reuseport logic on AF_INET socket if necessary.
* @net: network namespace.
@@ -342,6 +346,7 @@ static inline int compute_score(struct sock *sk, struct net *net,
* @sport: source port.
* @daddr: destination address.
* @hnum: destination port in host byte order.
+ * @ehashfn: hash function used to generate the fallback hash.
*
* Return: NULL if sk doesn't have SO_REUSEPORT set, otherwise a pointer to
* the selected sock or an error.
@@ -349,13 +354,15 @@ static inline int compute_score(struct sock *sk, struct net *net,
struct sock *inet_lookup_reuseport(struct net *net, struct sock *sk,
struct sk_buff *skb, int doff,
__be32 saddr, __be16 sport,
- __be32 daddr, unsigned short hnum)
+ __be32 daddr, unsigned short hnum,
+ inet_ehashfn_t ehashfn)
{
struct sock *reuse_sk = NULL;
u32 phash;
if (sk->sk_reuseport) {
- phash = inet_ehashfn(net, daddr, hnum, saddr, sport);
+ phash = INDIRECT_CALL_2(ehashfn, inet_ehashfn, udp_ehashfn,
+ net, daddr, hnum, saddr, sport);
reuse_sk = reuseport_select_sock(sk, phash, skb, doff);
}
return reuse_sk;
@@ -385,7 +392,7 @@ static struct sock *inet_lhash2_lookup(struct net *net,
score = compute_score(sk, net, hnum, daddr, dif, sdif);
if (score > hiscore) {
result = inet_lookup_reuseport(net, sk, skb, doff,
- saddr, sport, daddr, hnum);
+ saddr, sport, daddr, hnum, inet_ehashfn);
if (result)
return result;
@@ -414,7 +421,8 @@ static inline struct sock *inet_lookup_run_bpf(struct net *net,
if (no_reuseport || IS_ERR_OR_NULL(sk))
return sk;
- reuse_sk = inet_lookup_reuseport(net, sk, skb, doff, saddr, sport, daddr, hnum);
+ reuse_sk = inet_lookup_reuseport(net, sk, skb, doff, saddr, sport, daddr, hnum,
+ inet_ehashfn);
if (reuse_sk)
sk = reuse_sk;
return sk;
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index fd3dae081f3a..10468fe144d0 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -405,9 +405,9 @@ static int compute_score(struct sock *sk, struct net *net,
return score;
}
-static u32 udp_ehashfn(const struct net *net, const __be32 laddr,
- const __u16 lport, const __be32 faddr,
- const __be16 fport)
+INDIRECT_CALLABLE_SCOPE
+u32 udp_ehashfn(const struct net *net, const __be32 laddr, const __u16 lport,
+ const __be32 faddr, const __be16 fport)
{
static u32 udp_ehash_secret __read_mostly;
@@ -417,22 +417,6 @@ static u32 udp_ehashfn(const struct net *net, const __be32 laddr,
udp_ehash_secret + net_hash_mix(net));
}
-static struct sock *lookup_reuseport(struct net *net, struct sock *sk,
- struct sk_buff *skb,
- __be32 saddr, __be16 sport,
- __be32 daddr, unsigned short hnum)
-{
- struct sock *reuse_sk = NULL;
- u32 hash;
-
- if (sk->sk_reuseport && sk->sk_state != TCP_ESTABLISHED) {
- hash = udp_ehashfn(net, daddr, hnum, saddr, sport);
- reuse_sk = reuseport_select_sock(sk, hash, skb,
- sizeof(struct udphdr));
- }
- return reuse_sk;
-}
-
/* called with rcu_read_lock() */
static struct sock *udp4_lib_lookup2(struct net *net,
__be32 saddr, __be16 sport,
@@ -450,11 +434,13 @@ static struct sock *udp4_lib_lookup2(struct net *net,
score = compute_score(sk, net, saddr, sport,
daddr, hnum, dif, sdif);
if (score > badness) {
- result = lookup_reuseport(net, sk, skb,
- saddr, sport, daddr, hnum);
- /* Fall back to scoring if group has connections */
- if (result && !reuseport_has_conns(sk))
- return result;
+ if (sk->sk_state != TCP_ESTABLISHED) {
+ result = inet_lookup_reuseport(net, sk, skb, sizeof(struct udphdr),
+ saddr, sport, daddr, hnum, udp_ehashfn);
+ /* Fall back to scoring if group has connections */
+ if (result && !reuseport_has_conns(sk))
+ return result;
+ }
result = result ? : sk;
badness = score;
@@ -480,7 +466,8 @@ static struct sock *udp4_lookup_run_bpf(struct net *net,
if (no_reuseport || IS_ERR_OR_NULL(sk))
return sk;
- reuse_sk = lookup_reuseport(net, sk, skb, saddr, sport, daddr, hnum);
+ reuse_sk = inet_lookup_reuseport(net, sk, skb, sizeof(struct udphdr),
+ saddr, sport, daddr, hnum, udp_ehashfn);
if (reuse_sk)
sk = reuse_sk;
return sk;
diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
index 208998694ae3..a350ee40141c 100644
--- a/net/ipv6/inet6_hashtables.c
+++ b/net/ipv6/inet6_hashtables.c
@@ -111,6 +111,10 @@ static inline int compute_score(struct sock *sk, struct net *net,
return score;
}
+INDIRECT_CALLABLE_DECLARE(u32 udp6_ehashfn(const struct net *,
+ const struct in6_addr *, const u16,
+ const struct in6_addr *, const __be16));
+
/**
* inet6_lookup_reuseport() - execute reuseport logic on AF_INET6 socket if necessary.
* @net: network namespace.
@@ -121,6 +125,7 @@ static inline int compute_score(struct sock *sk, struct net *net,
* @sport: source port.
* @daddr: destination address.
* @hnum: destination port in host byte order.
+ * @ehashfn: hash function used to generate the fallback hash.
*
* Return: NULL if sk doesn't have SO_REUSEPORT set, otherwise a pointer to
* the selected sock or an error.
@@ -130,13 +135,15 @@ struct sock *inet6_lookup_reuseport(struct net *net, struct sock *sk,
const struct in6_addr *saddr,
__be16 sport,
const struct in6_addr *daddr,
- unsigned short hnum)
+ unsigned short hnum,
+ inet6_ehashfn_t ehashfn)
{
struct sock *reuse_sk = NULL;
u32 phash;
if (sk->sk_reuseport) {
- phash = inet6_ehashfn(net, daddr, hnum, saddr, sport);
+ phash = INDIRECT_CALL_2(ehashfn, inet6_ehashfn, udp6_ehashfn,
+ net, daddr, hnum, saddr, sport);
reuse_sk = reuseport_select_sock(sk, phash, skb, doff);
}
return reuse_sk;
@@ -159,7 +166,7 @@ static struct sock *inet6_lhash2_lookup(struct net *net,
score = compute_score(sk, net, hnum, daddr, dif, sdif);
if (score > hiscore) {
result = inet6_lookup_reuseport(net, sk, skb, doff,
- saddr, sport, daddr, hnum);
+ saddr, sport, daddr, hnum, inet6_ehashfn);
if (result)
return result;
@@ -190,7 +197,8 @@ static inline struct sock *inet6_lookup_run_bpf(struct net *net,
if (no_reuseport || IS_ERR_OR_NULL(sk))
return sk;
- reuse_sk = inet6_lookup_reuseport(net, sk, skb, doff, saddr, sport, daddr, hnum);
+ reuse_sk = inet6_lookup_reuseport(net, sk, skb, doff,
+ saddr, sport, daddr, hnum, inet6_ehashfn);
if (reuse_sk)
sk = reuse_sk;
return sk;
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index e5a337e6b970..2af3a595f38a 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -70,11 +70,12 @@ int udpv6_init_sock(struct sock *sk)
return 0;
}
-static u32 udp6_ehashfn(const struct net *net,
- const struct in6_addr *laddr,
- const u16 lport,
- const struct in6_addr *faddr,
- const __be16 fport)
+INDIRECT_CALLABLE_SCOPE
+u32 udp6_ehashfn(const struct net *net,
+ const struct in6_addr *laddr,
+ const u16 lport,
+ const struct in6_addr *faddr,
+ const __be16 fport)
{
static u32 udp6_ehash_secret __read_mostly;
static u32 udp_ipv6_hash_secret __read_mostly;
@@ -159,24 +160,6 @@ static int compute_score(struct sock *sk, struct net *net,
return score;
}
-static struct sock *lookup_reuseport(struct net *net, struct sock *sk,
- struct sk_buff *skb,
- const struct in6_addr *saddr,
- __be16 sport,
- const struct in6_addr *daddr,
- unsigned int hnum)
-{
- struct sock *reuse_sk = NULL;
- u32 hash;
-
- if (sk->sk_reuseport && sk->sk_state != TCP_ESTABLISHED) {
- hash = udp6_ehashfn(net, daddr, hnum, saddr, sport);
- reuse_sk = reuseport_select_sock(sk, hash, skb,
- sizeof(struct udphdr));
- }
- return reuse_sk;
-}
-
/* called with rcu_read_lock() */
static struct sock *udp6_lib_lookup2(struct net *net,
const struct in6_addr *saddr, __be16 sport,
@@ -193,11 +176,14 @@ static struct sock *udp6_lib_lookup2(struct net *net,
score = compute_score(sk, net, saddr, sport,
daddr, hnum, dif, sdif);
if (score > badness) {
- result = lookup_reuseport(net, sk, skb,
- saddr, sport, daddr, hnum);
- /* Fall back to scoring if group has connections */
- if (result && !reuseport_has_conns(sk))
- return result;
+ if (sk->sk_state != TCP_ESTABLISHED) {
+ result = inet6_lookup_reuseport(net, sk, skb, sizeof(struct udphdr),
+ saddr, sport, daddr, hnum,
+ udp6_ehashfn);
+ /* Fall back to scoring if group has connections */
+ if (result && !reuseport_has_conns(sk))
+ return result;
+ }
result = result ? : sk;
badness = score;
@@ -225,7 +211,8 @@ static inline struct sock *udp6_lookup_run_bpf(struct net *net,
if (no_reuseport || IS_ERR_OR_NULL(sk))
return sk;
- reuse_sk = lookup_reuseport(net, sk, skb, saddr, sport, daddr, hnum);
+ reuse_sk = inet6_lookup_reuseport(net, sk, skb, sizeof(struct udphdr),
+ saddr, sport, daddr, hnum, udp6_ehashfn);
if (reuse_sk)
sk = reuse_sk;
return sk;
--
2.40.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH bpf-next v2 4/6] net: remove duplicate sk_lookup helpers
2023-06-13 10:14 [PATCH bpf-next v2 0/6] Add SO_REUSEPORT support for TC bpf_sk_assign Lorenz Bauer
` (2 preceding siblings ...)
2023-06-13 10:14 ` [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions Lorenz Bauer
@ 2023-06-13 10:14 ` Lorenz Bauer
2023-06-13 19:03 ` Kuniyuki Iwashima
2023-06-13 19:41 ` kernel test robot
2023-06-13 10:15 ` [PATCH bpf-next v2 5/6] bpf, net: Support SO_REUSEPORT sockets with bpf_sk_assign Lorenz Bauer
2023-06-13 10:15 ` [PATCH bpf-next v2 6/6] selftests/bpf: Test that SO_REUSEPORT can be used with sk_assign helper Lorenz Bauer
5 siblings, 2 replies; 22+ messages in thread
From: Lorenz Bauer @ 2023-06-13 10:14 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
David Ahern, Willem de Bruijn, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Joe Stringer, Mykola Lysenko, Shuah Khan,
Kuniyuki Iwashima
Cc: Hemanth Malla, netdev, linux-kernel, bpf, linux-kselftest, Lorenz Bauer
Now that inet[6]_lookup_reuseport are parameterised on the ehashfn
we can remove two sk_lookup helpers.
Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
---
include/net/inet6_hashtables.h | 9 +++++++++
include/net/inet_hashtables.h | 7 +++++++
net/ipv4/inet_hashtables.c | 26 +++++++++++++-------------
net/ipv4/udp.c | 32 +++++---------------------------
net/ipv6/inet6_hashtables.c | 30 +++++++++++++++---------------
net/ipv6/udp.c | 34 +++++-----------------------------
6 files changed, 54 insertions(+), 84 deletions(-)
diff --git a/include/net/inet6_hashtables.h b/include/net/inet6_hashtables.h
index 49d586454287..4d2a1a3c0be7 100644
--- a/include/net/inet6_hashtables.h
+++ b/include/net/inet6_hashtables.h
@@ -73,6 +73,15 @@ struct sock *inet6_lookup_listener(struct net *net,
const unsigned short hnum,
const int dif, const int sdif);
+struct sock *inet6_lookup_run_sk_lookup(struct net *net,
+ int protocol,
+ struct sk_buff *skb, int doff,
+ const struct in6_addr *saddr,
+ const __be16 sport,
+ const struct in6_addr *daddr,
+ const u16 hnum, const int dif,
+ inet6_ehashfn_t ehashfn);
+
static inline struct sock *__inet6_lookup(struct net *net,
struct inet_hashinfo *hashinfo,
struct sk_buff *skb, int doff,
diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index 51ab6a1a3601..aa02f1db1f86 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -393,6 +393,13 @@ struct sock *inet_lookup_reuseport(struct net *net, struct sock *sk,
__be32 daddr, unsigned short hnum,
inet_ehashfn_t ehashfn);
+struct sock *inet_lookup_run_sk_lookup(struct net *net,
+ int protocol,
+ struct sk_buff *skb, int doff,
+ __be32 saddr, __be16 sport,
+ __be32 daddr, u16 hnum, const int dif,
+ inet_ehashfn_t ehashfn);
+
static inline struct sock *
inet_lookup_established(struct net *net, struct inet_hashinfo *hashinfo,
const __be32 saddr, const __be16 sport,
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 1ec895fd9905..47f57b060e9e 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -404,25 +404,23 @@ static struct sock *inet_lhash2_lookup(struct net *net,
return result;
}
-static inline struct sock *inet_lookup_run_bpf(struct net *net,
- struct inet_hashinfo *hashinfo,
- struct sk_buff *skb, int doff,
- __be32 saddr, __be16 sport,
- __be32 daddr, u16 hnum, const int dif)
+struct sock *inet_lookup_run_sk_lookup(struct net *net,
+ int protocol,
+ struct sk_buff *skb, int doff,
+ __be32 saddr, __be16 sport,
+ __be32 daddr, u16 hnum, const int dif,
+ inet_ehashfn_t ehashfn)
{
struct sock *sk, *reuse_sk;
bool no_reuseport;
- if (hashinfo != net->ipv4.tcp_death_row.hashinfo)
- return NULL; /* only TCP is supported */
-
- no_reuseport = bpf_sk_lookup_run_v4(net, IPPROTO_TCP, saddr, sport,
+ no_reuseport = bpf_sk_lookup_run_v4(net, protocol, saddr, sport,
daddr, hnum, dif, &sk);
if (no_reuseport || IS_ERR_OR_NULL(sk))
return sk;
reuse_sk = inet_lookup_reuseport(net, sk, skb, doff, saddr, sport, daddr, hnum,
- inet_ehashfn);
+ ehashfn);
if (reuse_sk)
sk = reuse_sk;
return sk;
@@ -440,9 +438,11 @@ struct sock *__inet_lookup_listener(struct net *net,
unsigned int hash2;
/* Lookup redirect from BPF */
- if (static_branch_unlikely(&bpf_sk_lookup_enabled)) {
- result = inet_lookup_run_bpf(net, hashinfo, skb, doff,
- saddr, sport, daddr, hnum, dif);
+ if (static_branch_unlikely(&bpf_sk_lookup_enabled) &&
+ hashinfo == net->ipv4.tcp_death_row.hashinfo) {
+ result = inet_lookup_run_sk_lookup(net, IPPROTO_TCP, skb, doff,
+ saddr, sport, daddr, hnum, dif,
+ inet_ehashfn);
if (result)
goto done;
}
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 10468fe144d0..2500e92050a0 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -449,30 +449,6 @@ static struct sock *udp4_lib_lookup2(struct net *net,
return result;
}
-static struct sock *udp4_lookup_run_bpf(struct net *net,
- struct udp_table *udptable,
- struct sk_buff *skb,
- __be32 saddr, __be16 sport,
- __be32 daddr, u16 hnum, const int dif)
-{
- struct sock *sk, *reuse_sk;
- bool no_reuseport;
-
- if (udptable != net->ipv4.udp_table)
- return NULL; /* only UDP is supported */
-
- no_reuseport = bpf_sk_lookup_run_v4(net, IPPROTO_UDP, saddr, sport,
- daddr, hnum, dif, &sk);
- if (no_reuseport || IS_ERR_OR_NULL(sk))
- return sk;
-
- reuse_sk = inet_lookup_reuseport(net, sk, skb, sizeof(struct udphdr),
- saddr, sport, daddr, hnum, udp_ehashfn);
- if (reuse_sk)
- sk = reuse_sk;
- return sk;
-}
-
/* UDP is nearly always wildcards out the wazoo, it makes no sense to try
* harder than this. -DaveM
*/
@@ -497,9 +473,11 @@ struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr,
goto done;
/* Lookup redirect from BPF */
- if (static_branch_unlikely(&bpf_sk_lookup_enabled)) {
- sk = udp4_lookup_run_bpf(net, udptable, skb,
- saddr, sport, daddr, hnum, dif);
+ if (static_branch_unlikely(&bpf_sk_lookup_enabled) &&
+ udptable == net->ipv4.udp_table) {
+ sk = inet_lookup_run_sk_lookup(net, IPPROTO_UDP, skb, sizeof(struct udphdr),
+ saddr, sport, daddr, hnum, dif,
+ udp_ehashfn);
if (sk) {
result = sk;
goto done;
diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
index a350ee40141c..80bf97669fc9 100644
--- a/net/ipv6/inet6_hashtables.c
+++ b/net/ipv6/inet6_hashtables.c
@@ -178,27 +178,25 @@ static struct sock *inet6_lhash2_lookup(struct net *net,
return result;
}
-static inline struct sock *inet6_lookup_run_bpf(struct net *net,
- struct inet_hashinfo *hashinfo,
- struct sk_buff *skb, int doff,
- const struct in6_addr *saddr,
- const __be16 sport,
- const struct in6_addr *daddr,
- const u16 hnum, const int dif)
+struct sock *inet6_lookup_run_sk_lookup(struct net *net,
+ int protocol,
+ struct sk_buff *skb, int doff,
+ const struct in6_addr *saddr,
+ const __be16 sport,
+ const struct in6_addr *daddr,
+ const u16 hnum, const int dif,
+ inet6_ehashfn_t ehashfn)
{
struct sock *sk, *reuse_sk;
bool no_reuseport;
- if (hashinfo != net->ipv4.tcp_death_row.hashinfo)
- return NULL; /* only TCP is supported */
-
- no_reuseport = bpf_sk_lookup_run_v6(net, IPPROTO_TCP, saddr, sport,
+ no_reuseport = bpf_sk_lookup_run_v6(net, protocol, saddr, sport,
daddr, hnum, dif, &sk);
if (no_reuseport || IS_ERR_OR_NULL(sk))
return sk;
reuse_sk = inet6_lookup_reuseport(net, sk, skb, doff,
- saddr, sport, daddr, hnum, inet6_ehashfn);
+ saddr, sport, daddr, hnum, ehashfn);
if (reuse_sk)
sk = reuse_sk;
return sk;
@@ -216,9 +214,11 @@ struct sock *inet6_lookup_listener(struct net *net,
unsigned int hash2;
/* Lookup redirect from BPF */
- if (static_branch_unlikely(&bpf_sk_lookup_enabled)) {
- result = inet6_lookup_run_bpf(net, hashinfo, skb, doff,
- saddr, sport, daddr, hnum, dif);
+ if (static_branch_unlikely(&bpf_sk_lookup_enabled) &&
+ hashinfo == net->ipv4.tcp_death_row.hashinfo) {
+ result = inet6_lookup_run_sk_lookup(net, IPPROTO_TCP, skb, doff,
+ saddr, sport, daddr, hnum, dif,
+ inet6_ehashfn);
if (result)
goto done;
}
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 2af3a595f38a..961b7e61f02c 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -192,32 +192,6 @@ static struct sock *udp6_lib_lookup2(struct net *net,
return result;
}
-static inline struct sock *udp6_lookup_run_bpf(struct net *net,
- struct udp_table *udptable,
- struct sk_buff *skb,
- const struct in6_addr *saddr,
- __be16 sport,
- const struct in6_addr *daddr,
- u16 hnum, const int dif)
-{
- struct sock *sk, *reuse_sk;
- bool no_reuseport;
-
- if (udptable != net->ipv4.udp_table)
- return NULL; /* only UDP is supported */
-
- no_reuseport = bpf_sk_lookup_run_v6(net, IPPROTO_UDP, saddr, sport,
- daddr, hnum, dif, &sk);
- if (no_reuseport || IS_ERR_OR_NULL(sk))
- return sk;
-
- reuse_sk = inet6_lookup_reuseport(net, sk, skb, sizeof(struct udphdr),
- saddr, sport, daddr, hnum, udp6_ehashfn);
- if (reuse_sk)
- sk = reuse_sk;
- return sk;
-}
-
/* rcu_read_lock() must be held */
struct sock *__udp6_lib_lookup(struct net *net,
const struct in6_addr *saddr, __be16 sport,
@@ -242,9 +216,11 @@ struct sock *__udp6_lib_lookup(struct net *net,
goto done;
/* Lookup redirect from BPF */
- if (static_branch_unlikely(&bpf_sk_lookup_enabled)) {
- sk = udp6_lookup_run_bpf(net, udptable, skb,
- saddr, sport, daddr, hnum, dif);
+ if (static_branch_unlikely(&bpf_sk_lookup_enabled) &&
+ udptable == net->ipv4.udp_table) {
+ sk = inet6_lookup_run_sk_lookup(net, IPPROTO_UDP, skb, sizeof(struct udphdr),
+ saddr, sport, daddr, hnum, dif,
+ udp6_ehashfn);
if (sk) {
result = sk;
goto done;
--
2.40.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH bpf-next v2 5/6] bpf, net: Support SO_REUSEPORT sockets with bpf_sk_assign
2023-06-13 10:14 [PATCH bpf-next v2 0/6] Add SO_REUSEPORT support for TC bpf_sk_assign Lorenz Bauer
` (3 preceding siblings ...)
2023-06-13 10:14 ` [PATCH bpf-next v2 4/6] net: remove duplicate sk_lookup helpers Lorenz Bauer
@ 2023-06-13 10:15 ` Lorenz Bauer
2023-06-13 17:26 ` kernel test robot
2023-06-13 10:15 ` [PATCH bpf-next v2 6/6] selftests/bpf: Test that SO_REUSEPORT can be used with sk_assign helper Lorenz Bauer
5 siblings, 1 reply; 22+ messages in thread
From: Lorenz Bauer @ 2023-06-13 10:15 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
David Ahern, Willem de Bruijn, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Joe Stringer, Mykola Lysenko, Shuah Khan,
Kuniyuki Iwashima
Cc: Hemanth Malla, netdev, linux-kernel, bpf, linux-kselftest,
Lorenz Bauer, Joe Stringer
Currently the bpf_sk_assign helper in tc BPF context refuses SO_REUSEPORT
sockets. This means we can't use the helper to steer traffic to Envoy,
which configures SO_REUSEPORT on its sockets. In turn, we're blocked
from removing TPROXY from our setup.
The reason that bpf_sk_assign refuses such sockets is that the
bpf_sk_lookup helpers don't execute SK_REUSEPORT programs. Instead,
one of the reuseport sockets is selected by hash. This could cause
dispatch to the "wrong" socket:
sk = bpf_sk_lookup_tcp(...) // select SO_REUSEPORT by hash
bpf_sk_assign(skb, sk) // SK_REUSEPORT wasn't executed
Fixing this isn't as simple as invoking SK_REUSEPORT from the lookup
helpers unfortunately. In the tc context, L2 headers are at the start
of the skb, while SK_REUSEPORT expects L3 headers instead.
Instead, we execute the SK_REUSEPORT program when the assigned socket
is pulled out of the skb, further up the stack. This creates some
trickiness with regards to refcounting as bpf_sk_assign will put both
refcounted and RCU freed sockets in skb->sk. reuseport sockets are RCU
freed. We can infer that the sk_assigned socket is RCU freed if the
reuseport lookup succeeds, but convincing yourself of this fact isn't
straight forward. Therefore we defensively check refcounting on the
sk_assign sock even though it's probably not required in practice.
Fixes: 8e368dc72e86 ("bpf: Fix use of sk->sk_reuseport from sk_assign")
Fixes: cf7fbe660f2d ("bpf: Add socket assign support")
Co-developed-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
Cc: Joe Stringer <joe@cilium.io>
Link: https://lore.kernel.org/bpf/CACAyw98+qycmpQzKupquhkxbvWK4OFyDuuLMBNROnfWMZxUWeA@mail.gmail.com/
---
include/net/inet6_hashtables.h | 59 ++++++++++++++++++++++++++++++++++++++----
include/net/inet_hashtables.h | 52 +++++++++++++++++++++++++++++++++++--
include/net/sock.h | 7 +++--
include/uapi/linux/bpf.h | 3 ---
net/core/filter.c | 2 --
net/ipv4/udp.c | 8 ++++--
net/ipv6/udp.c | 10 ++++---
tools/include/uapi/linux/bpf.h | 3 ---
8 files changed, 122 insertions(+), 22 deletions(-)
diff --git a/include/net/inet6_hashtables.h b/include/net/inet6_hashtables.h
index 4d2a1a3c0be7..4d300af6ccb6 100644
--- a/include/net/inet6_hashtables.h
+++ b/include/net/inet6_hashtables.h
@@ -103,6 +103,49 @@ static inline struct sock *__inet6_lookup(struct net *net,
daddr, hnum, dif, sdif);
}
+static inline
+struct sock *inet6_steal_sock(struct net *net, struct sk_buff *skb, int doff,
+ const struct in6_addr *saddr, const __be16 sport,
+ const struct in6_addr *daddr, const __be16 dport,
+ bool *refcounted, inet6_ehashfn_t ehashfn)
+{
+ struct sock *sk, *reuse_sk;
+ bool prefetched;
+
+ sk = skb_steal_sock(skb, refcounted, &prefetched);
+ if (!sk)
+ return NULL;
+
+ if (!prefetched)
+ return sk;
+
+ if (sk->sk_protocol == IPPROTO_TCP) {
+ if (sk->sk_state != TCP_LISTEN)
+ return sk;
+ } else if (sk->sk_protocol == IPPROTO_UDP) {
+ if (sk->sk_state != TCP_CLOSE)
+ return sk;
+ } else {
+ return sk;
+ }
+
+ reuse_sk = inet6_lookup_reuseport(net, sk, skb, doff,
+ saddr, sport, daddr, ntohs(dport),
+ ehashfn);
+ if (!reuse_sk || reuse_sk == sk)
+ return sk;
+
+ /* We've chosen a new reuseport sock which is never refcounted.
+ * sk might be refcounted however, drop the reference if necessary.
+ */
+ if (*refcounted) {
+ sock_put(sk);
+ *refcounted = false;
+ }
+
+ return reuse_sk;
+}
+
static inline struct sock *__inet6_lookup_skb(struct inet_hashinfo *hashinfo,
struct sk_buff *skb, int doff,
const __be16 sport,
@@ -110,14 +153,20 @@ static inline struct sock *__inet6_lookup_skb(struct inet_hashinfo *hashinfo,
int iif, int sdif,
bool *refcounted)
{
- struct sock *sk = skb_steal_sock(skb, refcounted);
-
+ struct net *net = dev_net(skb_dst(skb)->dev);
+ const struct ipv6hdr *ip6h = ipv6_hdr(skb);
+ struct sock *sk;
+
+ sk = inet6_steal_sock(net, skb, doff, &ip6h->saddr, sport, &ip6h->daddr, dport,
+ refcounted, inet6_ehashfn);
+ if (IS_ERR(sk))
+ return NULL;
if (sk)
return sk;
- return __inet6_lookup(dev_net(skb_dst(skb)->dev), hashinfo, skb,
- doff, &ipv6_hdr(skb)->saddr, sport,
- &ipv6_hdr(skb)->daddr, ntohs(dport),
+ return __inet6_lookup(net, hashinfo, skb,
+ doff, &ip6h->saddr, sport,
+ &ip6h->daddr, ntohs(dport),
iif, sdif, refcounted);
}
diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index aa02f1db1f86..2c405d9df300 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -449,6 +449,49 @@ static inline struct sock *inet_lookup(struct net *net,
return sk;
}
+static inline
+struct sock *inet_steal_sock(struct net *net, struct sk_buff *skb, int doff,
+ const __be32 saddr, const __be16 sport,
+ const __be32 daddr, const __be16 dport,
+ bool *refcounted, inet_ehashfn_t ehashfn)
+{
+ struct sock *sk, *reuse_sk;
+ bool prefetched;
+
+ sk = skb_steal_sock(skb, refcounted, &prefetched);
+ if (!sk)
+ return NULL;
+
+ if (!prefetched)
+ return sk;
+
+ if (sk->sk_protocol == IPPROTO_TCP) {
+ if (sk->sk_state != TCP_LISTEN)
+ return sk;
+ } else if (sk->sk_protocol == IPPROTO_UDP) {
+ if (sk->sk_state != TCP_CLOSE)
+ return sk;
+ } else {
+ return sk;
+ }
+
+ reuse_sk = inet_lookup_reuseport(net, sk, skb, doff,
+ saddr, sport, daddr, ntohs(dport),
+ ehashfn);
+ if (!reuse_sk || reuse_sk == sk)
+ return sk;
+
+ /* We've chosen a new reuseport sock which is never refcounted.
+ * sk might be refcounted however, drop the reference if necessary.
+ */
+ if (*refcounted) {
+ sock_put(sk);
+ *refcounted = false;
+ }
+
+ return reuse_sk;
+}
+
static inline struct sock *__inet_lookup_skb(struct inet_hashinfo *hashinfo,
struct sk_buff *skb,
int doff,
@@ -457,13 +500,18 @@ static inline struct sock *__inet_lookup_skb(struct inet_hashinfo *hashinfo,
const int sdif,
bool *refcounted)
{
- struct sock *sk = skb_steal_sock(skb, refcounted);
+ struct net *net = dev_net(skb_dst(skb)->dev);
const struct iphdr *iph = ip_hdr(skb);
+ struct sock *sk;
+ sk = inet_steal_sock(net, skb, doff, iph->saddr, sport, iph->daddr, dport,
+ refcounted, inet_ehashfn);
+ if (IS_ERR(sk))
+ return NULL;
if (sk)
return sk;
- return __inet_lookup(dev_net(skb_dst(skb)->dev), hashinfo, skb,
+ return __inet_lookup(net, hashinfo, skb,
doff, iph->saddr, sport,
iph->daddr, dport, inet_iif(skb), sdif,
refcounted);
diff --git a/include/net/sock.h b/include/net/sock.h
index 656ea89f60ff..5645570c2a64 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2806,20 +2806,23 @@ sk_is_refcounted(struct sock *sk)
* skb_steal_sock - steal a socket from an sk_buff
* @skb: sk_buff to steal the socket from
* @refcounted: is set to true if the socket is reference-counted
+ * @prefetched: is set to true if the socket was assigned from bpf
*/
static inline struct sock *
-skb_steal_sock(struct sk_buff *skb, bool *refcounted)
+skb_steal_sock(struct sk_buff *skb, bool *refcounted, bool *prefetched)
{
if (skb->sk) {
struct sock *sk = skb->sk;
*refcounted = true;
- if (skb_sk_is_prefetched(skb))
+ *prefetched = skb_sk_is_prefetched(skb);
+ if (*prefetched)
*refcounted = sk_is_refcounted(sk);
skb->destructor = NULL;
skb->sk = NULL;
return sk;
}
+ *prefetched = false;
*refcounted = false;
return NULL;
}
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index a7b5e91dd768..d6fb6f43b0f3 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -4158,9 +4158,6 @@ union bpf_attr {
* **-EOPNOTSUPP** if the operation is not supported, for example
* a call from outside of TC ingress.
*
- * **-ESOCKTNOSUPPORT** if the socket type is not supported
- * (reuseport).
- *
* long bpf_sk_assign(struct bpf_sk_lookup *ctx, struct bpf_sock *sk, u64 flags)
* Description
* Helper is overloaded depending on BPF program type. This
diff --git a/net/core/filter.c b/net/core/filter.c
index 428df050d021..d4be0a1d754c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7278,8 +7278,6 @@ BPF_CALL_3(bpf_sk_assign, struct sk_buff *, skb, struct sock *, sk, u64, flags)
return -EOPNOTSUPP;
if (unlikely(dev_net(skb->dev) != sock_net(sk)))
return -ENETUNREACH;
- if (unlikely(sk_fullsock(sk) && sk->sk_reuseport))
- return -ESOCKTNOSUPPORT;
if (sk_is_refcounted(sk) &&
unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
return -ENOENT;
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 2500e92050a0..a3fa781432b9 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2373,7 +2373,11 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
if (udp4_csum_init(skb, uh, proto))
goto csum_error;
- sk = skb_steal_sock(skb, &refcounted);
+ sk = inet_steal_sock(net, skb, sizeof(struct udphdr), saddr, uh->source, daddr, uh->dest,
+ &refcounted, udp_ehashfn);
+ if (IS_ERR(sk))
+ goto no_sk;
+
if (sk) {
struct dst_entry *dst = skb_dst(skb);
int ret;
@@ -2394,7 +2398,7 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
sk = __udp4_lib_lookup_skb(skb, uh->source, uh->dest, udptable);
if (sk)
return udp_unicast_rcv_skb(sk, skb, uh);
-
+no_sk:
if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
goto drop;
nf_reset_ct(skb);
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 961b7e61f02c..0a90f34696ad 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -910,9 +910,9 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
enum skb_drop_reason reason = SKB_DROP_REASON_NOT_SPECIFIED;
const struct in6_addr *saddr, *daddr;
struct net *net = dev_net(skb->dev);
+ bool refcounted;
struct udphdr *uh;
struct sock *sk;
- bool refcounted;
u32 ulen = 0;
if (!pskb_may_pull(skb, sizeof(struct udphdr)))
@@ -949,7 +949,11 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
goto csum_error;
/* Check if the socket is already available, e.g. due to early demux */
- sk = skb_steal_sock(skb, &refcounted);
+ sk = inet6_steal_sock(net, skb, sizeof(struct udphdr), saddr, uh->source, daddr, uh->dest,
+ &refcounted, udp6_ehashfn);
+ if (IS_ERR(sk))
+ goto no_sk;
+
if (sk) {
struct dst_entry *dst = skb_dst(skb);
int ret;
@@ -983,7 +987,7 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
goto report_csum_error;
return udp6_unicast_rcv_skb(sk, skb, uh);
}
-
+no_sk:
reason = SKB_DROP_REASON_NO_SOCKET;
if (!uh->check)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a7b5e91dd768..d6fb6f43b0f3 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -4158,9 +4158,6 @@ union bpf_attr {
* **-EOPNOTSUPP** if the operation is not supported, for example
* a call from outside of TC ingress.
*
- * **-ESOCKTNOSUPPORT** if the socket type is not supported
- * (reuseport).
- *
* long bpf_sk_assign(struct bpf_sk_lookup *ctx, struct bpf_sock *sk, u64 flags)
* Description
* Helper is overloaded depending on BPF program type. This
--
2.40.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH bpf-next v2 6/6] selftests/bpf: Test that SO_REUSEPORT can be used with sk_assign helper
2023-06-13 10:14 [PATCH bpf-next v2 0/6] Add SO_REUSEPORT support for TC bpf_sk_assign Lorenz Bauer
` (4 preceding siblings ...)
2023-06-13 10:15 ` [PATCH bpf-next v2 5/6] bpf, net: Support SO_REUSEPORT sockets with bpf_sk_assign Lorenz Bauer
@ 2023-06-13 10:15 ` Lorenz Bauer
5 siblings, 0 replies; 22+ messages in thread
From: Lorenz Bauer @ 2023-06-13 10:15 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
David Ahern, Willem de Bruijn, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Joe Stringer, Mykola Lysenko, Shuah Khan,
Kuniyuki Iwashima
Cc: Hemanth Malla, netdev, linux-kernel, bpf, linux-kselftest,
Lorenz Bauer, Joe Stringer
From: Daniel Borkmann <daniel@iogearbox.net>
We use two programs to check that the new reuseport logic is executed
appropriately.
The first is a TC clsact program which bpf_sk_assigns
the skb to a UDP or TCP socket created by user space. Since the test
communicates via lo we see both directions of packets in the eBPF.
Traffic ingressing to the reuseport socket is identified by looking
at the destination port. For TCP, we additionally need to make sure
that we only assign the initial SYN packets towards our listening
socket. The network stack then creates a request socket which
transitions to ESTABLISHED after the 3WHS.
The second is a reuseport program which shares the fact that
it has been executed with user space. This tells us that the delayed
lookup mechanism is working.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Co-developed-by: Lorenz Bauer <lmb@isovalent.com>
Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
Cc: Joe Stringer <joe@cilium.io>
---
tools/testing/selftests/bpf/network_helpers.c | 3 +
.../selftests/bpf/prog_tests/assign_reuse.c | 197 +++++++++++++++++++++
.../selftests/bpf/progs/test_assign_reuse.c | 142 +++++++++++++++
3 files changed, 342 insertions(+)
diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
index a105c0cd008a..8a33bcea97de 100644
--- a/tools/testing/selftests/bpf/network_helpers.c
+++ b/tools/testing/selftests/bpf/network_helpers.c
@@ -423,6 +423,9 @@ struct nstoken *open_netns(const char *name)
void close_netns(struct nstoken *token)
{
+ if (!token)
+ return;
+
ASSERT_OK(setns(token->orig_netns_fd, CLONE_NEWNET), "setns");
close(token->orig_netns_fd);
free(token);
diff --git a/tools/testing/selftests/bpf/prog_tests/assign_reuse.c b/tools/testing/selftests/bpf/prog_tests/assign_reuse.c
new file mode 100644
index 000000000000..622f123410f4
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/assign_reuse.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Isovalent */
+#include <uapi/linux/if_link.h>
+#include <test_progs.h>
+
+#include <netinet/tcp.h>
+#include <netinet/udp.h>
+
+#include "network_helpers.h"
+#include "test_assign_reuse.skel.h"
+
+#define NS_TEST "assign_reuse"
+#define LOOPBACK 1
+#define PORT 4443
+
+static int attach_reuseport(int sock_fd, int prog_fd)
+{
+ return setsockopt(sock_fd, SOL_SOCKET, SO_ATTACH_REUSEPORT_EBPF,
+ &prog_fd, sizeof(prog_fd));
+}
+
+static __u64 cookie(int fd)
+{
+ __u64 cookie = 0;
+ socklen_t cookie_len = sizeof(cookie);
+ int ret;
+
+ ret = getsockopt(fd, SOL_SOCKET, SO_COOKIE, &cookie, &cookie_len);
+ ASSERT_OK(ret, "cookie");
+ ASSERT_GT(cookie, 0, "cookie_invalid");
+
+ return cookie;
+}
+
+static int echo_test_udp(int fd_sv)
+{
+ struct sockaddr_storage addr = {};
+ socklen_t len = sizeof(addr);
+ char buff[1] = {};
+ int fd_cl = -1, ret;
+
+ fd_cl = connect_to_fd(fd_sv, 100);
+ ASSERT_GT(fd_cl, 0, "create_client");
+ ASSERT_EQ(getsockname(fd_cl, (void *)&addr, &len), 0, "getsockname");
+
+ ASSERT_EQ(send(fd_cl, buff, sizeof(buff), 0), 1, "send_client");
+
+ ret = recv(fd_sv, buff, sizeof(buff), 0);
+ if (ret < 0)
+ return errno;
+
+ ASSERT_EQ(ret, 1, "recv_server");
+ ASSERT_EQ(sendto(fd_sv, buff, sizeof(buff), 0, (void *)&addr, len), 1, "send_server");
+ ASSERT_EQ(recv(fd_cl, buff, sizeof(buff), 0), 1, "recv_client");
+ close(fd_cl);
+ return 0;
+}
+
+static int echo_test_tcp(int fd_sv)
+{
+ char buff[1] = {};
+ int fd_cl = -1, fd_sv_cl = -1;
+
+ fd_cl = connect_to_fd(fd_sv, 100);
+ if (fd_cl < 0)
+ return errno;
+
+ fd_sv_cl = accept(fd_sv, NULL, NULL);
+ ASSERT_GE(fd_sv_cl, 0, "accept_fd");
+
+ ASSERT_EQ(send(fd_cl, buff, sizeof(buff), 0), 1, "send_client");
+ ASSERT_EQ(recv(fd_sv_cl, buff, sizeof(buff), 0), 1, "recv_server");
+ ASSERT_EQ(send(fd_sv_cl, buff, sizeof(buff), 0), 1, "send_server");
+ ASSERT_EQ(recv(fd_cl, buff, sizeof(buff), 0), 1, "recv_client");
+ close(fd_sv_cl);
+ close(fd_cl);
+ return 0;
+}
+
+void run_assign_reuse(int family, int sotype, const char *ip, __u16 port)
+{
+ DECLARE_LIBBPF_OPTS(bpf_tc_hook, tc_hook,
+ .ifindex = LOOPBACK,
+ .attach_point = BPF_TC_INGRESS,
+ );
+ DECLARE_LIBBPF_OPTS(bpf_tc_opts, tc_opts,
+ .handle = 1,
+ .priority = 1,
+ );
+ bool hook_created = false, tc_attached = false;
+ int ret, fd_tc, fd_accept, fd_drop, fd_map;
+ int *fd_sv = NULL;
+ __u64 fd_val;
+ struct test_assign_reuse *skel;
+ const int zero = 0;
+
+ skel = test_assign_reuse__open();
+ if (!ASSERT_OK_PTR(skel, "skel_open"))
+ goto cleanup;
+
+ skel->rodata->dest_port = port;
+
+ ret = test_assign_reuse__load(skel);
+ if (!ASSERT_OK(ret, "skel_load"))
+ goto cleanup;
+
+ ASSERT_EQ(skel->bss->sk_cookie_seen, 0, "cookie_init");
+
+ fd_tc = bpf_program__fd(skel->progs.tc_main);
+ fd_accept = bpf_program__fd(skel->progs.reuse_accept);
+ fd_drop = bpf_program__fd(skel->progs.reuse_drop);
+ fd_map = bpf_map__fd(skel->maps.sk_map);
+
+ fd_sv = start_reuseport_server(family, sotype, ip, port, 100, 1);
+ if (!ASSERT_NEQ(fd_sv, NULL, "start_reuseport_server"))
+ goto cleanup;
+
+ ret = attach_reuseport(*fd_sv, fd_drop);
+ if (!ASSERT_OK(ret, "attach_reuseport"))
+ goto cleanup;
+
+ fd_val = *fd_sv;
+ ret = bpf_map_update_elem(fd_map, &zero, &fd_val, BPF_NOEXIST);
+ if (!ASSERT_OK(ret, "bpf_sk_map"))
+ goto cleanup;
+
+ ret = bpf_tc_hook_create(&tc_hook);
+ if (ret == 0)
+ hook_created = true;
+ ret = ret == -EEXIST ? 0 : ret;
+ if (!ASSERT_OK(ret, "bpf_tc_hook_create"))
+ goto cleanup;
+
+ tc_opts.prog_fd = fd_tc;
+ ret = bpf_tc_attach(&tc_hook, &tc_opts);
+ if (!ASSERT_OK(ret, "bpf_tc_attach"))
+ goto cleanup;
+ tc_attached = true;
+
+ if (sotype == SOCK_STREAM)
+ ASSERT_EQ(echo_test_tcp(*fd_sv), ECONNREFUSED, "drop_tcp");
+ else
+ ASSERT_EQ(echo_test_udp(*fd_sv), EAGAIN, "drop_udp");
+ ASSERT_EQ(skel->bss->reuseport_executed, 1, "program executed once");
+
+ skel->bss->sk_cookie_seen = 0;
+ skel->bss->reuseport_executed = 0;
+ ASSERT_OK(attach_reuseport(*fd_sv, fd_accept), "attach_reuseport(accept)");
+
+ if (sotype == SOCK_STREAM)
+ ASSERT_EQ(echo_test_tcp(*fd_sv), 0, "echo_tcp");
+ else
+ ASSERT_EQ(echo_test_udp(*fd_sv), 0, "echo_udp");
+
+ ASSERT_EQ(skel->bss->sk_cookie_seen, cookie(*fd_sv),
+ "cookie_mismatch");
+ ASSERT_EQ(skel->bss->reuseport_executed, 1, "program executed once");
+cleanup:
+ if (tc_attached) {
+ tc_opts.flags = tc_opts.prog_fd = tc_opts.prog_id = 0;
+ ret = bpf_tc_detach(&tc_hook, &tc_opts);
+ ASSERT_OK(ret, "bpf_tc_detach");
+ }
+ if (hook_created) {
+ tc_hook.attach_point = BPF_TC_INGRESS | BPF_TC_EGRESS;
+ bpf_tc_hook_destroy(&tc_hook);
+ }
+ test_assign_reuse__destroy(skel);
+ free_fds(fd_sv, 1);
+}
+
+void test_assign_reuse(void)
+{
+ struct nstoken *tok = NULL;
+
+ SYS(out, "ip netns add %s", NS_TEST);
+ SYS(cleanup, "ip -net %s link set dev lo up", NS_TEST);
+
+ tok = open_netns(NS_TEST);
+ if (!ASSERT_OK_PTR(tok, "netns token"))
+ return;
+
+ if (test__start_subtest("tcpv4"))
+ run_assign_reuse(AF_INET, SOCK_STREAM, "127.0.0.1", PORT);
+ if (test__start_subtest("tcpv6"))
+ run_assign_reuse(AF_INET6, SOCK_STREAM, "::1", PORT);
+ if (test__start_subtest("udpv4"))
+ run_assign_reuse(AF_INET, SOCK_DGRAM, "127.0.0.1", PORT);
+ if (test__start_subtest("udpv6"))
+ run_assign_reuse(AF_INET6, SOCK_DGRAM, "::1", PORT);
+
+cleanup:
+ close_netns(tok);
+ SYS_NOFAIL("ip netns delete %s", NS_TEST);
+out:
+ return;
+}
diff --git a/tools/testing/selftests/bpf/progs/test_assign_reuse.c b/tools/testing/selftests/bpf/progs/test_assign_reuse.c
new file mode 100644
index 000000000000..4f2e2321ea06
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_assign_reuse.c
@@ -0,0 +1,142 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Isovalent */
+#include <stdbool.h>
+#include <linux/bpf.h>
+#include <linux/if_ether.h>
+#include <linux/in.h>
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <linux/tcp.h>
+#include <linux/udp.h>
+#include <bpf/bpf_endian.h>
+#include <bpf/bpf_helpers.h>
+#include <linux/pkt_cls.h>
+
+char LICENSE[] SEC("license") = "GPL";
+
+__u64 sk_cookie_seen;
+__u64 reuseport_executed;
+union {
+ struct tcphdr tcp;
+ struct udphdr udp;
+} headers;
+
+const volatile __u16 dest_port;
+
+struct {
+ __uint(type, BPF_MAP_TYPE_SOCKMAP);
+ __uint(max_entries, 1);
+ __type(key, __u32);
+ __type(value, __u64);
+} sk_map SEC(".maps");
+
+SEC("sk_reuseport")
+int reuse_accept(struct sk_reuseport_md *ctx)
+{
+ reuseport_executed++;
+
+ if (ctx->ip_protocol == IPPROTO_TCP) {
+ if (ctx->data + sizeof(headers.tcp) > ctx->data_end)
+ return SK_DROP;
+
+ if (__builtin_memcmp(&headers.tcp, ctx->data, sizeof(headers.tcp)) != 0)
+ return SK_DROP;
+ } else if (ctx->ip_protocol == IPPROTO_UDP) {
+ if (ctx->data + sizeof(headers.udp) > ctx->data_end)
+ return SK_DROP;
+
+ if (__builtin_memcmp(&headers.udp, ctx->data, sizeof(headers.udp)) != 0)
+ return SK_DROP;
+ } else {
+ return SK_DROP;
+ }
+
+ sk_cookie_seen = bpf_get_socket_cookie(ctx->sk);
+ return SK_PASS;
+}
+
+SEC("sk_reuseport")
+int reuse_drop(struct sk_reuseport_md *ctx)
+{
+ reuseport_executed++;
+ sk_cookie_seen = 0;
+ return SK_DROP;
+}
+
+static int
+assign_sk(struct __sk_buff *skb)
+{
+ int zero = 0, ret = 0;
+ struct bpf_sock *sk;
+
+ sk = bpf_map_lookup_elem(&sk_map, &zero);
+ if (!sk)
+ return TC_ACT_SHOT;
+ ret = bpf_sk_assign(skb, sk, 0);
+ bpf_sk_release(sk);
+ return ret ? TC_ACT_SHOT : TC_ACT_OK;
+}
+
+static bool
+maybe_assign_tcp(struct __sk_buff *skb, struct tcphdr *th)
+{
+ if (th + 1 > (void *)(long)(skb->data_end))
+ return TC_ACT_SHOT;
+
+ if (!th->syn || th->ack || th->dest != bpf_htons(dest_port))
+ return TC_ACT_OK;
+
+ __builtin_memcpy(&headers.tcp, th, sizeof(headers.tcp));
+ return assign_sk(skb);
+}
+
+static bool
+maybe_assign_udp(struct __sk_buff *skb, struct udphdr *uh)
+{
+ if (uh + 1 > (void *)(long)(skb->data_end))
+ return TC_ACT_SHOT;
+
+ if (uh->dest != bpf_htons(dest_port))
+ return TC_ACT_OK;
+
+ __builtin_memcpy(&headers.udp, uh, sizeof(headers.udp));
+ return assign_sk(skb);
+}
+
+SEC("tc")
+int tc_main(struct __sk_buff *skb)
+{
+ void *data_end = (void *)(long)skb->data_end;
+ void *data = (void *)(long)skb->data;
+ struct ethhdr *eth;
+
+ eth = (struct ethhdr *)(data);
+ if (eth + 1 > data_end)
+ return TC_ACT_SHOT;
+
+ if (eth->h_proto == bpf_htons(ETH_P_IP)) {
+ struct iphdr *iph = (struct iphdr *)(data + sizeof(*eth));
+
+ if (iph + 1 > data_end)
+ return TC_ACT_SHOT;
+
+ if (iph->protocol == IPPROTO_TCP)
+ return maybe_assign_tcp(skb, (struct tcphdr *)(iph + 1));
+ else if (iph->protocol == IPPROTO_UDP)
+ return maybe_assign_udp(skb, (struct udphdr *)(iph + 1));
+ else
+ return TC_ACT_SHOT;
+ } else {
+ struct ipv6hdr *ip6h = (struct ipv6hdr *)(data + sizeof(*eth));
+
+ if (ip6h + 1 > data_end)
+ return TC_ACT_SHOT;
+
+ if (ip6h->nexthdr == IPPROTO_TCP)
+ return maybe_assign_tcp(skb, (struct tcphdr *)(ip6h + 1));
+ else if (ip6h->nexthdr == IPPROTO_UDP)
+ return maybe_assign_udp(skb, (struct udphdr *)(ip6h + 1));
+ else
+ return TC_ACT_SHOT;
+ }
+}
--
2.40.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions
2023-06-13 10:14 ` [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions Lorenz Bauer
@ 2023-06-13 15:32 ` Simon Horman
2023-06-14 15:42 ` Lorenz Bauer
2023-06-13 17:26 ` kernel test robot
2023-06-13 18:56 ` Kuniyuki Iwashima
2 siblings, 1 reply; 22+ messages in thread
From: Simon Horman @ 2023-06-13 15:32 UTC (permalink / raw)
To: Lorenz Bauer
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
David Ahern, Willem de Bruijn, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Joe Stringer, Mykola Lysenko, Shuah Khan,
Kuniyuki Iwashima, Hemanth Malla, netdev, linux-kernel, bpf,
linux-kselftest
On Tue, Jun 13, 2023 at 11:14:58AM +0100, Lorenz Bauer wrote:
...
> @@ -332,6 +332,10 @@ static inline int compute_score(struct sock *sk, struct net *net,
> return score;
> }
>
> +INDIRECT_CALLABLE_DECLARE(u32 udp_ehashfn(const struct net *,
> + const __be32, const __u16,
> + const __be32, const __be16));
> +
Hi Lorenz,
Would this be better placed in a header file?
GCC complains that in udp.c this function is neither static nor
has a prototype.
Liksewise for udp6_ehashfn()
...
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions
2023-06-13 10:14 ` [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions Lorenz Bauer
2023-06-13 15:32 ` Simon Horman
@ 2023-06-13 17:26 ` kernel test robot
2023-06-13 18:56 ` Kuniyuki Iwashima
2 siblings, 0 replies; 22+ messages in thread
From: kernel test robot @ 2023-06-13 17:26 UTC (permalink / raw)
To: Lorenz Bauer, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, David Ahern, Willem de Bruijn, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Joe Stringer, Mykola Lysenko, Shuah Khan,
Kuniyuki Iwashima
Cc: oe-kbuild-all, netdev, Hemanth Malla, linux-kernel, bpf,
linux-kselftest, Lorenz Bauer
Hi Lorenz,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 25085b4e9251c77758964a8e8651338972353642]
url: https://github.com/intel-lab-lkp/linux/commits/Lorenz-Bauer/net-export-inet_lookup_reuseport-and-inet6_lookup_reuseport/20230613-181619
base: 25085b4e9251c77758964a8e8651338972353642
patch link: https://lore.kernel.org/r/20230613-so-reuseport-v2-3-b7c69a342613%40isovalent.com
patch subject: [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions
config: i386-defconfig (https://download.01.org/0day-ci/archive/20230614/202306140138.DnwjedJ1-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build):
git checkout 25085b4e9251c77758964a8e8651338972353642
b4 shazam https://lore.kernel.org/r/20230613-so-reuseport-v2-3-b7c69a342613@isovalent.com
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 olddefconfig
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash net/ipv4/ net/ipv6/
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306140138.DnwjedJ1-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> net/ipv4/udp.c:409:5: warning: no previous prototype for 'udp_ehashfn' [-Wmissing-prototypes]
409 | u32 udp_ehashfn(const struct net *net, const __be32 laddr, const __u16 lport,
| ^~~~~~~~~~~
--
>> net/ipv6/udp.c:74:5: warning: no previous prototype for 'udp6_ehashfn' [-Wmissing-prototypes]
74 | u32 udp6_ehashfn(const struct net *net,
| ^~~~~~~~~~~~
vim +/udp_ehashfn +409 net/ipv4/udp.c
407
408 INDIRECT_CALLABLE_SCOPE
> 409 u32 udp_ehashfn(const struct net *net, const __be32 laddr, const __u16 lport,
410 const __be32 faddr, const __be16 fport)
411 {
412 static u32 udp_ehash_secret __read_mostly;
413
414 net_get_random_once(&udp_ehash_secret, sizeof(udp_ehash_secret));
415
416 return __inet_ehashfn(laddr, lport, faddr, fport,
417 udp_ehash_secret + net_hash_mix(net));
418 }
419
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 5/6] bpf, net: Support SO_REUSEPORT sockets with bpf_sk_assign
2023-06-13 10:15 ` [PATCH bpf-next v2 5/6] bpf, net: Support SO_REUSEPORT sockets with bpf_sk_assign Lorenz Bauer
@ 2023-06-13 17:26 ` kernel test robot
0 siblings, 0 replies; 22+ messages in thread
From: kernel test robot @ 2023-06-13 17:26 UTC (permalink / raw)
To: Lorenz Bauer, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, David Ahern, Willem de Bruijn, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Joe Stringer, Mykola Lysenko, Shuah Khan,
Kuniyuki Iwashima
Cc: llvm, oe-kbuild-all, netdev, Hemanth Malla, linux-kernel, bpf,
linux-kselftest, Lorenz Bauer
Hi Lorenz,
kernel test robot noticed the following build errors:
[auto build test ERROR on 25085b4e9251c77758964a8e8651338972353642]
url: https://github.com/intel-lab-lkp/linux/commits/Lorenz-Bauer/net-export-inet_lookup_reuseport-and-inet6_lookup_reuseport/20230613-181619
base: 25085b4e9251c77758964a8e8651338972353642
patch link: https://lore.kernel.org/r/20230613-so-reuseport-v2-5-b7c69a342613%40isovalent.com
patch subject: [PATCH bpf-next v2 5/6] bpf, net: Support SO_REUSEPORT sockets with bpf_sk_assign
config: arm-randconfig-r016-20230612 (https://download.01.org/0day-ci/archive/20230614/202306140012.hLncDFR5-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
git checkout 25085b4e9251c77758964a8e8651338972353642
b4 shazam https://lore.kernel.org/r/20230613-so-reuseport-v2-5-b7c69a342613@isovalent.com
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=arm olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306140012.hLncDFR5-lkp@intel.com/
All errors (new ones prefixed by >>, old ones prefixed by <<):
WARNING: modpost: lib/test_user_copy.o: section mismatch in reference: (unknown) (section: .text.fixup) -> .Ltmp1 (section: .init.text)
WARNING: modpost: lib/test_user_copy.o: section mismatch in reference: (unknown) (section: .text.fixup) -> .Ltmp4 (section: .init.text)
WARNING: modpost: lib/test_user_copy.o: section mismatch in reference: (unknown) (section: .text.fixup) -> .Ltmp7 (section: .init.text)
WARNING: modpost: lib/test_user_copy.o: section mismatch in reference: (unknown) (section: .text.fixup) -> .Ltmp10 (section: .init.text)
WARNING: modpost: lib/test_user_copy.o: section mismatch in reference: (unknown) (section: .text.fixup) -> .Ltmp13 (section: .init.text)
WARNING: modpost: lib/test_user_copy.o: section mismatch in reference: (unknown) (section: .text.fixup) -> .Ltmp16 (section: .init.text)
WARNING: modpost: lib/test_user_copy.o: section mismatch in reference: (unknown) (section: .text.fixup) -> .Ltmp19 (section: .init.text)
WARNING: modpost: lib/test_user_copy.o: section mismatch in reference: (unknown) (section: .text.fixup) -> .Ltmp22 (section: .init.text)
WARNING: modpost: lib/test_user_copy.o: section mismatch in reference: (unknown) (section: .text.fixup) -> .Ltmp25 (section: .init.text)
WARNING: modpost: lib/test_user_copy.o: section mismatch in reference: (unknown) (section: .text.fixup) -> .Ltmp28 (section: .init.text)
WARNING: modpost: lib/test_user_copy.o: section mismatch in reference: (unknown) (section: .text.fixup) -> .Ltmp31 (section: .init.text)
WARNING: modpost: lib/test_user_copy.o: section mismatch in reference: (unknown) (section: .text.fixup) -> .Ltmp34 (section: .init.text)
>> ERROR: modpost: "inet_ehashfn" [net/dccp/dccp_ipv4.ko] undefined!
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions
2023-06-13 10:14 ` [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions Lorenz Bauer
2023-06-13 15:32 ` Simon Horman
2023-06-13 17:26 ` kernel test robot
@ 2023-06-13 18:56 ` Kuniyuki Iwashima
2023-06-14 15:25 ` Lorenz Bauer
2023-06-20 14:26 ` Lorenz Bauer
2 siblings, 2 replies; 22+ messages in thread
From: Kuniyuki Iwashima @ 2023-06-13 18:56 UTC (permalink / raw)
To: lmb
Cc: andrii, ast, bpf, daniel, davem, dsahern, edumazet, haoluo,
hemanthmalla, joe, john.fastabend, jolsa, kpsingh, kuba, kuniyu,
linux-kernel, linux-kselftest, martin.lau, mykolal, netdev,
pabeni, sdf, shuah, song, willemdebruijn.kernel, yhs
From: Lorenz Bauer <lmb@isovalent.com>
Date: Tue, 13 Jun 2023 11:14:58 +0100
> There are currently four copies of reuseport_lookup: one each for
> (TCP, UDP)x(IPv4, IPv6). This forces us to duplicate all callers of
> those functions as well. This is already the case for sk_lookup
> helpers (inet,inet6,udp4,udp6)_lookup_run_bpf.
>
> The only difference between the reuseport_lookup helpers is calling
> a different hash function. Cut down the number of reuseport_lookup
> functions to one per IP version by using the INDIRECT_CALL
> infrastructure.
>
> Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
> ---
> include/net/inet6_hashtables.h | 11 ++++++++++-
> include/net/inet_hashtables.h | 15 +++++++++-----
> net/ipv4/inet_hashtables.c | 22 ++++++++++++++-------
> net/ipv4/udp.c | 37 +++++++++++-----------------------
> net/ipv6/inet6_hashtables.c | 16 +++++++++++----
> net/ipv6/udp.c | 45 +++++++++++++++---------------------------
> 6 files changed, 75 insertions(+), 71 deletions(-)
>
> diff --git a/include/net/inet6_hashtables.h b/include/net/inet6_hashtables.h
> index 032ddab48f8f..49d586454287 100644
> --- a/include/net/inet6_hashtables.h
> +++ b/include/net/inet6_hashtables.h
> @@ -48,12 +48,21 @@ struct sock *__inet6_lookup_established(struct net *net,
> const u16 hnum, const int dif,
> const int sdif);
>
> +typedef u32 (*inet6_ehashfn_t)(const struct net *net,
> + const struct in6_addr *laddr, const u16 lport,
> + const struct in6_addr *faddr, const __be16 fport);
> +
> +u32 inet6_ehashfn(const struct net *net,
> + const struct in6_addr *laddr, const u16 lport,
> + const struct in6_addr *faddr, const __be16 fport);
> +
> struct sock *inet6_lookup_reuseport(struct net *net, struct sock *sk,
> struct sk_buff *skb, int doff,
> const struct in6_addr *saddr,
> __be16 sport,
> const struct in6_addr *daddr,
> - unsigned short hnum);
> + unsigned short hnum,
> + inet6_ehashfn_t ehashfn);
>
> struct sock *inet6_lookup_listener(struct net *net,
> struct inet_hashinfo *hashinfo,
> diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
> index 8734f3488f5d..51ab6a1a3601 100644
> --- a/include/net/inet_hashtables.h
> +++ b/include/net/inet_hashtables.h
> @@ -379,10 +379,19 @@ struct sock *__inet_lookup_established(struct net *net,
> const __be32 daddr, const u16 hnum,
> const int dif, const int sdif);
>
> +typedef u32 (*inet_ehashfn_t)(const struct net *net,
> + const __be32 laddr, const __u16 lport,
> + const __be32 faddr, const __be16 fport);
> +
> +u32 inet_ehashfn(const struct net *net,
> + const __be32 laddr, const __u16 lport,
> + const __be32 faddr, const __be16 fport);
> +
> struct sock *inet_lookup_reuseport(struct net *net, struct sock *sk,
> struct sk_buff *skb, int doff,
> __be32 saddr, __be16 sport,
> - __be32 daddr, unsigned short hnum);
> + __be32 daddr, unsigned short hnum,
> + inet_ehashfn_t ehashfn);
>
> static inline struct sock *
> inet_lookup_established(struct net *net, struct inet_hashinfo *hashinfo,
> @@ -453,10 +462,6 @@ static inline struct sock *__inet_lookup_skb(struct inet_hashinfo *hashinfo,
> refcounted);
> }
>
> -u32 inet6_ehashfn(const struct net *net,
> - const struct in6_addr *laddr, const u16 lport,
> - const struct in6_addr *faddr, const __be16 fport);
> -
> static inline void sk_daddr_set(struct sock *sk, __be32 addr)
> {
> sk->sk_daddr = addr; /* alias of inet_daddr */
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index 91f9210d4e83..1ec895fd9905 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -28,9 +28,9 @@
> #include <net/tcp.h>
> #include <net/sock_reuseport.h>
>
> -static u32 inet_ehashfn(const struct net *net, const __be32 laddr,
> - const __u16 lport, const __be32 faddr,
> - const __be16 fport)
> +u32 inet_ehashfn(const struct net *net, const __be32 laddr,
> + const __u16 lport, const __be32 faddr,
> + const __be16 fport)
> {
> static u32 inet_ehash_secret __read_mostly;
>
> @@ -332,6 +332,10 @@ static inline int compute_score(struct sock *sk, struct net *net,
> return score;
> }
>
> +INDIRECT_CALLABLE_DECLARE(u32 udp_ehashfn(const struct net *,
> + const __be32, const __u16,
> + const __be32, const __be16));
> +
> /**
> * inet_lookup_reuseport() - execute reuseport logic on AF_INET socket if necessary.
> * @net: network namespace.
> @@ -342,6 +346,7 @@ static inline int compute_score(struct sock *sk, struct net *net,
> * @sport: source port.
> * @daddr: destination address.
> * @hnum: destination port in host byte order.
> + * @ehashfn: hash function used to generate the fallback hash.
> *
> * Return: NULL if sk doesn't have SO_REUSEPORT set, otherwise a pointer to
> * the selected sock or an error.
> @@ -349,13 +354,15 @@ static inline int compute_score(struct sock *sk, struct net *net,
> struct sock *inet_lookup_reuseport(struct net *net, struct sock *sk,
> struct sk_buff *skb, int doff,
> __be32 saddr, __be16 sport,
> - __be32 daddr, unsigned short hnum)
> + __be32 daddr, unsigned short hnum,
> + inet_ehashfn_t ehashfn)
> {
> struct sock *reuse_sk = NULL;
> u32 phash;
>
> if (sk->sk_reuseport) {
> - phash = inet_ehashfn(net, daddr, hnum, saddr, sport);
> + phash = INDIRECT_CALL_2(ehashfn, inet_ehashfn, udp_ehashfn,
> + net, daddr, hnum, saddr, sport);
> reuse_sk = reuseport_select_sock(sk, phash, skb, doff);
> }
> return reuse_sk;
> @@ -385,7 +392,7 @@ static struct sock *inet_lhash2_lookup(struct net *net,
> score = compute_score(sk, net, hnum, daddr, dif, sdif);
> if (score > hiscore) {
> result = inet_lookup_reuseport(net, sk, skb, doff,
> - saddr, sport, daddr, hnum);
> + saddr, sport, daddr, hnum, inet_ehashfn);
> if (result)
> return result;
>
> @@ -414,7 +421,8 @@ static inline struct sock *inet_lookup_run_bpf(struct net *net,
> if (no_reuseport || IS_ERR_OR_NULL(sk))
> return sk;
>
> - reuse_sk = inet_lookup_reuseport(net, sk, skb, doff, saddr, sport, daddr, hnum);
> + reuse_sk = inet_lookup_reuseport(net, sk, skb, doff, saddr, sport, daddr, hnum,
> + inet_ehashfn);
> if (reuse_sk)
> sk = reuse_sk;
> return sk;
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index fd3dae081f3a..10468fe144d0 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -405,9 +405,9 @@ static int compute_score(struct sock *sk, struct net *net,
> return score;
> }
>
> -static u32 udp_ehashfn(const struct net *net, const __be32 laddr,
> - const __u16 lport, const __be32 faddr,
> - const __be16 fport)
> +INDIRECT_CALLABLE_SCOPE
> +u32 udp_ehashfn(const struct net *net, const __be32 laddr, const __u16 lport,
> + const __be32 faddr, const __be16 fport)
> {
> static u32 udp_ehash_secret __read_mostly;
>
> @@ -417,22 +417,6 @@ static u32 udp_ehashfn(const struct net *net, const __be32 laddr,
> udp_ehash_secret + net_hash_mix(net));
> }
>
> -static struct sock *lookup_reuseport(struct net *net, struct sock *sk,
> - struct sk_buff *skb,
> - __be32 saddr, __be16 sport,
> - __be32 daddr, unsigned short hnum)
> -{
> - struct sock *reuse_sk = NULL;
> - u32 hash;
> -
> - if (sk->sk_reuseport && sk->sk_state != TCP_ESTABLISHED) {
> - hash = udp_ehashfn(net, daddr, hnum, saddr, sport);
> - reuse_sk = reuseport_select_sock(sk, hash, skb,
> - sizeof(struct udphdr));
> - }
> - return reuse_sk;
> -}
> -
> /* called with rcu_read_lock() */
> static struct sock *udp4_lib_lookup2(struct net *net,
> __be32 saddr, __be16 sport,
> @@ -450,11 +434,13 @@ static struct sock *udp4_lib_lookup2(struct net *net,
> score = compute_score(sk, net, saddr, sport,
> daddr, hnum, dif, sdif);
> if (score > badness) {
> - result = lookup_reuseport(net, sk, skb,
> - saddr, sport, daddr, hnum);
> - /* Fall back to scoring if group has connections */
> - if (result && !reuseport_has_conns(sk))
> - return result;
> + if (sk->sk_state != TCP_ESTABLISHED) {
> + result = inet_lookup_reuseport(net, sk, skb, sizeof(struct udphdr),
> + saddr, sport, daddr, hnum, udp_ehashfn);
> + /* Fall back to scoring if group has connections */
> + if (result && !reuseport_has_conns(sk))
> + return result;
result = result ? : sk;
> + }
else {
result = sk;
}
The assignment to result below is buggy. Let's say SO_REUSEPROT group
have TCP_CLOSE and TCP_ESTABLISHED sockets.
1. Find TCP_CLOSE sk and do SO_REUSEPORT lookup
2. result is not NULL, but the group has TCP_ESTABLISHED sk
3. result = result
4. Find TCP_ESTABLISHED sk, which has a higher score
5. result = result (TCP_CLOSE) <-- should be sk.
Same for v6 function.
>
> result = result ? : sk;
> badness = score;
> @@ -480,7 +466,8 @@ static struct sock *udp4_lookup_run_bpf(struct net *net,
> if (no_reuseport || IS_ERR_OR_NULL(sk))
> return sk;
>
> - reuse_sk = lookup_reuseport(net, sk, skb, saddr, sport, daddr, hnum);
> + reuse_sk = inet_lookup_reuseport(net, sk, skb, sizeof(struct udphdr),
> + saddr, sport, daddr, hnum, udp_ehashfn);
> if (reuse_sk)
> sk = reuse_sk;
> return sk;
> diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
> index 208998694ae3..a350ee40141c 100644
> --- a/net/ipv6/inet6_hashtables.c
> +++ b/net/ipv6/inet6_hashtables.c
> @@ -111,6 +111,10 @@ static inline int compute_score(struct sock *sk, struct net *net,
> return score;
> }
>
> +INDIRECT_CALLABLE_DECLARE(u32 udp6_ehashfn(const struct net *,
> + const struct in6_addr *, const u16,
> + const struct in6_addr *, const __be16));
> +
> /**
> * inet6_lookup_reuseport() - execute reuseport logic on AF_INET6 socket if necessary.
> * @net: network namespace.
> @@ -121,6 +125,7 @@ static inline int compute_score(struct sock *sk, struct net *net,
> * @sport: source port.
> * @daddr: destination address.
> * @hnum: destination port in host byte order.
> + * @ehashfn: hash function used to generate the fallback hash.
> *
> * Return: NULL if sk doesn't have SO_REUSEPORT set, otherwise a pointer to
> * the selected sock or an error.
> @@ -130,13 +135,15 @@ struct sock *inet6_lookup_reuseport(struct net *net, struct sock *sk,
> const struct in6_addr *saddr,
> __be16 sport,
> const struct in6_addr *daddr,
> - unsigned short hnum)
> + unsigned short hnum,
> + inet6_ehashfn_t ehashfn)
> {
> struct sock *reuse_sk = NULL;
> u32 phash;
>
> if (sk->sk_reuseport) {
> - phash = inet6_ehashfn(net, daddr, hnum, saddr, sport);
> + phash = INDIRECT_CALL_2(ehashfn, inet6_ehashfn, udp6_ehashfn,
> + net, daddr, hnum, saddr, sport);
> reuse_sk = reuseport_select_sock(sk, phash, skb, doff);
> }
> return reuse_sk;
> @@ -159,7 +166,7 @@ static struct sock *inet6_lhash2_lookup(struct net *net,
> score = compute_score(sk, net, hnum, daddr, dif, sdif);
> if (score > hiscore) {
> result = inet6_lookup_reuseport(net, sk, skb, doff,
> - saddr, sport, daddr, hnum);
> + saddr, sport, daddr, hnum, inet6_ehashfn);
> if (result)
> return result;
>
> @@ -190,7 +197,8 @@ static inline struct sock *inet6_lookup_run_bpf(struct net *net,
> if (no_reuseport || IS_ERR_OR_NULL(sk))
> return sk;
>
> - reuse_sk = inet6_lookup_reuseport(net, sk, skb, doff, saddr, sport, daddr, hnum);
> + reuse_sk = inet6_lookup_reuseport(net, sk, skb, doff,
> + saddr, sport, daddr, hnum, inet6_ehashfn);
> if (reuse_sk)
> sk = reuse_sk;
> return sk;
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index e5a337e6b970..2af3a595f38a 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -70,11 +70,12 @@ int udpv6_init_sock(struct sock *sk)
> return 0;
> }
>
> -static u32 udp6_ehashfn(const struct net *net,
> - const struct in6_addr *laddr,
> - const u16 lport,
> - const struct in6_addr *faddr,
> - const __be16 fport)
> +INDIRECT_CALLABLE_SCOPE
> +u32 udp6_ehashfn(const struct net *net,
> + const struct in6_addr *laddr,
> + const u16 lport,
> + const struct in6_addr *faddr,
> + const __be16 fport)
> {
> static u32 udp6_ehash_secret __read_mostly;
> static u32 udp_ipv6_hash_secret __read_mostly;
> @@ -159,24 +160,6 @@ static int compute_score(struct sock *sk, struct net *net,
> return score;
> }
>
> -static struct sock *lookup_reuseport(struct net *net, struct sock *sk,
> - struct sk_buff *skb,
> - const struct in6_addr *saddr,
> - __be16 sport,
> - const struct in6_addr *daddr,
> - unsigned int hnum)
> -{
> - struct sock *reuse_sk = NULL;
> - u32 hash;
> -
> - if (sk->sk_reuseport && sk->sk_state != TCP_ESTABLISHED) {
> - hash = udp6_ehashfn(net, daddr, hnum, saddr, sport);
> - reuse_sk = reuseport_select_sock(sk, hash, skb,
> - sizeof(struct udphdr));
> - }
> - return reuse_sk;
> -}
> -
> /* called with rcu_read_lock() */
> static struct sock *udp6_lib_lookup2(struct net *net,
> const struct in6_addr *saddr, __be16 sport,
> @@ -193,11 +176,14 @@ static struct sock *udp6_lib_lookup2(struct net *net,
> score = compute_score(sk, net, saddr, sport,
> daddr, hnum, dif, sdif);
> if (score > badness) {
> - result = lookup_reuseport(net, sk, skb,
> - saddr, sport, daddr, hnum);
> - /* Fall back to scoring if group has connections */
> - if (result && !reuseport_has_conns(sk))
> - return result;
> + if (sk->sk_state != TCP_ESTABLISHED) {
> + result = inet6_lookup_reuseport(net, sk, skb, sizeof(struct udphdr),
> + saddr, sport, daddr, hnum,
> + udp6_ehashfn);
> + /* Fall back to scoring if group has connections */
> + if (result && !reuseport_has_conns(sk))
> + return result;
> + }
>
> result = result ? : sk;
> badness = score;
> @@ -225,7 +211,8 @@ static inline struct sock *udp6_lookup_run_bpf(struct net *net,
> if (no_reuseport || IS_ERR_OR_NULL(sk))
> return sk;
>
> - reuse_sk = lookup_reuseport(net, sk, skb, saddr, sport, daddr, hnum);
> + reuse_sk = inet6_lookup_reuseport(net, sk, skb, sizeof(struct udphdr),
> + saddr, sport, daddr, hnum, udp6_ehashfn);
> if (reuse_sk)
> sk = reuse_sk;
> return sk;
>
> --
> 2.40.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 4/6] net: remove duplicate sk_lookup helpers
2023-06-13 10:14 ` [PATCH bpf-next v2 4/6] net: remove duplicate sk_lookup helpers Lorenz Bauer
@ 2023-06-13 19:03 ` Kuniyuki Iwashima
2023-06-13 19:41 ` kernel test robot
1 sibling, 0 replies; 22+ messages in thread
From: Kuniyuki Iwashima @ 2023-06-13 19:03 UTC (permalink / raw)
To: lmb
Cc: andrii, ast, bpf, daniel, davem, dsahern, edumazet, haoluo,
hemanthmalla, joe, john.fastabend, jolsa, kpsingh, kuba, kuniyu,
linux-kernel, linux-kselftest, martin.lau, mykolal, netdev,
pabeni, sdf, shuah, song, willemdebruijn.kernel, yhs
From: Lorenz Bauer <lmb@isovalent.com>
Date: Tue, 13 Jun 2023 11:14:59 +0100
> Now that inet[6]_lookup_reuseport are parameterised on the ehashfn
> we can remove two sk_lookup helpers.
>
> Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
> ---
> include/net/inet6_hashtables.h | 9 +++++++++
> include/net/inet_hashtables.h | 7 +++++++
> net/ipv4/inet_hashtables.c | 26 +++++++++++++-------------
> net/ipv4/udp.c | 32 +++++---------------------------
> net/ipv6/inet6_hashtables.c | 30 +++++++++++++++---------------
> net/ipv6/udp.c | 34 +++++-----------------------------
> 6 files changed, 54 insertions(+), 84 deletions(-)
>
> diff --git a/include/net/inet6_hashtables.h b/include/net/inet6_hashtables.h
> index 49d586454287..4d2a1a3c0be7 100644
> --- a/include/net/inet6_hashtables.h
> +++ b/include/net/inet6_hashtables.h
> @@ -73,6 +73,15 @@ struct sock *inet6_lookup_listener(struct net *net,
> const unsigned short hnum,
> const int dif, const int sdif);
>
> +struct sock *inet6_lookup_run_sk_lookup(struct net *net,
I understand this comes from SEC("sk_lookup"), but this sounds
redundant and run_bpf is clearer for non-BPF folks.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 4/6] net: remove duplicate sk_lookup helpers
2023-06-13 10:14 ` [PATCH bpf-next v2 4/6] net: remove duplicate sk_lookup helpers Lorenz Bauer
2023-06-13 19:03 ` Kuniyuki Iwashima
@ 2023-06-13 19:41 ` kernel test robot
1 sibling, 0 replies; 22+ messages in thread
From: kernel test robot @ 2023-06-13 19:41 UTC (permalink / raw)
To: Lorenz Bauer, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, David Ahern, Willem de Bruijn, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Joe Stringer, Mykola Lysenko, Shuah Khan,
Kuniyuki Iwashima
Cc: llvm, oe-kbuild-all, netdev, Hemanth Malla, linux-kernel, bpf,
linux-kselftest, Lorenz Bauer
Hi Lorenz,
kernel test robot noticed the following build errors:
[auto build test ERROR on 25085b4e9251c77758964a8e8651338972353642]
url: https://github.com/intel-lab-lkp/linux/commits/Lorenz-Bauer/net-export-inet_lookup_reuseport-and-inet6_lookup_reuseport/20230613-181619
base: 25085b4e9251c77758964a8e8651338972353642
patch link: https://lore.kernel.org/r/20230613-so-reuseport-v2-4-b7c69a342613%40isovalent.com
patch subject: [PATCH bpf-next v2 4/6] net: remove duplicate sk_lookup helpers
config: hexagon-randconfig-r041-20230612 (https://download.01.org/0day-ci/archive/20230614/202306140351.Y1JjOIxo-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 25085b4e9251c77758964a8e8651338972353642
b4 shazam https://lore.kernel.org/r/20230613-so-reuseport-v2-4-b7c69a342613@isovalent.com
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306140351.Y1JjOIxo-lkp@intel.com/
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "inet6_lookup_run_sk_lookup" [net/ipv6/ipv6.ko] undefined!
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions
2023-06-13 18:56 ` Kuniyuki Iwashima
@ 2023-06-14 15:25 ` Lorenz Bauer
2023-06-14 16:52 ` Kuniyuki Iwashima
2023-06-20 14:26 ` Lorenz Bauer
1 sibling, 1 reply; 22+ messages in thread
From: Lorenz Bauer @ 2023-06-14 15:25 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: andrii, ast, bpf, daniel, davem, dsahern, edumazet, haoluo,
hemanthmalla, joe, john.fastabend, jolsa, kpsingh, kuba,
linux-kernel, linux-kselftest, martin.lau, mykolal, netdev,
pabeni, sdf, shuah, song, willemdebruijn.kernel, yhs
On Tue, Jun 13, 2023 at 7:57 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> else {
> result = sk;
> }
>
> The assignment to result below is buggy. Let's say SO_REUSEPROT group
> have TCP_CLOSE and TCP_ESTABLISHED sockets.
I'm not very familiar with SO_REUSEPORT, I assumed (incorrectly
probably) that such a group would only ever have TCP_CLOSE in UDP case
and TCP_LISTENING in TCP case. Can you explain how I could end up in
this situation?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions
2023-06-13 15:32 ` Simon Horman
@ 2023-06-14 15:42 ` Lorenz Bauer
2023-06-15 7:21 ` Simon Horman
0 siblings, 1 reply; 22+ messages in thread
From: Lorenz Bauer @ 2023-06-14 15:42 UTC (permalink / raw)
To: Simon Horman
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
David Ahern, Willem de Bruijn, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Joe Stringer, Mykola Lysenko, Shuah Khan,
Kuniyuki Iwashima, Hemanth Malla, netdev, linux-kernel, bpf,
linux-kselftest
On Tue, Jun 13, 2023 at 4:33 PM Simon Horman <simon.horman@corigine.com> wrote:
> >
> > +INDIRECT_CALLABLE_DECLARE(u32 udp_ehashfn(const struct net *,
> > + const __be32, const __u16,
> > + const __be32, const __be16));
> > +
>
> Hi Lorenz,
>
> Would this be better placed in a header file?
> GCC complains that in udp.c this function is neither static nor
> has a prototype.
Hi Simon,
The problem is that I don't want to pull in udp.h in
inet_hashtables.c, but that is the natural place to define that
function. I was hoping the macro magic would solve the problem, but oh
well. How do you make gcc complain, and what is the full error
message?
Thanks
Lorenz
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions
2023-06-14 15:25 ` Lorenz Bauer
@ 2023-06-14 16:52 ` Kuniyuki Iwashima
0 siblings, 0 replies; 22+ messages in thread
From: Kuniyuki Iwashima @ 2023-06-14 16:52 UTC (permalink / raw)
To: lmb
Cc: andrii, ast, bpf, daniel, davem, dsahern, edumazet, haoluo,
hemanthmalla, joe, john.fastabend, jolsa, kpsingh, kuba, kuniyu,
linux-kernel, linux-kselftest, martin.lau, mykolal, netdev,
pabeni, sdf, shuah, song, willemdebruijn.kernel, yhs
From: Lorenz Bauer <lmb@isovalent.com>
Date: Wed, 14 Jun 2023 16:25:05 +0100
> On Tue, Jun 13, 2023 at 7:57 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> >
> > else {
> > result = sk;
> > }
> >
> > The assignment to result below is buggy. Let's say SO_REUSEPROT group
> > have TCP_CLOSE and TCP_ESTABLISHED sockets.
>
> I'm not very familiar with SO_REUSEPORT, I assumed (incorrectly
> probably) that such a group would only ever have TCP_CLOSE in UDP case
> and TCP_LISTENING in TCP case. Can you explain how I could end up in
> this situation?
When we call conenct() for UDP socket in SO_REUSEPORT group, the state
is changed from TCP_CLOSE to TCP_ESTABLISHED in __ip4_datagram_connect(),
and the socket remains in the group.
That's why we check TCP_ESTABLISHED in reuseport_select_sock_by_hash()
that is always false for TCP but true for UDP in the case above.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions
2023-06-14 15:42 ` Lorenz Bauer
@ 2023-06-15 7:21 ` Simon Horman
0 siblings, 0 replies; 22+ messages in thread
From: Simon Horman @ 2023-06-15 7:21 UTC (permalink / raw)
To: Lorenz Bauer
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
David Ahern, Willem de Bruijn, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Joe Stringer, Mykola Lysenko, Shuah Khan,
Kuniyuki Iwashima, Hemanth Malla, netdev, linux-kernel, bpf,
linux-kselftest
On Wed, Jun 14, 2023 at 04:42:45PM +0100, Lorenz Bauer wrote:
> On Tue, Jun 13, 2023 at 4:33 PM Simon Horman <simon.horman@corigine.com> wrote:
> > >
> > > +INDIRECT_CALLABLE_DECLARE(u32 udp_ehashfn(const struct net *,
> > > + const __be32, const __u16,
> > > + const __be32, const __be16));
> > > +
> >
> > Hi Lorenz,
> >
> > Would this be better placed in a header file?
> > GCC complains that in udp.c this function is neither static nor
> > has a prototype.
>
> Hi Simon,
>
> The problem is that I don't want to pull in udp.h in
> inet_hashtables.c, but that is the natural place to define that
> function. I was hoping the macro magic would solve the problem, but oh
> well. How do you make gcc complain, and what is the full error
> message?
Hi Lorenz,
sorry for the bother.
With gcc 12.3.0 [1] on x86_64 I see:
$ make allmodconfig
$ make W=1 net/ipv4/udp.o
net/ipv4/udp.c:410:5: error: no previous prototype for 'udp_ehashfn' [-Werror=missing-prototypes]
410 | u32 udp_ehashfn(const struct net *net, const __be32 laddr, const __u16 lport,
| ^~~~~~~~~~~
[1] https://mirrors.edge.kernel.org/pub/tools/crosstool/
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions
2023-06-13 18:56 ` Kuniyuki Iwashima
2023-06-14 15:25 ` Lorenz Bauer
@ 2023-06-20 14:26 ` Lorenz Bauer
2023-06-20 18:31 ` Kuniyuki Iwashima
1 sibling, 1 reply; 22+ messages in thread
From: Lorenz Bauer @ 2023-06-20 14:26 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: andrii, ast, bpf, daniel, davem, dsahern, edumazet, haoluo,
hemanthmalla, joe, john.fastabend, jolsa, kpsingh, kuba,
linux-kernel, linux-kselftest, martin.lau, mykolal, netdev,
pabeni, sdf, shuah, song, willemdebruijn.kernel, yhs
On Tue, Jun 13, 2023 at 7:57 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> The assignment to result below is buggy. Let's say SO_REUSEPROT group
> have TCP_CLOSE and TCP_ESTABLISHED sockets.
>
> 1. Find TCP_CLOSE sk and do SO_REUSEPORT lookup
> 2. result is not NULL, but the group has TCP_ESTABLISHED sk
> 3. result = result
> 4. Find TCP_ESTABLISHED sk, which has a higher score
> 5. result = result (TCP_CLOSE) <-- should be sk.
>
> Same for v6 function.
Thanks for your explanation, I think I get it now. I misunderstood
that you were worried about returning TCP_ESTABLISHED instead of
TCP_CLOSE, but it's exactly the other way around.
I have a follow up question regarding the existing code:
result = lookup_reuseport(net, sk, skb,
saddr, sport, daddr, hnum);
/* Fall back to scoring if group has connections */
if (result && !reuseport_has_conns(sk))
return result;
result = result ? : sk;
badness = score;
Assuming that result != NULL but reuseport_has_conns() == true, we use
the reuseport socket as the result, but assign the score of sk to
badness. Shouldn't we use the score of the reuseport socket?
Thanks
Lorenz
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions
2023-06-20 14:26 ` Lorenz Bauer
@ 2023-06-20 18:31 ` Kuniyuki Iwashima
2023-06-21 8:01 ` Lorenz Bauer
2023-06-21 13:49 ` Lorenz Bauer
0 siblings, 2 replies; 22+ messages in thread
From: Kuniyuki Iwashima @ 2023-06-20 18:31 UTC (permalink / raw)
To: lmb
Cc: andrii, ast, bpf, daniel, davem, dsahern, edumazet, haoluo,
hemanthmalla, joe, john.fastabend, jolsa, kpsingh, kuba, kuniyu,
linux-kernel, linux-kselftest, martin.lau, mykolal, netdev,
pabeni, sdf, shuah, song, willemdebruijn.kernel, yhs
From: Lorenz Bauer <lmb@isovalent.com>
Date: Tue, 20 Jun 2023 15:26:05 +0100
> On Tue, Jun 13, 2023 at 7:57 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> >
> > The assignment to result below is buggy. Let's say SO_REUSEPROT group
> > have TCP_CLOSE and TCP_ESTABLISHED sockets.
> >
> > 1. Find TCP_CLOSE sk and do SO_REUSEPORT lookup
> > 2. result is not NULL, but the group has TCP_ESTABLISHED sk
> > 3. result = result
> > 4. Find TCP_ESTABLISHED sk, which has a higher score
> > 5. result = result (TCP_CLOSE) <-- should be sk.
> >
> > Same for v6 function.
>
> Thanks for your explanation, I think I get it now. I misunderstood
> that you were worried about returning TCP_ESTABLISHED instead of
> TCP_CLOSE, but it's exactly the other way around.
>
> I have a follow up question regarding the existing code:
>
> result = lookup_reuseport(net, sk, skb,
> saddr, sport, daddr, hnum);
> /* Fall back to scoring if group has connections */
> if (result && !reuseport_has_conns(sk))
> return result;
>
> result = result ? : sk;
> badness = score;
>
> Assuming that result != NULL but reuseport_has_conns() == true, we use
> the reuseport socket as the result, but assign the score of sk to
> badness. Shouldn't we use the score of the reuseport socket?
Good point. This is based on an assumption that all SO_REUSEPORT
sockets have the same score, which is wrong for two corner cases
if reuseport_has_conns() == true :
1) SO_INCOMING_CPU is set
-> selected sk might have +1 score
2) BPF prog returns ESTABLISHED and/or SO_INCOMING_CPU sk
-> selected sk will have more than 8
Using the old score could trigger more lookups depending on the
order that sockets are created.
sk -> sk (SO_INCOMING_CPU) -> sk (ESTABLISHED)
| |
`-> select the next SO_INCOMING_CPU sk
|
`-> select itself (We should save this lookup)
So, yes, we should update badness like
if (unlikely(result)) {
badness = compute_score(result, ...);
} else {
result = sk;
badness = score;
}
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions
2023-06-20 18:31 ` Kuniyuki Iwashima
@ 2023-06-21 8:01 ` Lorenz Bauer
2023-06-21 13:49 ` Lorenz Bauer
1 sibling, 0 replies; 22+ messages in thread
From: Lorenz Bauer @ 2023-06-21 8:01 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: andrii, ast, bpf, daniel, davem, dsahern, edumazet, haoluo,
hemanthmalla, joe, john.fastabend, jolsa, kpsingh, kuba,
linux-kernel, linux-kselftest, martin.lau, mykolal, netdev,
pabeni, sdf, shuah, song, willemdebruijn.kernel, yhs
On Tue, Jun 20, 2023 at 7:31 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> Good point. This is based on an assumption that all SO_REUSEPORT
> sockets have the same score, which is wrong for two corner cases
> if reuseport_has_conns() == true :
>
> 1) SO_INCOMING_CPU is set
> -> selected sk might have +1 score
>
> 2) BPF prog returns ESTABLISHED and/or SO_INCOMING_CPU sk
> -> selected sk will have more than 8
>
> Using the old score could trigger more lookups depending on the
> order that sockets are created.
So the result will still be correct, but it's less performant? Happy
to fix a perf regression, but if the result is incorrect this might
need a separate fix?
Best
Lorenz
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions
2023-06-20 18:31 ` Kuniyuki Iwashima
2023-06-21 8:01 ` Lorenz Bauer
@ 2023-06-21 13:49 ` Lorenz Bauer
2023-06-21 15:00 ` Kuniyuki Iwashima
1 sibling, 1 reply; 22+ messages in thread
From: Lorenz Bauer @ 2023-06-21 13:49 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: andrii, ast, bpf, daniel, davem, dsahern, edumazet, haoluo,
hemanthmalla, joe, john.fastabend, jolsa, kpsingh, kuba,
linux-kernel, linux-kselftest, martin.lau, mykolal, netdev,
pabeni, sdf, shuah, song, willemdebruijn.kernel, yhs
On Tue, Jun 20, 2023 at 7:31 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> Good point. This is based on an assumption that all SO_REUSEPORT
> sockets have the same score, which is wrong for two corner cases [...]
I did some more digging. I think this was introduced by commit
efc6b6f6c311 ("udp: Improve load balancing for SO_REUSEPORT.") which
unfortunately ran into a merge conflict. That resulted in Dave Miller
moving the bug around in commit a57066b1a019 ("Merge
git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net"). Can you
take a look and let me know if you think that is correct?
Best
Lorenz
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions
2023-06-21 13:49 ` Lorenz Bauer
@ 2023-06-21 15:00 ` Kuniyuki Iwashima
0 siblings, 0 replies; 22+ messages in thread
From: Kuniyuki Iwashima @ 2023-06-21 15:00 UTC (permalink / raw)
To: lmb
Cc: andrii, ast, bpf, daniel, davem, dsahern, edumazet, haoluo,
hemanthmalla, joe, john.fastabend, jolsa, kpsingh, kuba, kuniyu,
linux-kernel, linux-kselftest, martin.lau, mykolal, netdev,
pabeni, sdf, shuah, song, willemdebruijn.kernel, yhs
From: Lorenz Bauer <lmb@isovalent.com>
Date: Wed, 21 Jun 2023 09:01:15 +0100
> On Tue, Jun 20, 2023 at 7:31 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> >
> > Good point. This is based on an assumption that all SO_REUSEPORT
> > sockets have the same score, which is wrong for two corner cases
> > if reuseport_has_conns() == true :
> >
> > 1) SO_INCOMING_CPU is set
> > -> selected sk might have +1 score
> >
> > 2) BPF prog returns ESTABLISHED and/or SO_INCOMING_CPU sk
> > -> selected sk will have more than 8
> >
> > Using the old score could trigger more lookups depending on the
> > order that sockets are created.
>
> So the result will still be correct, but it's less performant? Happy
> to fix a perf regression, but if the result is incorrect this might
> need a separate fix?
Right, the result is always correct.
If BPF prog selects a different socket per lookup, there is no
consistency, but it _is_ corret.
> I did some more digging. I think this was introduced by commit
> efc6b6f6c311 ("udp: Improve load balancing for SO_REUSEPORT.") which
> unfortunately ran into a merge conflict. That resulted in Dave Miller
> moving the bug around in commit a57066b1a019 ("Merge
> git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net"). Can you
> take a look and let me know if you think that is correct?
Yes, I should have updated the score too in efc6b6f6c311 to save
unneeded lookups. The conflict itself was resolved properly.
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2023-06-21 15:07 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-13 10:14 [PATCH bpf-next v2 0/6] Add SO_REUSEPORT support for TC bpf_sk_assign Lorenz Bauer
2023-06-13 10:14 ` [PATCH bpf-next v2 1/6] net: export inet_lookup_reuseport and inet6_lookup_reuseport Lorenz Bauer
2023-06-13 10:14 ` [PATCH bpf-next v2 2/6] net: document inet[6]_lookup_reuseport sk_state requirements Lorenz Bauer
2023-06-13 10:14 ` [PATCH bpf-next v2 3/6] net: remove duplicate reuseport_lookup functions Lorenz Bauer
2023-06-13 15:32 ` Simon Horman
2023-06-14 15:42 ` Lorenz Bauer
2023-06-15 7:21 ` Simon Horman
2023-06-13 17:26 ` kernel test robot
2023-06-13 18:56 ` Kuniyuki Iwashima
2023-06-14 15:25 ` Lorenz Bauer
2023-06-14 16:52 ` Kuniyuki Iwashima
2023-06-20 14:26 ` Lorenz Bauer
2023-06-20 18:31 ` Kuniyuki Iwashima
2023-06-21 8:01 ` Lorenz Bauer
2023-06-21 13:49 ` Lorenz Bauer
2023-06-21 15:00 ` Kuniyuki Iwashima
2023-06-13 10:14 ` [PATCH bpf-next v2 4/6] net: remove duplicate sk_lookup helpers Lorenz Bauer
2023-06-13 19:03 ` Kuniyuki Iwashima
2023-06-13 19:41 ` kernel test robot
2023-06-13 10:15 ` [PATCH bpf-next v2 5/6] bpf, net: Support SO_REUSEPORT sockets with bpf_sk_assign Lorenz Bauer
2023-06-13 17:26 ` kernel test robot
2023-06-13 10:15 ` [PATCH bpf-next v2 6/6] selftests/bpf: Test that SO_REUSEPORT can be used with sk_assign helper Lorenz Bauer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).