bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper
@ 2020-03-27  4:25 Joe Stringer
  2020-03-27  4:25 ` [PATCHv3 bpf-next 1/5] bpf: Add socket assign support Joe Stringer
                   ` (7 more replies)
  0 siblings, 8 replies; 21+ messages in thread
From: Joe Stringer @ 2020-03-27  4:25 UTC (permalink / raw)
  To: bpf; +Cc: netdev, daniel, ast, eric.dumazet, lmb, kafai

Introduce a new helper that allows assigning a previously-found socket
to the skb as the packet is received towards the stack, to cause the
stack to guide the packet towards that socket subject to local routing
configuration. The intention is to support TProxy use cases more
directly from eBPF programs attached at TC ingress, to simplify and
streamline Linux stack configuration in scale environments with Cilium.

Normally in ip{,6}_rcv_core(), the skb will be orphaned, dropping any
existing socket reference associated with the skb. Existing tproxy
implementations in netfilter get around this restriction by running the
tproxy logic after ip_rcv_core() in the PREROUTING table. However, this
is not an option for TC-based logic (including eBPF programs attached at
TC ingress).

This series introduces the BPF helper bpf_sk_assign() to associate the
socket with the skb on the ingress path as the packet is passed up the
stack. The initial patch in the series simply takes a reference on the
socket to ensure safety, but later patches relax this for listen
sockets.

To ensure delivery to the relevant socket, we still consult the routing
table, for full examples of how to configure see the tests in patch #5;
the simplest form of the route would look like this:

  $ ip route add local default dev lo

This series is laid out as follows:
* Patch 1 extends the eBPF API to add sk_assign() and defines a new
  socket free function to allow the later paths to understand when the
  socket associated with the skb should be kept through receive.
* Patches 2-3 optimize the receive path to avoid taking a reference on
  listener sockets during receive.
* Patches 4-5 extends the selftests with examples of the new
  functionality and validation of correct behaviour.

Changes since v2:
* Add selftests for UDP socket redirection
* Drop the early demux optimization patch (defer for more testing)
* Fix check for orphaning after TC act return
* Tidy up the tests to clean up properly and be less noisy.

Changes since v1:
* Replace the metadata_dst approach with using the skb->destructor to
  determine whether the socket has been prefetched. This is much
  simpler.
* Avoid taking a reference on listener sockets during receive
* Restrict assigning sockets across namespaces
* Restrict assigning SO_REUSEPORT sockets
* Fix cookie usage for socket dst check
* Rebase the tests against test_progs infrastructure
* Tidy up commit messages

Joe Stringer (4):
  bpf: Add socket assign support
  net: Track socket refcounts in skb_steal_sock()
  bpf: Don't refcount LISTEN sockets in sk_assign()
  selftests: bpf: Extend sk_assign tests for UDP

Lorenz Bauer (1):
  selftests: bpf: add test for sk_assign

 include/net/inet6_hashtables.h                |   3 +-
 include/net/inet_hashtables.h                 |   3 +-
 include/net/sock.h                            |  42 ++-
 include/uapi/linux/bpf.h                      |  25 +-
 net/core/filter.c                             |  35 +-
 net/core/sock.c                               |  10 +
 net/ipv4/ip_input.c                           |   3 +-
 net/ipv4/udp.c                                |   6 +-
 net/ipv6/ip6_input.c                          |   3 +-
 net/ipv6/udp.c                                |   9 +-
 net/sched/act_bpf.c                           |   3 +
 tools/include/uapi/linux/bpf.h                |  25 +-
 .../selftests/bpf/prog_tests/sk_assign.c      | 309 ++++++++++++++++++
 .../selftests/bpf/progs/test_sk_assign.c      | 204 ++++++++++++
 14 files changed, 656 insertions(+), 24 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/sk_assign.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_sk_assign.c

-- 
2.20.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCHv3 bpf-next 1/5] bpf: Add socket assign support
  2020-03-27  4:25 [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper Joe Stringer
@ 2020-03-27  4:25 ` Joe Stringer
  2020-03-27 18:44   ` Martin KaFai Lau
  2020-03-27  4:25 ` [PATCHv3 bpf-next 2/5] net: Track socket refcounts in skb_steal_sock() Joe Stringer
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Joe Stringer @ 2020-03-27  4:25 UTC (permalink / raw)
  To: bpf; +Cc: netdev, daniel, ast, eric.dumazet, lmb, kafai

Add support for TPROXY via a new bpf helper, bpf_sk_assign().

This helper requires the BPF program to discover the socket via a call
to bpf_sk*_lookup_*(), then pass this socket to the new helper. The
helper takes its own reference to the socket in addition to any existing
reference that may or may not currently be obtained for the duration of
BPF processing. For the destination socket to receive the traffic, the
traffic must be routed towards that socket via local route. The
simplest example route is below, but in practice you may want to route
traffic more narrowly (eg by CIDR):

  $ ip route add local default dev lo

This patch avoids trying to introduce an extra bit into the skb->sk, as
that would require more invasive changes to all code interacting with
the socket to ensure that the bit is handled correctly, such as all
error-handling cases along the path from the helper in BPF through to
the orphan path in the input. Instead, we opt to use the destructor
variable to switch on the prefetch of the socket.

Signed-off-by: Joe Stringer <joe@wand.net.nz>
---
v3: Check skb_sk_is_prefetched() in TC level redirect check
v2: Use skb->destructor to determine socket prefetch usage instead of
      introducing a new metadata_dst
    Restrict socket assign to same netns as TC device
    Restrict assigning reuseport sockets
    Adjust commit wording
v1: Initial version
---
 include/net/sock.h             |  7 +++++++
 include/uapi/linux/bpf.h       | 25 ++++++++++++++++++++++++-
 net/core/filter.c              | 31 +++++++++++++++++++++++++++++++
 net/core/sock.c                |  9 +++++++++
 net/ipv4/ip_input.c            |  3 ++-
 net/ipv6/ip6_input.c           |  3 ++-
 net/sched/act_bpf.c            |  3 +++
 tools/include/uapi/linux/bpf.h | 25 ++++++++++++++++++++++++-
 8 files changed, 102 insertions(+), 4 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index b5cca7bae69b..2613d21a667a 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1657,6 +1657,7 @@ struct sk_buff *sock_omalloc(struct sock *sk, unsigned long size,
 void skb_orphan_partial(struct sk_buff *skb);
 void sock_rfree(struct sk_buff *skb);
 void sock_efree(struct sk_buff *skb);
+void sock_pfree(struct sk_buff *skb);
 #ifdef CONFIG_INET
 void sock_edemux(struct sk_buff *skb);
 #else
@@ -2526,6 +2527,12 @@ void sock_net_set(struct sock *sk, struct net *net)
 	write_pnet(&sk->sk_net, net);
 }
 
+static inline bool
+skb_sk_is_prefetched(struct sk_buff *skb)
+{
+	return skb->destructor == sock_pfree;
+}
+
 static inline struct sock *skb_steal_sock(struct sk_buff *skb)
 {
 	if (skb->sk) {
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 5d01c5c7e598..c77cd630a724 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2950,6 +2950,28 @@ union bpf_attr {
  *		restricted to raw_tracepoint bpf programs.
  *	Return
  *		0 on success, or a negative error in case of failure.
+ *
+ * int bpf_sk_assign(struct sk_buff *skb, struct bpf_sock *sk, u64 flags)
+ *	Description
+ *		Assign the *sk* to the *skb*. When combined with appropriate
+ *		routing configuration to receive the packet towards the socket,
+ *		will cause *skb* to be delivered to the specified socket.
+ *		Subsequent redirection of *skb* via  **bpf_redirect**\ (),
+ *		**bpf_clone_redirect**\ () or other methods outside of BPF may
+ *		interfere with successful delivery to the socket.
+ *
+ *		This operation is only valid from TC ingress path.
+ *
+ *		The *flags* argument must be zero.
+ *	Return
+ *		0 on success, or a negative errno in case of failure.
+ *
+ *		* **-EINVAL**		Unsupported flags specified.
+ *		* **-ENOENT**		Socket is unavailable for assignment.
+ *		* **-ENETUNREACH**	Socket is unreachable (wrong netns).
+ *		* **-EOPNOTSUPP**	Unsupported operation, for example a
+ *					call from outside of TC ingress.
+ *		* **-ESOCKTNOSUPPORT**	Socket type not supported (reuseport).
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3073,7 +3095,8 @@ union bpf_attr {
 	FN(jiffies64),			\
 	FN(read_branch_records),	\
 	FN(get_ns_current_pid_tgid),	\
-	FN(xdp_output),
+	FN(xdp_output),			\
+	FN(sk_assign),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
diff --git a/net/core/filter.c b/net/core/filter.c
index 96350a743539..2bcc29c1a595 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5860,6 +5860,35 @@ static const struct bpf_func_proto bpf_tcp_gen_syncookie_proto = {
 	.arg5_type	= ARG_CONST_SIZE,
 };
 
+BPF_CALL_3(bpf_sk_assign, struct sk_buff *, skb, struct sock *, sk, u64, flags)
+{
+	if (flags != 0)
+		return -EINVAL;
+	if (!skb_at_tc_ingress(skb))
+		return -EOPNOTSUPP;
+	if (unlikely(dev_net(skb->dev) != sock_net(sk)))
+		return -ENETUNREACH;
+	if (unlikely(sk->sk_reuseport))
+		return -ESOCKTNOSUPPORT;
+	if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
+		return -ENOENT;
+
+	skb_orphan(skb);
+	skb->sk = sk;
+	skb->destructor = sock_pfree;
+
+	return 0;
+}
+
+static const struct bpf_func_proto bpf_sk_assign_proto = {
+	.func		= bpf_sk_assign,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type      = ARG_PTR_TO_CTX,
+	.arg2_type      = ARG_PTR_TO_SOCK_COMMON,
+	.arg3_type	= ARG_ANYTHING,
+};
+
 #endif /* CONFIG_INET */
 
 bool bpf_helper_changes_pkt_data(void *func)
@@ -6153,6 +6182,8 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_skb_ecn_set_ce_proto;
 	case BPF_FUNC_tcp_gen_syncookie:
 		return &bpf_tcp_gen_syncookie_proto;
+	case BPF_FUNC_sk_assign:
+		return &bpf_sk_assign_proto;
 #endif
 	default:
 		return bpf_base_func_proto(func_id);
diff --git a/net/core/sock.c b/net/core/sock.c
index 0fc8937a7ff4..cfaf60267360 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2071,6 +2071,15 @@ void sock_efree(struct sk_buff *skb)
 }
 EXPORT_SYMBOL(sock_efree);
 
+/* Buffer destructor for prefetch/receive path where reference count may
+ * not be held, e.g. for listen sockets.
+ */
+void sock_pfree(struct sk_buff *skb)
+{
+	sock_edemux(skb);
+}
+EXPORT_SYMBOL(sock_pfree);
+
 kuid_t sock_i_uid(struct sock *sk)
 {
 	kuid_t uid;
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index aa438c6758a7..b0c244af1e4d 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -509,7 +509,8 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
 	IPCB(skb)->iif = skb->skb_iif;
 
 	/* Must drop socket now because of tproxy. */
-	skb_orphan(skb);
+	if (!skb_sk_is_prefetched(skb))
+		skb_orphan(skb);
 
 	return skb;
 
diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
index 7b089d0ac8cd..e96304d8a4a7 100644
--- a/net/ipv6/ip6_input.c
+++ b/net/ipv6/ip6_input.c
@@ -285,7 +285,8 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
 	rcu_read_unlock();
 
 	/* Must drop socket now because of tproxy. */
-	skb_orphan(skb);
+	if (!skb_sk_is_prefetched(skb))
+		skb_orphan(skb);
 
 	return skb;
 err:
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 46f47e58b3be..54d5652cfe6c 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -12,6 +12,7 @@
 #include <linux/bpf.h>
 
 #include <net/netlink.h>
+#include <net/sock.h>
 #include <net/pkt_sched.h>
 #include <net/pkt_cls.h>
 
@@ -53,6 +54,8 @@ static int tcf_bpf_act(struct sk_buff *skb, const struct tc_action *act,
 		bpf_compute_data_pointers(skb);
 		filter_res = BPF_PROG_RUN(filter, skb);
 	}
+	if (skb_sk_is_prefetched(skb) && filter_res != TC_ACT_OK)
+		skb_orphan(skb);
 	rcu_read_unlock();
 
 	/* A BPF program may overwrite the default action opcode.
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 5d01c5c7e598..c77cd630a724 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2950,6 +2950,28 @@ union bpf_attr {
  *		restricted to raw_tracepoint bpf programs.
  *	Return
  *		0 on success, or a negative error in case of failure.
+ *
+ * int bpf_sk_assign(struct sk_buff *skb, struct bpf_sock *sk, u64 flags)
+ *	Description
+ *		Assign the *sk* to the *skb*. When combined with appropriate
+ *		routing configuration to receive the packet towards the socket,
+ *		will cause *skb* to be delivered to the specified socket.
+ *		Subsequent redirection of *skb* via  **bpf_redirect**\ (),
+ *		**bpf_clone_redirect**\ () or other methods outside of BPF may
+ *		interfere with successful delivery to the socket.
+ *
+ *		This operation is only valid from TC ingress path.
+ *
+ *		The *flags* argument must be zero.
+ *	Return
+ *		0 on success, or a negative errno in case of failure.
+ *
+ *		* **-EINVAL**		Unsupported flags specified.
+ *		* **-ENOENT**		Socket is unavailable for assignment.
+ *		* **-ENETUNREACH**	Socket is unreachable (wrong netns).
+ *		* **-EOPNOTSUPP**	Unsupported operation, for example a
+ *					call from outside of TC ingress.
+ *		* **-ESOCKTNOSUPPORT**	Socket type not supported (reuseport).
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3073,7 +3095,8 @@ union bpf_attr {
 	FN(jiffies64),			\
 	FN(read_branch_records),	\
 	FN(get_ns_current_pid_tgid),	\
-	FN(xdp_output),
+	FN(xdp_output),			\
+	FN(sk_assign),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCHv3 bpf-next 2/5] net: Track socket refcounts in skb_steal_sock()
  2020-03-27  4:25 [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper Joe Stringer
  2020-03-27  4:25 ` [PATCHv3 bpf-next 1/5] bpf: Add socket assign support Joe Stringer
@ 2020-03-27  4:25 ` Joe Stringer
  2020-03-27 18:45   ` Martin KaFai Lau
  2020-03-27  4:25 ` [PATCHv3 bpf-next 3/5] bpf: Don't refcount LISTEN sockets in sk_assign() Joe Stringer
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Joe Stringer @ 2020-03-27  4:25 UTC (permalink / raw)
  To: bpf; +Cc: netdev, daniel, ast, eric.dumazet, lmb, kafai

Refactor the UDP/TCP handlers slightly to allow skb_steal_sock() to make
the determination of whether the socket is reference counted in the case
where it is prefetched by earlier logic such as early_demux or
dst_sk_prefetch.

Signed-off-by: Joe Stringer <joe@wand.net.nz>
---
v3: No changes
v2: Initial version
---
 include/net/inet6_hashtables.h |  3 +--
 include/net/inet_hashtables.h  |  3 +--
 include/net/sock.h             | 10 +++++++++-
 net/ipv4/udp.c                 |  6 ++++--
 net/ipv6/udp.c                 |  9 ++++++---
 5 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/include/net/inet6_hashtables.h b/include/net/inet6_hashtables.h
index fe96bf247aac..81b965953036 100644
--- a/include/net/inet6_hashtables.h
+++ b/include/net/inet6_hashtables.h
@@ -85,9 +85,8 @@ static inline struct sock *__inet6_lookup_skb(struct inet_hashinfo *hashinfo,
 					      int iif, int sdif,
 					      bool *refcounted)
 {
-	struct sock *sk = skb_steal_sock(skb);
+	struct sock *sk = skb_steal_sock(skb, refcounted);
 
-	*refcounted = true;
 	if (sk)
 		return sk;
 
diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index d0019d3395cf..ad64ba6a057f 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -379,10 +379,9 @@ static inline struct sock *__inet_lookup_skb(struct inet_hashinfo *hashinfo,
 					     const int sdif,
 					     bool *refcounted)
 {
-	struct sock *sk = skb_steal_sock(skb);
+	struct sock *sk = skb_steal_sock(skb, refcounted);
 	const struct iphdr *iph = ip_hdr(skb);
 
-	*refcounted = true;
 	if (sk)
 		return sk;
 
diff --git a/include/net/sock.h b/include/net/sock.h
index 2613d21a667a..1ca2e808cb8e 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2533,15 +2533,23 @@ skb_sk_is_prefetched(struct sk_buff *skb)
 	return skb->destructor == sock_pfree;
 }
 
-static inline struct sock *skb_steal_sock(struct sk_buff *skb)
+/**
+ * skb_steal_sock
+ * @skb to steal the socket from
+ * @refcounted is set to true if the socket is reference-counted
+ */
+static inline struct sock *
+skb_steal_sock(struct sk_buff *skb, bool *refcounted)
 {
 	if (skb->sk) {
 		struct sock *sk = skb->sk;
 
+		*refcounted = true;
 		skb->destructor = NULL;
 		skb->sk = NULL;
 		return sk;
 	}
+	*refcounted = false;
 	return NULL;
 }
 
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 2633fc231593..b4035021bbd3 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2288,6 +2288,7 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
 	struct rtable *rt = skb_rtable(skb);
 	__be32 saddr, daddr;
 	struct net *net = dev_net(skb->dev);
+	bool refcounted;
 
 	/*
 	 *  Validate the packet.
@@ -2313,7 +2314,7 @@ 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);
+	sk = skb_steal_sock(skb, &refcounted);
 	if (sk) {
 		struct dst_entry *dst = skb_dst(skb);
 		int ret;
@@ -2322,7 +2323,8 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
 			udp_sk_rx_dst_set(sk, dst);
 
 		ret = udp_unicast_rcv_skb(sk, skb, uh);
-		sock_put(sk);
+		if (refcounted)
+			sock_put(sk);
 		return ret;
 	}
 
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 5dc439a391fe..7d4151747340 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -843,6 +843,7 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
 	struct net *net = dev_net(skb->dev);
 	struct udphdr *uh;
 	struct sock *sk;
+	bool refcounted;
 	u32 ulen = 0;
 
 	if (!pskb_may_pull(skb, sizeof(struct udphdr)))
@@ -879,7 +880,7 @@ 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);
+	sk = skb_steal_sock(skb, &refcounted);
 	if (sk) {
 		struct dst_entry *dst = skb_dst(skb);
 		int ret;
@@ -888,12 +889,14 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
 			udp6_sk_rx_dst_set(sk, dst);
 
 		if (!uh->check && !udp_sk(sk)->no_check6_rx) {
-			sock_put(sk);
+			if (refcounted)
+				sock_put(sk);
 			goto report_csum_error;
 		}
 
 		ret = udp6_unicast_rcv_skb(sk, skb, uh);
-		sock_put(sk);
+		if (refcounted)
+			sock_put(sk);
 		return ret;
 	}
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCHv3 bpf-next 3/5] bpf: Don't refcount LISTEN sockets in sk_assign()
  2020-03-27  4:25 [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper Joe Stringer
  2020-03-27  4:25 ` [PATCHv3 bpf-next 1/5] bpf: Add socket assign support Joe Stringer
  2020-03-27  4:25 ` [PATCHv3 bpf-next 2/5] net: Track socket refcounts in skb_steal_sock() Joe Stringer
@ 2020-03-27  4:25 ` Joe Stringer
  2020-03-27 14:26   ` Jamal Hadi Salim
  2020-03-27  4:25 ` [PATCHv3 bpf-next 4/5] selftests: bpf: add test for sk_assign Joe Stringer
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Joe Stringer @ 2020-03-27  4:25 UTC (permalink / raw)
  To: bpf; +Cc: netdev, daniel, ast, eric.dumazet, lmb, kafai

Avoid taking a reference on listen sockets by checking the socket type
in the sk_assign and in the corresponding skb_steal_sock() code in the
the transport layer, and by ensuring that the prefetch free (sock_pfree)
function uses the same logic to check whether the socket is refcounted.

Suggested-by: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: Joe Stringer <joe@wand.net.nz>
---
v3: No changes
v2: Initial version
---
 include/net/sock.h | 25 +++++++++++++++++--------
 net/core/filter.c  |  6 +++---
 net/core/sock.c    |  3 ++-
 3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 1ca2e808cb8e..3ec1865f173e 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2533,6 +2533,21 @@ skb_sk_is_prefetched(struct sk_buff *skb)
 	return skb->destructor == sock_pfree;
 }
 
+/* This helper checks if a socket is a full socket,
+ * ie _not_ a timewait or request socket.
+ */
+static inline bool sk_fullsock(const struct sock *sk)
+{
+	return (1 << sk->sk_state) & ~(TCPF_TIME_WAIT | TCPF_NEW_SYN_RECV);
+}
+
+static inline bool
+sk_is_refcounted(struct sock *sk)
+{
+	/* Only full sockets have sk->sk_flags. */
+	return !sk_fullsock(sk) || !sock_flag(sk, SOCK_RCU_FREE);
+}
+
 /**
  * skb_steal_sock
  * @skb to steal the socket from
@@ -2545,6 +2560,8 @@ skb_steal_sock(struct sk_buff *skb, bool *refcounted)
 		struct sock *sk = skb->sk;
 
 		*refcounted = true;
+		if (skb_sk_is_prefetched(skb))
+			*refcounted = sk_is_refcounted(sk);
 		skb->destructor = NULL;
 		skb->sk = NULL;
 		return sk;
@@ -2553,14 +2570,6 @@ skb_steal_sock(struct sk_buff *skb, bool *refcounted)
 	return NULL;
 }
 
-/* This helper checks if a socket is a full socket,
- * ie _not_ a timewait or request socket.
- */
-static inline bool sk_fullsock(const struct sock *sk)
-{
-	return (1 << sk->sk_state) & ~(TCPF_TIME_WAIT | TCPF_NEW_SYN_RECV);
-}
-
 /* Checks if this SKB belongs to an HW offloaded socket
  * and whether any SW fallbacks are required based on dev.
  * Check decrypted mark in case skb_orphan() cleared socket.
diff --git a/net/core/filter.c b/net/core/filter.c
index 2bcc29c1a595..fb73e6452c91 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5343,8 +5343,7 @@ static const struct bpf_func_proto bpf_sk_lookup_udp_proto = {
 
 BPF_CALL_1(bpf_sk_release, struct sock *, sk)
 {
-	/* Only full sockets have sk->sk_flags. */
-	if (!sk_fullsock(sk) || !sock_flag(sk, SOCK_RCU_FREE))
+	if (sk_is_refcounted(sk))
 		sock_gen_put(sk);
 	return 0;
 }
@@ -5870,7 +5869,8 @@ BPF_CALL_3(bpf_sk_assign, struct sk_buff *, skb, struct sock *, sk, u64, flags)
 		return -ENETUNREACH;
 	if (unlikely(sk->sk_reuseport))
 		return -ESOCKTNOSUPPORT;
-	if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
+	if (sk_is_refcounted(sk) &&
+	    unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
 		return -ENOENT;
 
 	skb_orphan(skb);
diff --git a/net/core/sock.c b/net/core/sock.c
index cfaf60267360..a2ab79446f59 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2076,7 +2076,8 @@ EXPORT_SYMBOL(sock_efree);
  */
 void sock_pfree(struct sk_buff *skb)
 {
-	sock_edemux(skb);
+	if (sk_is_refcounted(skb->sk))
+		sock_edemux(skb);
 }
 EXPORT_SYMBOL(sock_pfree);
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCHv3 bpf-next 4/5] selftests: bpf: add test for sk_assign
  2020-03-27  4:25 [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper Joe Stringer
                   ` (2 preceding siblings ...)
  2020-03-27  4:25 ` [PATCHv3 bpf-next 3/5] bpf: Don't refcount LISTEN sockets in sk_assign() Joe Stringer
@ 2020-03-27  4:25 ` Joe Stringer
  2020-03-27  4:25 ` [PATCHv3 bpf-next 5/5] selftests: bpf: Extend sk_assign tests for UDP Joe Stringer
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 21+ messages in thread
From: Joe Stringer @ 2020-03-27  4:25 UTC (permalink / raw)
  To: bpf; +Cc: Lorenz Bauer, netdev, daniel, ast, eric.dumazet, kafai

From: Lorenz Bauer <lmb@cloudflare.com>

Attach a tc direct-action classifier to lo in a fresh network
namespace, and rewrite all connection attempts to localhost:4321
to localhost:1234 (for port tests) and connections to unreachable
IPv4/IPv6 IPs to the local socket (for address tests). Includes
implementations for both TCP and UDP.

Keep in mind that both client to server and server to client traffic
passes the classifier.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Co-authored-by: Joe Stringer <joe@wand.net.nz>
Signed-off-by: Joe Stringer <joe@wand.net.nz>
---
v3: Add tests for UDP socket assign
    Fix switching back to original netns after test
    Avoid using signals to timeout connections
    Refactor to iterate through test cases
v2: Rebase onto test_progs infrastructure
v1: Initial version
---
 .../selftests/bpf/prog_tests/sk_assign.c      | 276 ++++++++++++++++++
 .../selftests/bpf/progs/test_sk_assign.c      | 143 +++++++++
 2 files changed, 419 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/sk_assign.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_sk_assign.c

diff --git a/tools/testing/selftests/bpf/prog_tests/sk_assign.c b/tools/testing/selftests/bpf/prog_tests/sk_assign.c
new file mode 100644
index 000000000000..25f17fe7d678
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/sk_assign.c
@@ -0,0 +1,276 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Facebook
+// Copyright (c) 2019 Cloudflare
+// Copyright (c) 2020 Isovalent, Inc.
+/*
+ * Test that the socket assign program is able to redirect traffic towards a
+ * socket, regardless of whether the port or address destination of the traffic
+ * matches the port.
+ */
+
+#define _GNU_SOURCE
+#include <fcntl.h>
+#include <signal.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include "test_progs.h"
+
+#define BIND_PORT 1234
+#define CONNECT_PORT 4321
+#define TEST_DADDR (0xC0A80203)
+#define NS_SELF "/proc/self/ns/net"
+
+static const struct timeval timeo_sec = { .tv_sec = 3 };
+static const size_t timeo_optlen = sizeof(timeo_sec);
+static int stop, duration;
+
+static bool
+configure_stack(void)
+{
+	char tc_cmd[BUFSIZ];
+
+	/* Move to a new networking namespace */
+	if (CHECK_FAIL(unshare(CLONE_NEWNET)))
+		return false;
+
+	/* Configure necessary links, routes */
+	if (CHECK_FAIL(system("ip link set dev lo up")))
+		return false;
+	if (CHECK_FAIL(system("ip route add local default dev lo")))
+		return false;
+	if (CHECK_FAIL(system("ip -6 route add local default dev lo")))
+		return false;
+
+	/* Load qdisc, BPF program */
+	if (CHECK_FAIL(system("tc qdisc add dev lo clsact")))
+		return false;
+	sprintf(tc_cmd, "%s %s %s %s", "tc filter add dev lo ingress bpf",
+		       "direct-action object-file ./test_sk_assign.o",
+		       "section classifier/sk_assign_test",
+		       (env.verbosity < VERBOSE_VERY) ? " 2>/dev/null" : "");
+	if (CHECK(system(tc_cmd), "BPF load failed;",
+		  "run with -vv for more info\n"))
+		return false;
+
+	return true;
+}
+
+static int
+start_server(const struct sockaddr *addr, socklen_t len, int type)
+{
+	int fd;
+
+	fd = socket(addr->sa_family, type, 0);
+	if (CHECK_FAIL(fd == -1))
+		goto out;
+	if (CHECK_FAIL(setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeo_sec,
+				  timeo_optlen)))
+		goto close_out;
+	if (CHECK_FAIL(bind(fd, addr, len) == -1))
+		goto close_out;
+	if (CHECK_FAIL(listen(fd, 128) == -1))
+		goto close_out;
+
+	goto out;
+close_out:
+	close(fd);
+	fd = -1;
+out:
+	return fd;
+}
+
+static int
+connect_to_server(const struct sockaddr *addr, socklen_t len, int type)
+{
+	int fd = -1;
+
+	fd = socket(addr->sa_family, type, 0);
+	if (CHECK_FAIL(fd == -1))
+		goto out;
+	if (CHECK_FAIL(setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &timeo_sec,
+				  timeo_optlen)))
+		goto close_out;
+	if (CHECK_FAIL(connect(fd, addr, len)))
+		goto close_out;
+
+	goto out;
+close_out:
+	close(fd);
+	fd = -1;
+out:
+	return fd;
+}
+
+static in_port_t
+get_port(int fd)
+{
+	struct sockaddr_storage ss;
+	socklen_t slen = sizeof(ss);
+	in_port_t port = 0;
+
+	if (CHECK_FAIL(getsockname(fd, (struct sockaddr *)&ss, &slen)))
+		return port;
+
+	switch (ss.ss_family) {
+	case AF_INET:
+		port = ((struct sockaddr_in *)&ss)->sin_port;
+		break;
+	case AF_INET6:
+		port = ((struct sockaddr_in6 *)&ss)->sin6_port;
+		break;
+	default:
+		CHECK(1, "Invalid address family", "%d\n", ss.ss_family);
+	}
+	return port;
+}
+
+static int
+run_test(int server_fd, const struct sockaddr *addr, socklen_t len, int type)
+{
+	int client = -1, srv_client = -1;
+	char buf[] = "testing";
+	in_port_t port;
+	int ret = 1;
+
+	client = connect_to_server(addr, len, type);
+	if (client == -1) {
+		perror("Cannot connect to server");
+		goto out;
+	}
+
+	srv_client = accept(server_fd, NULL, NULL);
+	if (CHECK_FAIL(srv_client == -1)) {
+		perror("Can't accept connection");
+		goto out;
+	}
+	if (CHECK_FAIL(write(client, buf, sizeof(buf)) != sizeof(buf))) {
+		perror("Can't write on client");
+		goto out;
+	}
+	if (CHECK_FAIL(read(srv_client, &buf, sizeof(buf)) != sizeof(buf))) {
+		perror("Can't read on server");
+		goto out;
+	}
+
+	port = get_port(srv_client);
+	if (CHECK_FAIL(!port))
+		goto out;
+	if (CHECK(port != htons(CONNECT_PORT), "Expected", "port %u but got %u",
+		  CONNECT_PORT, ntohs(port)))
+		goto out;
+
+	ret = 0;
+out:
+	close(client);
+	if (srv_client != server_fd)
+		close(srv_client);
+	if (ret)
+		WRITE_ONCE(stop, 1);
+	return ret;
+}
+
+static void
+prepare_addr(struct sockaddr *addr, int family, __u16 port, bool rewrite_addr)
+{
+	struct sockaddr_in *addr4;
+	struct sockaddr_in6 *addr6;
+
+	switch (family) {
+	case AF_INET:
+		addr4 = (struct sockaddr_in *)addr;
+		memset(addr4, 0, sizeof(*addr4));
+		addr4->sin_family = family;
+		addr4->sin_port = htons(port);
+		if (rewrite_addr)
+			addr4->sin_addr.s_addr = htonl(TEST_DADDR);
+		else
+			addr4->sin_addr.s_addr = htonl(INADDR_LOOPBACK);
+		break;
+	case AF_INET6:
+		addr6 = (struct sockaddr_in6 *)addr;
+		memset(addr6, 0, sizeof(*addr6));
+		addr6->sin6_family = family;
+		addr6->sin6_port = htons(port);
+		addr6->sin6_addr = in6addr_loopback;
+		if (rewrite_addr)
+			addr6->sin6_addr.s6_addr32[3] = htonl(TEST_DADDR);
+		break;
+	default:
+		fprintf(stderr, "Invalid family %d", family);
+	}
+}
+
+struct test_sk_cfg {
+	const char *name;
+	int family;
+	struct sockaddr *addr;
+	socklen_t len;
+	int type;
+	bool rewrite_addr;
+};
+
+#define TEST(NAME, FAMILY, TYPE, REWRITE)				\
+{									\
+	.name = NAME,							\
+	.family = FAMILY,						\
+	.addr = (FAMILY == AF_INET) ? (struct sockaddr *)&addr4		\
+				    : (struct sockaddr *)&addr6,	\
+	.len = (FAMILY == AF_INET) ? sizeof(addr4) : sizeof(addr6),	\
+	.type = TYPE,							\
+	.rewrite_addr = REWRITE,					\
+}
+
+void test_sk_assign(void)
+{
+	struct sockaddr_in addr4;
+	struct sockaddr_in6 addr6;
+	struct test_sk_cfg tests[] = {
+		TEST("ipv4 tcp port redir", AF_INET, SOCK_STREAM, false),
+		TEST("ipv4 tcp addr redir", AF_INET, SOCK_STREAM, true),
+		TEST("ipv6 tcp port redir", AF_INET6, SOCK_STREAM, false),
+		TEST("ipv6 tcp addr redir", AF_INET6, SOCK_STREAM, true),
+	};
+	int server = -1;
+	int self_net;
+
+	self_net = open(NS_SELF, O_RDONLY);
+	if (CHECK_FAIL(self_net < 0)) {
+		perror("Unable to open "NS_SELF);
+		return;
+	}
+
+	if (!configure_stack()) {
+		perror("configure_stack");
+		goto cleanup;
+	}
+
+	for (int i = 0; i < ARRAY_SIZE(tests) && !READ_ONCE(stop); i++) {
+		struct test_sk_cfg *test = &tests[i];
+		const struct sockaddr *addr;
+
+		if (!test__start_subtest(test->name))
+			continue;
+		prepare_addr(test->addr, test->family, BIND_PORT, false);
+		addr = (const struct sockaddr *)test->addr;
+		server = start_server(addr, test->len, test->type);
+		if (server == -1)
+			goto cleanup;
+
+		/* connect to unbound ports */
+		prepare_addr(test->addr, test->family, CONNECT_PORT,
+			     test->rewrite_addr);
+		if (run_test(server, addr, test->len, test->type))
+			goto close;
+
+		close(server);
+		server = -1;
+	}
+
+close:
+	close(server);
+cleanup:
+	if (CHECK_FAIL(setns(self_net, CLONE_NEWNET)))
+		perror("Failed to setns("NS_SELF")");
+	close(self_net);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_sk_assign.c b/tools/testing/selftests/bpf/progs/test_sk_assign.c
new file mode 100644
index 000000000000..bde8748799eb
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_sk_assign.c
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2019 Cloudflare Ltd.
+// Copyright (c) 2020 Isovalent, Inc.
+
+#include <stddef.h>
+#include <stdbool.h>
+#include <string.h>
+#include <linux/bpf.h>
+#include <linux/if_ether.h>
+#include <linux/in.h>
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <linux/pkt_cls.h>
+#include <linux/tcp.h>
+#include <sys/socket.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+
+int _version SEC("version") = 1;
+char _license[] SEC("license") = "GPL";
+
+/* Fill 'tuple' with L3 info, and attempt to find L4. On fail, return NULL. */
+static inline struct bpf_sock_tuple *
+get_tuple(struct __sk_buff *skb, bool *ipv4)
+{
+	void *data_end = (void *)(long)skb->data_end;
+	void *data = (void *)(long)skb->data;
+	struct bpf_sock_tuple *result;
+	struct ethhdr *eth;
+	__u64 tuple_len;
+	__u8 proto = 0;
+	__u64 ihl_len;
+
+	eth = (struct ethhdr *)(data);
+	if (eth + 1 > data_end)
+		return NULL;
+
+	if (eth->h_proto == bpf_htons(ETH_P_IP)) {
+		struct iphdr *iph = (struct iphdr *)(data + sizeof(*eth));
+
+		if (iph + 1 > data_end)
+			return NULL;
+		if (iph->ihl != 5)
+			/* Options are not supported */
+			return NULL;
+		ihl_len = iph->ihl * 4;
+		proto = iph->protocol;
+		*ipv4 = true;
+		result = (struct bpf_sock_tuple *)&iph->saddr;
+	} else if (eth->h_proto == bpf_htons(ETH_P_IPV6)) {
+		struct ipv6hdr *ip6h = (struct ipv6hdr *)(data + sizeof(*eth));
+
+		if (ip6h + 1 > data_end)
+			return NULL;
+		ihl_len = sizeof(*ip6h);
+		proto = ip6h->nexthdr;
+		*ipv4 = false;
+		result = (struct bpf_sock_tuple *)&ip6h->saddr;
+	} else {
+		return (struct bpf_sock_tuple *)data;
+	}
+
+	if (result + 1 > data_end || proto != IPPROTO_TCP)
+		return NULL;
+
+	return result;
+}
+
+static inline int
+handle_tcp(struct __sk_buff *skb, struct bpf_sock_tuple *tuple, bool ipv4)
+{
+	struct bpf_sock_tuple ln = {0};
+	struct bpf_sock *sk;
+	size_t tuple_len;
+	int ret;
+
+	tuple_len = ipv4 ? sizeof(tuple->ipv4) : sizeof(tuple->ipv6);
+	if ((void *)tuple + tuple_len > skb->data_end)
+		return TC_ACT_SHOT;
+
+	sk = bpf_skc_lookup_tcp(skb, tuple, tuple_len, BPF_F_CURRENT_NETNS, 0);
+	if (sk) {
+		if (sk->state != BPF_TCP_LISTEN)
+			goto assign;
+		bpf_sk_release(sk);
+	}
+
+	if (ipv4) {
+		if (tuple->ipv4.dport != bpf_htons(4321))
+			return TC_ACT_OK;
+
+		ln.ipv4.daddr = bpf_htonl(0x7f000001);
+		ln.ipv4.dport = bpf_htons(1234);
+
+		sk = bpf_skc_lookup_tcp(skb, &ln, sizeof(ln.ipv4),
+					BPF_F_CURRENT_NETNS, 0);
+	} else {
+		if (tuple->ipv6.dport != bpf_htons(4321))
+			return TC_ACT_OK;
+
+		/* Upper parts of daddr are already zero. */
+		ln.ipv6.daddr[3] = bpf_htonl(0x1);
+		ln.ipv6.dport = bpf_htons(1234);
+
+		sk = bpf_skc_lookup_tcp(skb, &ln, sizeof(ln.ipv6),
+					BPF_F_CURRENT_NETNS, 0);
+	}
+
+	/* workaround: We can't do a single socket lookup here, because then
+	 * the compiler will likely spill tuple_len to the stack. This makes it
+	 * lose all bounds information in the verifier, which then rejects the
+	 * call as unsafe.
+	 */
+	if (!sk)
+		return TC_ACT_SHOT;
+
+	if (sk->state != BPF_TCP_LISTEN) {
+		bpf_sk_release(sk);
+		return TC_ACT_SHOT;
+	}
+
+assign:
+	ret = bpf_sk_assign(skb, sk, 0);
+	bpf_sk_release(sk);
+	return ret;
+}
+
+SEC("classifier/sk_assign_test")
+int bpf_sk_assign_test(struct __sk_buff *skb)
+{
+	struct bpf_sock_tuple *tuple, ln = {0};
+	bool ipv4 = false;
+	int tuple_len;
+	int ret = 0;
+
+	tuple = get_tuple(skb, &ipv4);
+	if (!tuple)
+		return TC_ACT_SHOT;
+
+	ret = handle_tcp(skb, tuple, ipv4);
+
+	return ret == 0 ? TC_ACT_OK : TC_ACT_SHOT;
+}
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCHv3 bpf-next 5/5] selftests: bpf: Extend sk_assign tests for UDP
  2020-03-27  4:25 [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper Joe Stringer
                   ` (3 preceding siblings ...)
  2020-03-27  4:25 ` [PATCHv3 bpf-next 4/5] selftests: bpf: add test for sk_assign Joe Stringer
@ 2020-03-27  4:25 ` Joe Stringer
       [not found]   ` <CACAyw9-GOw5tkR8n6p7Kct9-wq4B-9ka-X8R2V8uZv8VWUY5UQ@mail.gmail.com>
  2020-03-27  5:02 ` [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper Alexei Starovoitov
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Joe Stringer @ 2020-03-27  4:25 UTC (permalink / raw)
  To: bpf; +Cc: netdev, daniel, ast, eric.dumazet, lmb, kafai

Add support for testing UDP sk_assign to the existing tests.

Signed-off-by: Joe Stringer <joe@wand.net.nz>
---
v3: Initial version
---
 .../selftests/bpf/prog_tests/sk_assign.c      | 47 +++++++++++--
 .../selftests/bpf/progs/test_sk_assign.c      | 69 +++++++++++++++++--
 2 files changed, 105 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sk_assign.c b/tools/testing/selftests/bpf/prog_tests/sk_assign.c
index 25f17fe7d678..d572e1a2c297 100644
--- a/tools/testing/selftests/bpf/prog_tests/sk_assign.c
+++ b/tools/testing/selftests/bpf/prog_tests/sk_assign.c
@@ -69,7 +69,7 @@ start_server(const struct sockaddr *addr, socklen_t len, int type)
 		goto close_out;
 	if (CHECK_FAIL(bind(fd, addr, len) == -1))
 		goto close_out;
-	if (CHECK_FAIL(listen(fd, 128) == -1))
+	if (type == SOCK_STREAM && CHECK_FAIL(listen(fd, 128) == -1))
 		goto close_out;
 
 	goto out;
@@ -125,6 +125,20 @@ get_port(int fd)
 	return port;
 }
 
+static ssize_t
+rcv_msg(int srv_client, int type)
+{
+	struct sockaddr_storage ss;
+	char buf[BUFSIZ];
+	socklen_t slen;
+
+	if (type == SOCK_STREAM)
+		return read(srv_client, &buf, sizeof(buf));
+	else
+		return recvfrom(srv_client, &buf, sizeof(buf), 0,
+				(struct sockaddr *)&ss, &slen);
+}
+
 static int
 run_test(int server_fd, const struct sockaddr *addr, socklen_t len, int type)
 {
@@ -139,16 +153,20 @@ run_test(int server_fd, const struct sockaddr *addr, socklen_t len, int type)
 		goto out;
 	}
 
-	srv_client = accept(server_fd, NULL, NULL);
-	if (CHECK_FAIL(srv_client == -1)) {
-		perror("Can't accept connection");
-		goto out;
+	if (type == SOCK_STREAM) {
+		srv_client = accept(server_fd, NULL, NULL);
+		if (CHECK_FAIL(srv_client == -1)) {
+			perror("Can't accept connection");
+			goto out;
+		}
+	} else {
+		srv_client = server_fd;
 	}
 	if (CHECK_FAIL(write(client, buf, sizeof(buf)) != sizeof(buf))) {
 		perror("Can't write on client");
 		goto out;
 	}
-	if (CHECK_FAIL(read(srv_client, &buf, sizeof(buf)) != sizeof(buf))) {
+	if (CHECK_FAIL(rcv_msg(srv_client, type) != sizeof(buf))) {
 		perror("Can't read on server");
 		goto out;
 	}
@@ -156,9 +174,20 @@ run_test(int server_fd, const struct sockaddr *addr, socklen_t len, int type)
 	port = get_port(srv_client);
 	if (CHECK_FAIL(!port))
 		goto out;
-	if (CHECK(port != htons(CONNECT_PORT), "Expected", "port %u but got %u",
+	/* SOCK_STREAM is connected via accept(), so the server's local address
+	 * will be the CONNECT_PORT rather than the BIND port that corresponds
+	 * to the listen socket. SOCK_DGRAM on the other hand is connectionless
+	 * so we can't really do the same check there; the server doesn't ever
+	 * create a socket with CONNECT_PORT.
+	 */
+	if (type == SOCK_STREAM &&
+	    CHECK(port != htons(CONNECT_PORT), "Expected", "port %u but got %u",
 		  CONNECT_PORT, ntohs(port)))
 		goto out;
+	else if (type == SOCK_DGRAM &&
+		 CHECK(port != htons(BIND_PORT), "Expected",
+		       "port %u but got %u", BIND_PORT, ntohs(port)))
+		goto out;
 
 	ret = 0;
 out:
@@ -230,6 +259,10 @@ void test_sk_assign(void)
 		TEST("ipv4 tcp addr redir", AF_INET, SOCK_STREAM, true),
 		TEST("ipv6 tcp port redir", AF_INET6, SOCK_STREAM, false),
 		TEST("ipv6 tcp addr redir", AF_INET6, SOCK_STREAM, true),
+		TEST("ipv4 udp port redir", AF_INET, SOCK_DGRAM, false),
+		TEST("ipv4 udp addr redir", AF_INET, SOCK_DGRAM, true),
+		TEST("ipv6 udp port redir", AF_INET6, SOCK_DGRAM, false),
+		TEST("ipv6 udp addr redir", AF_INET6, SOCK_DGRAM, true),
 	};
 	int server = -1;
 	int self_net;
diff --git a/tools/testing/selftests/bpf/progs/test_sk_assign.c b/tools/testing/selftests/bpf/progs/test_sk_assign.c
index bde8748799eb..99547dcaac12 100644
--- a/tools/testing/selftests/bpf/progs/test_sk_assign.c
+++ b/tools/testing/selftests/bpf/progs/test_sk_assign.c
@@ -21,7 +21,7 @@ char _license[] SEC("license") = "GPL";
 
 /* Fill 'tuple' with L3 info, and attempt to find L4. On fail, return NULL. */
 static inline struct bpf_sock_tuple *
-get_tuple(struct __sk_buff *skb, bool *ipv4)
+get_tuple(struct __sk_buff *skb, bool *ipv4, bool *tcp)
 {
 	void *data_end = (void *)(long)skb->data_end;
 	void *data = (void *)(long)skb->data;
@@ -60,12 +60,64 @@ get_tuple(struct __sk_buff *skb, bool *ipv4)
 		return (struct bpf_sock_tuple *)data;
 	}
 
-	if (result + 1 > data_end || proto != IPPROTO_TCP)
+	if (proto != IPPROTO_TCP && proto != IPPROTO_UDP)
 		return NULL;
 
+	*tcp = (proto == IPPROTO_TCP);
 	return result;
 }
 
+static inline int
+handle_udp(struct __sk_buff *skb, struct bpf_sock_tuple *tuple, bool ipv4)
+{
+	struct bpf_sock_tuple ln = {0};
+	struct bpf_sock *sk;
+	size_t tuple_len;
+	int ret;
+
+	tuple_len = ipv4 ? sizeof(tuple->ipv4) : sizeof(tuple->ipv6);
+	if ((void *)tuple + tuple_len > skb->data_end)
+		return TC_ACT_SHOT;
+
+	sk = bpf_sk_lookup_udp(skb, tuple, tuple_len, BPF_F_CURRENT_NETNS, 0);
+	if (sk)
+		goto assign;
+
+	if (ipv4) {
+		if (tuple->ipv4.dport != bpf_htons(4321))
+			return TC_ACT_OK;
+
+		ln.ipv4.daddr = bpf_htonl(0x7f000001);
+		ln.ipv4.dport = bpf_htons(1234);
+
+		sk = bpf_sk_lookup_udp(skb, &ln, sizeof(ln.ipv4),
+					BPF_F_CURRENT_NETNS, 0);
+	} else {
+		if (tuple->ipv6.dport != bpf_htons(4321))
+			return TC_ACT_OK;
+
+		/* Upper parts of daddr are already zero. */
+		ln.ipv6.daddr[3] = bpf_htonl(0x1);
+		ln.ipv6.dport = bpf_htons(1234);
+
+		sk = bpf_sk_lookup_udp(skb, &ln, sizeof(ln.ipv6),
+					BPF_F_CURRENT_NETNS, 0);
+	}
+
+	/* workaround: We can't do a single socket lookup here, because then
+	 * the compiler will likely spill tuple_len to the stack. This makes it
+	 * lose all bounds information in the verifier, which then rejects the
+	 * call as unsafe.
+	 */
+	if (!sk)
+		return TC_ACT_SHOT;
+
+assign:
+	ret = bpf_sk_assign(skb, sk, 0);
+	bpf_sk_release(sk);
+	return ret;
+}
+
 static inline int
 handle_tcp(struct __sk_buff *skb, struct bpf_sock_tuple *tuple, bool ipv4)
 {
@@ -130,14 +182,23 @@ int bpf_sk_assign_test(struct __sk_buff *skb)
 {
 	struct bpf_sock_tuple *tuple, ln = {0};
 	bool ipv4 = false;
+	bool tcp = false;
 	int tuple_len;
 	int ret = 0;
 
-	tuple = get_tuple(skb, &ipv4);
+	tuple = get_tuple(skb, &ipv4, &tcp);
 	if (!tuple)
 		return TC_ACT_SHOT;
 
-	ret = handle_tcp(skb, tuple, ipv4);
+	/* Note that the verifier socket return type for bpf_skc_lookup_tcp()
+	 * differs from bpf_sk_lookup_udp(), so even though the C-level type is
+	 * the same here, if we try to share the implementations they will
+	 * fail to verify because we're crossing pointer types.
+	 */
+	if (tcp)
+		ret = handle_tcp(skb, tuple, ipv4);
+	else
+		ret = handle_udp(skb, tuple, ipv4);
 
 	return ret == 0 ? TC_ACT_OK : TC_ACT_SHOT;
 }
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper
  2020-03-27  4:25 [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper Joe Stringer
                   ` (4 preceding siblings ...)
  2020-03-27  4:25 ` [PATCHv3 bpf-next 5/5] selftests: bpf: Extend sk_assign tests for UDP Joe Stringer
@ 2020-03-27  5:02 ` Alexei Starovoitov
  2020-03-27  5:42   ` Eric Dumazet
  2020-03-27 14:13 ` Jamal Hadi Salim
  2020-03-27 18:46 ` Martin KaFai Lau
  7 siblings, 1 reply; 21+ messages in thread
From: Alexei Starovoitov @ 2020-03-27  5:02 UTC (permalink / raw)
  To: Joe Stringer; +Cc: bpf, netdev, daniel, ast, eric.dumazet, lmb, kafai

On Thu, Mar 26, 2020 at 09:25:51PM -0700, Joe Stringer wrote:
> Introduce a new helper that allows assigning a previously-found socket
> to the skb as the packet is received towards the stack, to cause the
> stack to guide the packet towards that socket subject to local routing
> configuration. The intention is to support TProxy use cases more
> directly from eBPF programs attached at TC ingress, to simplify and
> streamline Linux stack configuration in scale environments with Cilium.

Thanks for the quick respin.
It builds. And tests are passing for me.
The lack of acks and reviewed-by is a bit concerning for such important feature.

Folks, please be more generous with acks :)
so we can apply it with more confidence.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper
  2020-03-27  5:02 ` [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper Alexei Starovoitov
@ 2020-03-27  5:42   ` Eric Dumazet
  0 siblings, 0 replies; 21+ messages in thread
From: Eric Dumazet @ 2020-03-27  5:42 UTC (permalink / raw)
  To: Alexei Starovoitov, Joe Stringer
  Cc: bpf, netdev, daniel, ast, eric.dumazet, lmb, kafai



On 3/26/20 10:02 PM, Alexei Starovoitov wrote:
> On Thu, Mar 26, 2020 at 09:25:51PM -0700, Joe Stringer wrote:
>> Introduce a new helper that allows assigning a previously-found socket
>> to the skb as the packet is received towards the stack, to cause the
>> stack to guide the packet towards that socket subject to local routing
>> configuration. The intention is to support TProxy use cases more
>> directly from eBPF programs attached at TC ingress, to simplify and
>> streamline Linux stack configuration in scale environments with Cilium.
> 
> Thanks for the quick respin.
> It builds. And tests are passing for me.
> The lack of acks and reviewed-by is a bit concerning for such important feature.
> 
> Folks, please be more generous with acks :)
> so we can apply it with more confidence.
> 

I can review this tomorrow morning, thanks.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper
  2020-03-27  4:25 [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper Joe Stringer
                   ` (5 preceding siblings ...)
  2020-03-27  5:02 ` [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper Alexei Starovoitov
@ 2020-03-27 14:13 ` Jamal Hadi Salim
  2020-03-27 17:43   ` Joe Stringer
  2020-03-27 18:46 ` Martin KaFai Lau
  7 siblings, 1 reply; 21+ messages in thread
From: Jamal Hadi Salim @ 2020-03-27 14:13 UTC (permalink / raw)
  To: Joe Stringer, bpf
  Cc: netdev, daniel, ast, eric.dumazet, lmb, kafai, Roman Mashak

On 2020-03-27 12:25 a.m., Joe Stringer wrote:
> Introduce a new helper that allows assigning a previously-found socket
> to the skb as the packet is received towards the stack, to cause the
> stack to guide the packet towards that socket subject to local routing
> configuration. The intention is to support TProxy use cases more
> directly from eBPF programs attached at TC ingress, to simplify and
> streamline Linux stack configuration in scale environments with Cilium.
> 
> Normally in ip{,6}_rcv_core(), the skb will be orphaned, dropping any
> existing socket reference associated with the skb. Existing tproxy
> implementations in netfilter get around this restriction by running the
> tproxy logic after ip_rcv_core() in the PREROUTING table. However, this
> is not an option for TC-based logic (including eBPF programs attached at
> TC ingress).
> 
> This series introduces the BPF helper bpf_sk_assign() to associate the
> socket with the skb on the ingress path as the packet is passed up the
> stack. The initial patch in the series simply takes a reference on the
> socket to ensure safety, but later patches relax this for listen
> sockets.
> 
> To ensure delivery to the relevant socket, we still consult the routing
> table, for full examples of how to configure see the tests in patch #5;
> the simplest form of the route would look like this:
> 
>    $ ip route add local default dev lo
> 

Trying to understand so if we can port our tc action (and upstream),
we would need to replicate:

  bpf_sk_assign() - invoked everytime we succeed finding the sk
  bpf_sk_release() - invoked everytime we are done processing the sk

Anything else i missed?

cheers,
jamal

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCHv3 bpf-next 3/5] bpf: Don't refcount LISTEN sockets in sk_assign()
  2020-03-27  4:25 ` [PATCHv3 bpf-next 3/5] bpf: Don't refcount LISTEN sockets in sk_assign() Joe Stringer
@ 2020-03-27 14:26   ` Jamal Hadi Salim
  2020-03-27 17:38     ` Joe Stringer
  0 siblings, 1 reply; 21+ messages in thread
From: Jamal Hadi Salim @ 2020-03-27 14:26 UTC (permalink / raw)
  To: Joe Stringer, bpf; +Cc: netdev, daniel, ast, eric.dumazet, lmb, kafai

On 2020-03-27 12:25 a.m., Joe Stringer wrote:
> BPF_CALL_1(bpf_sk_release, struct sock *, sk)
>   {
> -	/* Only full sockets have sk->sk_flags. */
> -	if (!sk_fullsock(sk) || !sock_flag(sk, SOCK_RCU_FREE))
> +	if (sk_is_refcounted(sk))
>   		sock_gen_put(sk);
>   	return 0;
>   }


Would it make sense to have both the bpf_sk_release and bpf_sk_assign()
centralized so we dont replicate the functionality in tc? Reduces
maintainance overhead.

Thanks for working on this!

cheers,
jamal

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCHv3 bpf-next 3/5] bpf: Don't refcount LISTEN sockets in sk_assign()
  2020-03-27 14:26   ` Jamal Hadi Salim
@ 2020-03-27 17:38     ` Joe Stringer
  2020-03-27 18:29       ` Jamal Hadi Salim
  0 siblings, 1 reply; 21+ messages in thread
From: Joe Stringer @ 2020-03-27 17:38 UTC (permalink / raw)
  To: Jamal Hadi Salim
  Cc: Joe Stringer, bpf, netdev, Daniel Borkmann, Alexei Starovoitov,
	Eric Dumazet, Lorenz Bauer, Martin KaFai Lau

On Fri, Mar 27, 2020 at 7:26 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On 2020-03-27 12:25 a.m., Joe Stringer wrote:
> > BPF_CALL_1(bpf_sk_release, struct sock *, sk)
> >   {
> > -     /* Only full sockets have sk->sk_flags. */
> > -     if (!sk_fullsock(sk) || !sock_flag(sk, SOCK_RCU_FREE))
> > +     if (sk_is_refcounted(sk))
> >               sock_gen_put(sk);
> >       return 0;
> >   }
>
>
> Would it make sense to have both the bpf_sk_release and bpf_sk_assign()
> centralized so we dont replicate the functionality in tc? Reduces
> maintainance overhead.

I think sock_pfree() steps in that direction, we would just need the
corresponding refactoring for sk_assign bits. Sounds like a good idea.

This shouldn't functionally affect this series, I'm happy to either
spin this into next revision of this series (if there's other
feedback), or send a followup refactor for this, or defer this to your
TC follow-up series that would consume the refactored functions.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper
  2020-03-27 14:13 ` Jamal Hadi Salim
@ 2020-03-27 17:43   ` Joe Stringer
  2020-03-27 18:34     ` Jamal Hadi Salim
  0 siblings, 1 reply; 21+ messages in thread
From: Joe Stringer @ 2020-03-27 17:43 UTC (permalink / raw)
  To: Jamal Hadi Salim
  Cc: Joe Stringer, bpf, netdev, Daniel Borkmann, Alexei Starovoitov,
	Eric Dumazet, Lorenz Bauer, Martin KaFai Lau, Roman Mashak

On Fri, Mar 27, 2020 at 7:14 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On 2020-03-27 12:25 a.m., Joe Stringer wrote:
> > Introduce a new helper that allows assigning a previously-found socket
> > to the skb as the packet is received towards the stack, to cause the
> > stack to guide the packet towards that socket subject to local routing
> > configuration. The intention is to support TProxy use cases more
> > directly from eBPF programs attached at TC ingress, to simplify and
> > streamline Linux stack configuration in scale environments with Cilium.
> >
> > Normally in ip{,6}_rcv_core(), the skb will be orphaned, dropping any
> > existing socket reference associated with the skb. Existing tproxy
> > implementations in netfilter get around this restriction by running the
> > tproxy logic after ip_rcv_core() in the PREROUTING table. However, this
> > is not an option for TC-based logic (including eBPF programs attached at
> > TC ingress).
> >
> > This series introduces the BPF helper bpf_sk_assign() to associate the
> > socket with the skb on the ingress path as the packet is passed up the
> > stack. The initial patch in the series simply takes a reference on the
> > socket to ensure safety, but later patches relax this for listen
> > sockets.
> >
> > To ensure delivery to the relevant socket, we still consult the routing
> > table, for full examples of how to configure see the tests in patch #5;
> > the simplest form of the route would look like this:
> >
> >    $ ip route add local default dev lo
> >
>
> Trying to understand so if we can port our tc action (and upstream),
> we would need to replicate:
>
>   bpf_sk_assign() - invoked everytime we succeed finding the sk
>   bpf_sk_release() - invoked everytime we are done processing the sk

The skb->destructor = sock_pfree() is the balanced other half of
bpf_sk_assign(), so you shouldn't need to explicitly call
bpf_sk_release() to handle the refcounting of the assigned socket.

The `bpf_sk_release()` pairs with BPF socket lookup, so if you already
have other socket lookup code handling the core tproxy logic (looking
up established, then looking up listen sockets with different tuple)
then you're presumably already handling that to avoid leaking
references.

I think that looking at the test_sk_assign.c BPF program in patch 4/5
should give you a good sense for what you'd need in the TC action
logic.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCHv3 bpf-next 3/5] bpf: Don't refcount LISTEN sockets in sk_assign()
  2020-03-27 17:38     ` Joe Stringer
@ 2020-03-27 18:29       ` Jamal Hadi Salim
  0 siblings, 0 replies; 21+ messages in thread
From: Jamal Hadi Salim @ 2020-03-27 18:29 UTC (permalink / raw)
  To: Joe Stringer
  Cc: bpf, netdev, Daniel Borkmann, Alexei Starovoitov, Eric Dumazet,
	Lorenz Bauer, Martin KaFai Lau

On 2020-03-27 1:38 p.m., Joe Stringer wrote:
> On Fri, Mar 27, 2020 at 7:26 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>

> 
> I think sock_pfree() steps in that direction, we would just need the
> corresponding refactoring for sk_assign bits. Sounds like a good idea.
> 
> This shouldn't functionally affect this series, I'm happy to either
> spin this into next revision of this series (if there's other
> feedback), or send a followup refactor for this, or defer this to your
> TC follow-up series that would consume the refactored functions.
> 

Does not affect this series - a followup would be great.

cheers,
jamal

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper
  2020-03-27 17:43   ` Joe Stringer
@ 2020-03-27 18:34     ` Jamal Hadi Salim
  0 siblings, 0 replies; 21+ messages in thread
From: Jamal Hadi Salim @ 2020-03-27 18:34 UTC (permalink / raw)
  To: Joe Stringer
  Cc: bpf, netdev, Daniel Borkmann, Alexei Starovoitov, Eric Dumazet,
	Lorenz Bauer, Martin KaFai Lau, Roman Mashak

On 2020-03-27 1:43 p.m., Joe Stringer wrote:
> On Fri, Mar 27, 2020 at 7:14 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>

[..]
>>
>> Trying to understand so if we can port our tc action (and upstream),
>> we would need to replicate:
>>
>>    bpf_sk_assign() - invoked everytime we succeed finding the sk
>>    bpf_sk_release() - invoked everytime we are done processing the sk
> 
> The skb->destructor = sock_pfree() is the balanced other half of
> bpf_sk_assign(), so you shouldn't need to explicitly call
> bpf_sk_release() to handle the refcounting of the assigned socket.
>

per other thread, I think once you factor out what those two functions
call into the kernel proper we will just call those same
things..

> The `bpf_sk_release()` pairs with BPF socket lookup, so if you already
> have other socket lookup code handling the core tproxy logic (looking
> up established, then looking up listen sockets with different tuple)
> then you're presumably already handling that to avoid leaking
> references.
> 

Yes, we have all that code already.

> I think that looking at the test_sk_assign.c BPF program in patch 4/5
> should give you a good sense for what you'd need in the TC action
> logic.

Seems like we are on track. Thanks again for working on this.

cheers,
jamal



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCHv3 bpf-next 1/5] bpf: Add socket assign support
  2020-03-27  4:25 ` [PATCHv3 bpf-next 1/5] bpf: Add socket assign support Joe Stringer
@ 2020-03-27 18:44   ` Martin KaFai Lau
  0 siblings, 0 replies; 21+ messages in thread
From: Martin KaFai Lau @ 2020-03-27 18:44 UTC (permalink / raw)
  To: Joe Stringer; +Cc: bpf, netdev, daniel, ast, eric.dumazet, lmb

On Thu, Mar 26, 2020 at 09:25:52PM -0700, Joe Stringer wrote:
> Add support for TPROXY via a new bpf helper, bpf_sk_assign().
> 
> This helper requires the BPF program to discover the socket via a call
> to bpf_sk*_lookup_*(), then pass this socket to the new helper. The
> helper takes its own reference to the socket in addition to any existing
> reference that may or may not currently be obtained for the duration of
> BPF processing. For the destination socket to receive the traffic, the
> traffic must be routed towards that socket via local route. The
> simplest example route is below, but in practice you may want to route
> traffic more narrowly (eg by CIDR):
> 
>   $ ip route add local default dev lo
> 
> This patch avoids trying to introduce an extra bit into the skb->sk, as
> that would require more invasive changes to all code interacting with
> the socket to ensure that the bit is handled correctly, such as all
> error-handling cases along the path from the helper in BPF through to
> the orphan path in the input. Instead, we opt to use the destructor
> variable to switch on the prefetch of the socket.
> 
> Signed-off-by: Joe Stringer <joe@wand.net.nz>
> ---
> v3: Check skb_sk_is_prefetched() in TC level redirect check
> v2: Use skb->destructor to determine socket prefetch usage instead of
>       introducing a new metadata_dst
>     Restrict socket assign to same netns as TC device
>     Restrict assigning reuseport sockets
>     Adjust commit wording
> v1: Initial version
> ---
>  include/net/sock.h             |  7 +++++++
>  include/uapi/linux/bpf.h       | 25 ++++++++++++++++++++++++-
>  net/core/filter.c              | 31 +++++++++++++++++++++++++++++++
>  net/core/sock.c                |  9 +++++++++
>  net/ipv4/ip_input.c            |  3 ++-
>  net/ipv6/ip6_input.c           |  3 ++-
>  net/sched/act_bpf.c            |  3 +++
>  tools/include/uapi/linux/bpf.h | 25 ++++++++++++++++++++++++-
>  8 files changed, 102 insertions(+), 4 deletions(-)
> 

[ ... ]

> diff --git a/net/core/sock.c b/net/core/sock.c
> index 0fc8937a7ff4..cfaf60267360 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2071,6 +2071,15 @@ void sock_efree(struct sk_buff *skb)
>  }
>  EXPORT_SYMBOL(sock_efree);
>  
> +/* Buffer destructor for prefetch/receive path where reference count may
> + * not be held, e.g. for listen sockets.
> + */
> +void sock_pfree(struct sk_buff *skb)
> +{
> +	sock_edemux(skb);
Nit. may be directly call sock_gen_put().

> +}
> +EXPORT_SYMBOL(sock_pfree);
> +

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCHv3 bpf-next 2/5] net: Track socket refcounts in skb_steal_sock()
  2020-03-27  4:25 ` [PATCHv3 bpf-next 2/5] net: Track socket refcounts in skb_steal_sock() Joe Stringer
@ 2020-03-27 18:45   ` Martin KaFai Lau
  0 siblings, 0 replies; 21+ messages in thread
From: Martin KaFai Lau @ 2020-03-27 18:45 UTC (permalink / raw)
  To: Joe Stringer; +Cc: bpf, netdev, daniel, ast, eric.dumazet, lmb

On Thu, Mar 26, 2020 at 09:25:53PM -0700, Joe Stringer wrote:
> Refactor the UDP/TCP handlers slightly to allow skb_steal_sock() to make
> the determination of whether the socket is reference counted in the case
> where it is prefetched by earlier logic such as early_demux or

> dst_sk_prefetch.
Left over comment from v1?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper
  2020-03-27  4:25 [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper Joe Stringer
                   ` (6 preceding siblings ...)
  2020-03-27 14:13 ` Jamal Hadi Salim
@ 2020-03-27 18:46 ` Martin KaFai Lau
  2020-03-27 21:05   ` Joe Stringer
  7 siblings, 1 reply; 21+ messages in thread
From: Martin KaFai Lau @ 2020-03-27 18:46 UTC (permalink / raw)
  To: Joe Stringer; +Cc: bpf, netdev, daniel, ast, eric.dumazet, lmb

On Thu, Mar 26, 2020 at 09:25:51PM -0700, Joe Stringer wrote:
> Introduce a new helper that allows assigning a previously-found socket
> to the skb as the packet is received towards the stack, to cause the
> stack to guide the packet towards that socket subject to local routing
> configuration. The intention is to support TProxy use cases more
> directly from eBPF programs attached at TC ingress, to simplify and
> streamline Linux stack configuration in scale environments with Cilium.
> 
> Normally in ip{,6}_rcv_core(), the skb will be orphaned, dropping any
> existing socket reference associated with the skb. Existing tproxy
> implementations in netfilter get around this restriction by running the
> tproxy logic after ip_rcv_core() in the PREROUTING table. However, this
> is not an option for TC-based logic (including eBPF programs attached at
> TC ingress).
> 
> This series introduces the BPF helper bpf_sk_assign() to associate the
> socket with the skb on the ingress path as the packet is passed up the
> stack. The initial patch in the series simply takes a reference on the
> socket to ensure safety, but later patches relax this for listen
> sockets.
> 
> To ensure delivery to the relevant socket, we still consult the routing
> table, for full examples of how to configure see the tests in patch #5;
> the simplest form of the route would look like this:
> 
>   $ ip route add local default dev lo
> 
> This series is laid out as follows:
> * Patch 1 extends the eBPF API to add sk_assign() and defines a new
>   socket free function to allow the later paths to understand when the
>   socket associated with the skb should be kept through receive.
> * Patches 2-3 optimize the receive path to avoid taking a reference on
>   listener sockets during receive.
> * Patches 4-5 extends the selftests with examples of the new
>   functionality and validation of correct behaviour.
> 
> Changes since v2:
> * Add selftests for UDP socket redirection
> * Drop the early demux optimization patch (defer for more testing)
> * Fix check for orphaning after TC act return
> * Tidy up the tests to clean up properly and be less noisy.
> 
> Changes since v1:
> * Replace the metadata_dst approach with using the skb->destructor to
>   determine whether the socket has been prefetched. This is much
>   simpler.
> * Avoid taking a reference on listener sockets during receive
> * Restrict assigning sockets across namespaces
> * Restrict assigning SO_REUSEPORT sockets
> * Fix cookie usage for socket dst check
> * Rebase the tests against test_progs infrastructure
> * Tidy up commit messages
lgtm.

Acked-by: Martin KaFai Lau <kafai@fb.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCHv3 bpf-next 5/5] selftests: bpf: Extend sk_assign tests for UDP
       [not found]   ` <CACAyw9-GOw5tkR8n6p7Kct9-wq4B-9ka-X8R2V8uZv8VWUY5UQ@mail.gmail.com>
@ 2020-03-27 19:37     ` Joe Stringer
  0 siblings, 0 replies; 21+ messages in thread
From: Joe Stringer @ 2020-03-27 19:37 UTC (permalink / raw)
  To: Lorenz Bauer; +Cc: bpf, netdev

On Fri, Mar 27, 2020 at 3:12 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> On Fri, 27 Mar 2020 at 04:26, Joe Stringer <joe@wand.net.nz> wrote:
> >
> > Add support for testing UDP sk_assign to the existing tests.
> >
> > Signed-off-by: Joe Stringer <joe@wand.net.nz>
>
> Thanks!
>
> Acked-by: Lorenz Bauer <lmb@cloudflare.com>

Thanks! I assume you meant to ack this on-list but looks like it just
made it to me individually :-)

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper
  2020-03-27 18:46 ` Martin KaFai Lau
@ 2020-03-27 21:05   ` Joe Stringer
  2020-03-28 17:25     ` Daniel Borkmann
  0 siblings, 1 reply; 21+ messages in thread
From: Joe Stringer @ 2020-03-27 21:05 UTC (permalink / raw)
  To: Martin KaFai Lau, Alexei Starovoitov
  Cc: Joe Stringer, bpf, netdev, Daniel Borkmann, Eric Dumazet, Lorenz Bauer

On Fri, Mar 27, 2020 at 11:46 AM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Thu, Mar 26, 2020 at 09:25:51PM -0700, Joe Stringer wrote:
> > Introduce a new helper that allows assigning a previously-found socket
> > to the skb as the packet is received towards the stack, to cause the
> > stack to guide the packet towards that socket subject to local routing
> > configuration. The intention is to support TProxy use cases more
> > directly from eBPF programs attached at TC ingress, to simplify and
> > streamline Linux stack configuration in scale environments with Cilium.
> >
> > Normally in ip{,6}_rcv_core(), the skb will be orphaned, dropping any
> > existing socket reference associated with the skb. Existing tproxy
> > implementations in netfilter get around this restriction by running the
> > tproxy logic after ip_rcv_core() in the PREROUTING table. However, this
> > is not an option for TC-based logic (including eBPF programs attached at
> > TC ingress).
> >
> > This series introduces the BPF helper bpf_sk_assign() to associate the
> > socket with the skb on the ingress path as the packet is passed up the
> > stack. The initial patch in the series simply takes a reference on the
> > socket to ensure safety, but later patches relax this for listen
> > sockets.
> >
> > To ensure delivery to the relevant socket, we still consult the routing
> > table, for full examples of how to configure see the tests in patch #5;
> > the simplest form of the route would look like this:
> >
> >   $ ip route add local default dev lo
> >
> > This series is laid out as follows:
> > * Patch 1 extends the eBPF API to add sk_assign() and defines a new
> >   socket free function to allow the later paths to understand when the
> >   socket associated with the skb should be kept through receive.
> > * Patches 2-3 optimize the receive path to avoid taking a reference on
> >   listener sockets during receive.
> > * Patches 4-5 extends the selftests with examples of the new
> >   functionality and validation of correct behaviour.
> >
> > Changes since v2:
> > * Add selftests for UDP socket redirection
> > * Drop the early demux optimization patch (defer for more testing)
> > * Fix check for orphaning after TC act return
> > * Tidy up the tests to clean up properly and be less noisy.
> >
> > Changes since v1:
> > * Replace the metadata_dst approach with using the skb->destructor to
> >   determine whether the socket has been prefetched. This is much
> >   simpler.
> > * Avoid taking a reference on listener sockets during receive
> > * Restrict assigning sockets across namespaces
> > * Restrict assigning SO_REUSEPORT sockets
> > * Fix cookie usage for socket dst check
> > * Rebase the tests against test_progs infrastructure
> > * Tidy up commit messages
> lgtm.
>
> Acked-by: Martin KaFai Lau <kafai@fb.com>

Thanks for the reviews!

I've rolled in the current nits + acks into the branch below, pending
any further feedback. Alexei, happy to respin this on the mailinglist
at some point if that's easier for you.

https://github.com/joestringer/linux/tree/submit/bpf-sk-assign-v3+

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper
  2020-03-27 21:05   ` Joe Stringer
@ 2020-03-28 17:25     ` Daniel Borkmann
  2020-03-28 17:42       ` Joe Stringer
  0 siblings, 1 reply; 21+ messages in thread
From: Daniel Borkmann @ 2020-03-28 17:25 UTC (permalink / raw)
  To: Joe Stringer, Martin KaFai Lau, Alexei Starovoitov
  Cc: bpf, netdev, Eric Dumazet, Lorenz Bauer

On 3/27/20 10:05 PM, Joe Stringer wrote:
> On Fri, Mar 27, 2020 at 11:46 AM Martin KaFai Lau <kafai@fb.com> wrote:
>>
>> On Thu, Mar 26, 2020 at 09:25:51PM -0700, Joe Stringer wrote:
>>> Introduce a new helper that allows assigning a previously-found socket
>>> to the skb as the packet is received towards the stack, to cause the
[...]
>>> Changes since v1:
>>> * Replace the metadata_dst approach with using the skb->destructor to
>>>    determine whether the socket has been prefetched. This is much
>>>    simpler.
>>> * Avoid taking a reference on listener sockets during receive
>>> * Restrict assigning sockets across namespaces
>>> * Restrict assigning SO_REUSEPORT sockets
>>> * Fix cookie usage for socket dst check
>>> * Rebase the tests against test_progs infrastructure
>>> * Tidy up commit messages
>> lgtm.
>>
>> Acked-by: Martin KaFai Lau <kafai@fb.com>
> 
> Thanks for the reviews!
> 
> I've rolled in the current nits + acks into the branch below, pending
> any further feedback. Alexei, happy to respin this on the mailinglist
> at some point if that's easier for you.
> 
> https://github.com/joestringer/linux/tree/submit/bpf-sk-assign-v3+

Please send the updated series to the list with Martin's ACK retained, so
that we can process the series through our patchwork scripts wrt formatting,
tags etc (please also make sure it's rebased).

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper
  2020-03-28 17:25     ` Daniel Borkmann
@ 2020-03-28 17:42       ` Joe Stringer
  0 siblings, 0 replies; 21+ messages in thread
From: Joe Stringer @ 2020-03-28 17:42 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Joe Stringer, Martin KaFai Lau, Alexei Starovoitov, bpf, netdev,
	Eric Dumazet, Lorenz Bauer

On Sat, Mar 28, 2020 at 10:26 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 3/27/20 10:05 PM, Joe Stringer wrote:
> > On Fri, Mar 27, 2020 at 11:46 AM Martin KaFai Lau <kafai@fb.com> wrote:
> >>
> >> On Thu, Mar 26, 2020 at 09:25:51PM -0700, Joe Stringer wrote:
> >>> Introduce a new helper that allows assigning a previously-found socket
> >>> to the skb as the packet is received towards the stack, to cause the
> [...]
> >>> Changes since v1:
> >>> * Replace the metadata_dst approach with using the skb->destructor to
> >>>    determine whether the socket has been prefetched. This is much
> >>>    simpler.
> >>> * Avoid taking a reference on listener sockets during receive
> >>> * Restrict assigning sockets across namespaces
> >>> * Restrict assigning SO_REUSEPORT sockets
> >>> * Fix cookie usage for socket dst check
> >>> * Rebase the tests against test_progs infrastructure
> >>> * Tidy up commit messages
> >> lgtm.
> >>
> >> Acked-by: Martin KaFai Lau <kafai@fb.com>
> >
> > Thanks for the reviews!
> >
> > I've rolled in the current nits + acks into the branch below, pending
> > any further feedback. Alexei, happy to respin this on the mailinglist
> > at some point if that's easier for you.
> >
> > https://github.com/joestringer/linux/tree/submit/bpf-sk-assign-v3+
>
> Please send the updated series to the list with Martin's ACK retained, so
> that we can process the series through our patchwork scripts wrt formatting,
> tags etc (please also make sure it's rebased).

Sure thing, will send it out soon.

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2020-03-28 17:42 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-27  4:25 [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper Joe Stringer
2020-03-27  4:25 ` [PATCHv3 bpf-next 1/5] bpf: Add socket assign support Joe Stringer
2020-03-27 18:44   ` Martin KaFai Lau
2020-03-27  4:25 ` [PATCHv3 bpf-next 2/5] net: Track socket refcounts in skb_steal_sock() Joe Stringer
2020-03-27 18:45   ` Martin KaFai Lau
2020-03-27  4:25 ` [PATCHv3 bpf-next 3/5] bpf: Don't refcount LISTEN sockets in sk_assign() Joe Stringer
2020-03-27 14:26   ` Jamal Hadi Salim
2020-03-27 17:38     ` Joe Stringer
2020-03-27 18:29       ` Jamal Hadi Salim
2020-03-27  4:25 ` [PATCHv3 bpf-next 4/5] selftests: bpf: add test for sk_assign Joe Stringer
2020-03-27  4:25 ` [PATCHv3 bpf-next 5/5] selftests: bpf: Extend sk_assign tests for UDP Joe Stringer
     [not found]   ` <CACAyw9-GOw5tkR8n6p7Kct9-wq4B-9ka-X8R2V8uZv8VWUY5UQ@mail.gmail.com>
2020-03-27 19:37     ` Joe Stringer
2020-03-27  5:02 ` [PATCHv3 bpf-next 0/5] Add bpf_sk_assign eBPF helper Alexei Starovoitov
2020-03-27  5:42   ` Eric Dumazet
2020-03-27 14:13 ` Jamal Hadi Salim
2020-03-27 17:43   ` Joe Stringer
2020-03-27 18:34     ` Jamal Hadi Salim
2020-03-27 18:46 ` Martin KaFai Lau
2020-03-27 21:05   ` Joe Stringer
2020-03-28 17:25     ` Daniel Borkmann
2020-03-28 17:42       ` Joe Stringer

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).