[PATCH bpf-next v9 00/12] net-timestamp: bpf extension to equip applications transparently

[PATCH bpf-next v9 00/12] net-timestamp: bpf extension to equip applications transparently

[Jason Xing]

"Timestamping is key to debugging network stack latency. With
SO_TIMESTAMPING, bugs that are otherwise incorrectly assumed to be
network issues can be attributed to the kernel." This is extracted
from the talk "SO_TIMESTAMPING: Powering Fleetwide RPC Monitoring"
addressed by Willem de Bruijn at netdevconf 0x17).

There are a few areas that need optimization with the consideration of
easier use and less performance impact, which I highlighted and mainly
discussed at netconf 2024 with Willem de Bruijn and John Fastabend:
uAPI compatibility, extra system call overhead, and the need for
application modification. I initially managed to solve these issues
by writing a kernel module that hooks various key functions. However,
this approach is not suitable for the next kernel release. Therefore,
a BPF extension was proposed. During recent period, Martin KaFai Lau
provides invaluable suggestions about BPF along the way. Many thanks
here!

In this series, only support foundamental codes and tx for TCP.
This approach mostly relies on existing SO_TIMESTAMPING feature, users
only needs to pass certain flags through bpf_setsocktopt() to a separate
tsflags. Please see the last selftest patch in this series.

---
v8
Link: https://lore.kernel.org/all/20250128084620.57547-1-kerneljasonxing@gmail.com/
1. adjust some commit messages and titles
2. add sk cookie in selftests
3. handle the NULL pointer in hwstamp
4. use kfunc to do selective sampling

v7
Link: https://lore.kernel.org/all/20250121012901.87763-1-kerneljasonxing@gmail.com/
1. target bpf-next tree
2. simplely and directly stop timestamping callbacks calling a few BPF
CALLS due to safety concern.
3. add more new testcases and adjust the existing testcases
4. revise some comments of new timestamping callbacks
5. remove a few BPF CGROUP locks

RFC v6
In the meantime, any suggestions and reviews are welcome!
Link: https://lore.kernel.org/all/20250112113748.73504-1-kerneljasonxing@gmail.com/
1. handle those safety problem by using the correct method.
2. support bpf_getsockopt.
3. adjust the position of BPF_SOCK_OPS_TS_TCP_SND_CB
4. fix mishandling the hardware timestamp error
5. add more corresponding tests

v5
Link: https://lore.kernel.org/all/20241207173803.90744-1-kerneljasonxing@gmail.com/
1. handle the safety issus when someone tries to call unrelated bpf
helpers.
2. avoid adding direct function call in the hot path like
__dev_queue_xmit()
3. remove reporting the hardware timestamp and tskey since they can be
fetched through the existing helper with the help of
bpf_skops_init_skb(), please see the selftest.
4. add new sendmsg callback in tcp_sendmsg, and introduce tskey_bpf used
by bpf program to correlate tcp_sendmsg with other hook points in patch [13/15].

v4
Link: https://lore.kernel.org/all/20241028110535.82999-1-kerneljasonxing@gmail.com/
1. introduce sk->sk_bpf_cb_flags to let user use bpf_setsockopt() (Martin)
2. introduce SKBTX_BPF to enable the bpf SO_TIMESTAMPING feature (Martin)
3. introduce bpf map in tests (Martin)
4. I choose to make this series as simple as possible, so I only support
most cases in the tx path for TCP protocol.

v3
Link: https://lore.kernel.org/all/20241012040651.95616-1-kerneljasonxing@gmail.com/
1. support UDP proto by introducing a new generation point.
2. for OPT_ID, introducing sk_tskey_bpf_offset to compute the delta
between the current socket key and bpf socket key. It is desiged for
UDP, which also applies to TCP.
3. support bpf_getsockopt()
4. use cgroup static key instead.
5. add one simple bpf selftest to show how it can be used.
6. remove the rx support from v2 because the number of patches could
exceed the limit of one series.

V2
Link: https://lore.kernel.org/all/20241008095109.99918-1-kerneljasonxing@gmail.com/
1. Introduce tsflag requestors so that we are able to extend more in the
future. Besides, it enables TX flags for bpf extension feature separately
without breaking users. It is suggested by Vadim Fedorenko.
2. introduce a static key to control the whole feature. (Willem)
3. Open the gate of bpf_setsockopt for the SO_TIMESTAMPING feature in
some TX/RX cases, not all the cases.

Jason Xing (12):
  bpf: add support for bpf_setsockopt()
  bpf: prepare for timestamping callbacks use
  bpf: stop unsafely accessing TCP fields in bpf callbacks
  bpf: stop calling some sock_op BPF CALLs in new timestamping callbacks
  net-timestamp: prepare for isolating two modes of SO_TIMESTAMPING
  bpf: support SCM_TSTAMP_SCHED of SO_TIMESTAMPING
  bpf: support sw SCM_TSTAMP_SND of SO_TIMESTAMPING
  bpf: support hw SCM_TSTAMP_SND of SO_TIMESTAMPING
  bpf: support SCM_TSTAMP_ACK of SO_TIMESTAMPING
  bpf: add a new callback in tcp_tx_timestamp()
  bpf: support selective sampling for bpf timestamping
  selftests/bpf: add simple bpf tests in the tx path for timestamping
    feature

 include/linux/filter.h                        |   1 +
 include/linux/skbuff.h                        |  12 +-
 include/net/sock.h                            |  10 +
 include/net/tcp.h                             |   5 +-
 include/uapi/linux/bpf.h                      |  30 ++
 kernel/bpf/btf.c                              |   1 +
 net/core/dev.c                                |   3 +-
 net/core/filter.c                             |  75 ++++-
 net/core/skbuff.c                             |  48 ++-
 net/core/sock.c                               |  15 +
 net/dsa/user.c                                |   2 +-
 net/ipv4/tcp.c                                |   4 +
 net/ipv4/tcp_input.c                          |   2 +
 net/ipv4/tcp_output.c                         |   2 +
 net/socket.c                                  |   2 +-
 tools/include/uapi/linux/bpf.h                |  23 ++
 .../bpf/prog_tests/so_timestamping.c          |  79 +++++
 .../selftests/bpf/progs/so_timestamping.c     | 312 ++++++++++++++++++
 18 files changed, 612 insertions(+), 14 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/so_timestamping.c
 create mode 100644 tools/testing/selftests/bpf/progs/so_timestamping.c

-- 
2.43.5

[PATCH bpf-next v9 01/12] bpf: add support for bpf_setsockopt()

[Jason Xing]

Users can write the following code to enable the bpf extension:
int flags = SK_BPF_CB_TX_TIMESTAMPING;
int opts = SK_BPF_CB_FLAGS;
bpf_setsockopt(skops, SOL_SOCKET, opts, &flags, sizeof(flags));

Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 include/net/sock.h             |  3 +++
 include/uapi/linux/bpf.h       |  8 ++++++++
 net/core/filter.c              | 23 +++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h |  1 +
 4 files changed, 35 insertions(+)

diff --git a/include/net/sock.h b/include/net/sock.h
index 8036b3b79cd8..7916982343c6 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -303,6 +303,7 @@ struct sk_filter;
   *	@sk_stamp: time stamp of last packet received
   *	@sk_stamp_seq: lock for accessing sk_stamp on 32 bit architectures only
   *	@sk_tsflags: SO_TIMESTAMPING flags
+  *	@sk_bpf_cb_flags: used in bpf_setsockopt()
   *	@sk_use_task_frag: allow sk_page_frag() to use current->task_frag.
   *			   Sockets that can be used under memory reclaim should
   *			   set this to false.
@@ -445,6 +446,8 @@ struct sock {
 	u32			sk_reserved_mem;
 	int			sk_forward_alloc;
 	u32			sk_tsflags;
+#define SK_BPF_CB_FLAG_TEST(SK, FLAG) ((SK)->sk_bpf_cb_flags & (FLAG))
+	u32			sk_bpf_cb_flags;
 	__cacheline_group_end(sock_write_rxtx);
 
 	__cacheline_group_begin(sock_write_tx);
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 2acf9b336371..6116eb3d1515 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -6913,6 +6913,13 @@ enum {
 	BPF_SOCK_OPS_ALL_CB_FLAGS       = 0x7F,
 };
 
+/* Definitions for bpf_sk_cb_flags */
+enum {
+	SK_BPF_CB_TX_TIMESTAMPING	= 1<<0,
+	SK_BPF_CB_MASK			= (SK_BPF_CB_TX_TIMESTAMPING - 1) |
+					   SK_BPF_CB_TX_TIMESTAMPING
+};
+
 /* List of known BPF sock_ops operators.
  * New entries can only be added at the end
  */
@@ -7091,6 +7098,7 @@ enum {
 	TCP_BPF_SYN_IP		= 1006, /* Copy the IP[46] and TCP header */
 	TCP_BPF_SYN_MAC         = 1007, /* Copy the MAC, IP[46], and TCP header */
 	TCP_BPF_SOCK_OPS_CB_FLAGS = 1008, /* Get or Set TCP sock ops flags */
+	SK_BPF_CB_FLAGS		= 1009, /* Used to set socket bpf flags */
 };
 
 enum {
diff --git a/net/core/filter.c b/net/core/filter.c
index 2ec162dd83c4..1c6c07507a78 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5222,6 +5222,25 @@ static const struct bpf_func_proto bpf_get_socket_uid_proto = {
 	.arg1_type      = ARG_PTR_TO_CTX,
 };
 
+static int sk_bpf_set_get_cb_flags(struct sock *sk, char *optval, bool getopt)
+{
+	u32 sk_bpf_cb_flags;
+
+	if (getopt) {
+		*(u32 *)optval = sk->sk_bpf_cb_flags;
+		return 0;
+	}
+
+	sk_bpf_cb_flags = *(u32 *)optval;
+
+	if (sk_bpf_cb_flags & ~SK_BPF_CB_MASK)
+		return -EINVAL;
+
+	sk->sk_bpf_cb_flags = sk_bpf_cb_flags;
+
+	return 0;
+}
+
 static int sol_socket_sockopt(struct sock *sk, int optname,
 			      char *optval, int *optlen,
 			      bool getopt)
@@ -5238,6 +5257,7 @@ static int sol_socket_sockopt(struct sock *sk, int optname,
 	case SO_MAX_PACING_RATE:
 	case SO_BINDTOIFINDEX:
 	case SO_TXREHASH:
+	case SK_BPF_CB_FLAGS:
 		if (*optlen != sizeof(int))
 			return -EINVAL;
 		break;
@@ -5247,6 +5267,9 @@ static int sol_socket_sockopt(struct sock *sk, int optname,
 		return -EINVAL;
 	}
 
+	if (optname == SK_BPF_CB_FLAGS)
+		return sk_bpf_set_get_cb_flags(sk, optval, getopt);
+
 	if (getopt) {
 		if (optname == SO_BINDTODEVICE)
 			return -EINVAL;
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 2acf9b336371..70366f74ef4e 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -7091,6 +7091,7 @@ enum {
 	TCP_BPF_SYN_IP		= 1006, /* Copy the IP[46] and TCP header */
 	TCP_BPF_SYN_MAC         = 1007, /* Copy the MAC, IP[46], and TCP header */
 	TCP_BPF_SOCK_OPS_CB_FLAGS = 1008, /* Get or Set TCP sock ops flags */
+	SK_BPF_CB_FLAGS		= 1009, /* Used to set socket bpf flags */
 };
 
 enum {
-- 
2.43.5

[PATCH bpf-next v9 02/12] bpf: prepare for timestamping callbacks use

[Jason Xing]

Later, four callback points to report information to user space
based on this patch will be introduced.

As to skb initialization here, users can follow these three steps
as below to fetch the shared info from the exported skb in the bpf
prog:
1. skops_kern = bpf_cast_to_kern_ctx(skops);
2. skb = skops_kern->skb;
3. shinfo = bpf_core_cast(skb->head + skb->end, struct skb_shared_info);

More details can be seen in the last selftest patch of the series.

Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 include/net/sock.h |  7 +++++++
 net/core/sock.c    | 15 +++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/include/net/sock.h b/include/net/sock.h
index 7916982343c6..6f4d54faba92 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2923,6 +2923,13 @@ int sock_set_timestamping(struct sock *sk, int optname,
 			  struct so_timestamping timestamping);
 
 void sock_enable_timestamps(struct sock *sk);
+#if defined(CONFIG_CGROUP_BPF)
+void bpf_skops_tx_timestamping(struct sock *sk, struct sk_buff *skb, int op);
+#else
+static inline void bpf_skops_tx_timestamping(struct sock *sk, struct sk_buff *skb, int op)
+{
+}
+#endif
 void sock_no_linger(struct sock *sk);
 void sock_set_keepalive(struct sock *sk);
 void sock_set_priority(struct sock *sk, u32 priority);
diff --git a/net/core/sock.c b/net/core/sock.c
index eae2ae70a2e0..41db6407e360 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -948,6 +948,21 @@ int sock_set_timestamping(struct sock *sk, int optname,
 	return 0;
 }
 
+#if defined(CONFIG_CGROUP_BPF)
+void bpf_skops_tx_timestamping(struct sock *sk, struct sk_buff *skb, int op)
+{
+	struct bpf_sock_ops_kern sock_ops;
+
+	memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp));
+	sock_ops.op = op;
+	sock_ops.is_fullsock = 1;
+	sock_ops.sk = sk;
+	bpf_skops_init_skb(&sock_ops, skb, 0);
+	/* Timestamping bpf extension supports only TCP and UDP full socket */
+	__cgroup_bpf_run_filter_sock_ops(sk, &sock_ops, CGROUP_SOCK_OPS);
+}
+#endif
+
 void sock_set_keepalive(struct sock *sk)
 {
 	lock_sock(sk);
-- 
2.43.5

[PATCH bpf-next v9 03/12] bpf: stop unsafely accessing TCP fields in bpf callbacks

[Jason Xing]

The "is_locked_tcp_sock" flag is added to indicate that the callback
site has a tcp_sock locked.

Apply the new member is_locked_tcp_sock in the existing callbacks
where is_fullsock is set to 1 can stop UDP socket accessing struct
tcp_sock and stop TCP socket without sk lock protecting does the
similar thing, or else it could be catastrophe leading to panic.

To keep it simple, instead of distinguishing between read and write
access, users aren't allowed all read/write access to the tcp_sock
through the older bpf_sock_ops ctx. The new timestamping callbacks
can use newer helpers to read everything from a sk (e.g. bpf_core_cast),
so nothing is lost.

Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 include/linux/filter.h | 1 +
 include/net/tcp.h      | 1 +
 net/core/filter.c      | 8 ++++----
 net/ipv4/tcp_input.c   | 2 ++
 net/ipv4/tcp_output.c  | 2 ++
 5 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index a3ea46281595..d36d5d5180b1 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1508,6 +1508,7 @@ struct bpf_sock_ops_kern {
 	void	*skb_data_end;
 	u8	op;
 	u8	is_fullsock;
+	u8	is_locked_tcp_sock;
 	u8	remaining_opt_len;
 	u64	temp;			/* temp and everything after is not
 					 * initialized to 0 before calling
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 5b2b04835688..4c4dca59352b 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2649,6 +2649,7 @@ static inline int tcp_call_bpf(struct sock *sk, int op, u32 nargs, u32 *args)
 	memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp));
 	if (sk_fullsock(sk)) {
 		sock_ops.is_fullsock = 1;
+		sock_ops.is_locked_tcp_sock = 1;
 		sock_owned_by_me(sk);
 	}
 
diff --git a/net/core/filter.c b/net/core/filter.c
index 1c6c07507a78..8631036f6b64 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -10381,10 +10381,10 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
 		}							      \
 		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(			      \
 						struct bpf_sock_ops_kern,     \
-						is_fullsock),		      \
+						is_locked_tcp_sock),	      \
 				      fullsock_reg, si->src_reg,	      \
 				      offsetof(struct bpf_sock_ops_kern,      \
-					       is_fullsock));		      \
+					       is_locked_tcp_sock));	      \
 		*insn++ = BPF_JMP_IMM(BPF_JEQ, fullsock_reg, 0, jmp);	      \
 		if (si->dst_reg == si->src_reg)				      \
 			*insn++ = BPF_LDX_MEM(BPF_DW, reg, si->src_reg,	      \
@@ -10469,10 +10469,10 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
 					       temp));			      \
 		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(			      \
 						struct bpf_sock_ops_kern,     \
-						is_fullsock),		      \
+						is_locked_tcp_sock),	      \
 				      reg, si->dst_reg,			      \
 				      offsetof(struct bpf_sock_ops_kern,      \
-					       is_fullsock));		      \
+					       is_locked_tcp_sock));	      \
 		*insn++ = BPF_JMP_IMM(BPF_JEQ, reg, 0, 2);		      \
 		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(			      \
 						struct bpf_sock_ops_kern, sk),\
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index eb82e01da911..95733dcdfb4b 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -169,6 +169,7 @@ static void bpf_skops_parse_hdr(struct sock *sk, struct sk_buff *skb)
 	memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp));
 	sock_ops.op = BPF_SOCK_OPS_PARSE_HDR_OPT_CB;
 	sock_ops.is_fullsock = 1;
+	sock_ops.is_locked_tcp_sock = 1;
 	sock_ops.sk = sk;
 	bpf_skops_init_skb(&sock_ops, skb, tcp_hdrlen(skb));
 
@@ -185,6 +186,7 @@ static void bpf_skops_established(struct sock *sk, int bpf_op,
 	memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp));
 	sock_ops.op = bpf_op;
 	sock_ops.is_fullsock = 1;
+	sock_ops.is_locked_tcp_sock = 1;
 	sock_ops.sk = sk;
 	/* sk with TCP_REPAIR_ON does not have skb in tcp_finish_connect */
 	if (skb)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 0e5b9a654254..75e935ec7916 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -522,6 +522,7 @@ static void bpf_skops_hdr_opt_len(struct sock *sk, struct sk_buff *skb,
 		sock_owned_by_me(sk);
 
 		sock_ops.is_fullsock = 1;
+		sock_ops.is_locked_tcp_sock = 1;
 		sock_ops.sk = sk;
 	}
 
@@ -567,6 +568,7 @@ static void bpf_skops_write_hdr_opt(struct sock *sk, struct sk_buff *skb,
 		sock_owned_by_me(sk);
 
 		sock_ops.is_fullsock = 1;
+		sock_ops.is_locked_tcp_sock = 1;
 		sock_ops.sk = sk;
 	}
 
-- 
2.43.5

[PATCH bpf-next v9 04/12] bpf: stop calling some sock_op BPF CALLs in new timestamping callbacks

[Jason Xing]

Considering the potential invalid access issues, calling
bpf_sock_ops_setsockopt/getsockopt, bpf_sock_ops_cb_flags_set,
and the bpf_sock_ops_load_hdr_opt in the new timestamping
callbacks will return -EOPNOTSUPP error value.

It also prevents the UDP socket trying to access TCP fields in
the bpf extension for SO_TIMESTAMPING for the same consideration.

Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 net/core/filter.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index 8631036f6b64..7f56d0bbeb00 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5523,6 +5523,11 @@ static int __bpf_setsockopt(struct sock *sk, int level, int optname,
 	return -EINVAL;
 }
 
+static bool is_locked_tcp_sock_ops(struct bpf_sock_ops_kern *bpf_sock)
+{
+	return bpf_sock->op <= BPF_SOCK_OPS_WRITE_HDR_OPT_CB;
+}
+
 static int _bpf_setsockopt(struct sock *sk, int level, int optname,
 			   char *optval, int optlen)
 {
@@ -5673,6 +5678,9 @@ static const struct bpf_func_proto bpf_sock_addr_getsockopt_proto = {
 BPF_CALL_5(bpf_sock_ops_setsockopt, struct bpf_sock_ops_kern *, bpf_sock,
 	   int, level, int, optname, char *, optval, int, optlen)
 {
+	if (!is_locked_tcp_sock_ops(bpf_sock))
+		return -EOPNOTSUPP;
+
 	return _bpf_setsockopt(bpf_sock->sk, level, optname, optval, optlen);
 }
 
@@ -5758,6 +5766,9 @@ static int bpf_sock_ops_get_syn(struct bpf_sock_ops_kern *bpf_sock,
 BPF_CALL_5(bpf_sock_ops_getsockopt, struct bpf_sock_ops_kern *, bpf_sock,
 	   int, level, int, optname, char *, optval, int, optlen)
 {
+	if (!is_locked_tcp_sock_ops(bpf_sock))
+		return -EOPNOTSUPP;
+
 	if (IS_ENABLED(CONFIG_INET) && level == SOL_TCP &&
 	    optname >= TCP_BPF_SYN && optname <= TCP_BPF_SYN_MAC) {
 		int ret, copy_len = 0;
@@ -5800,6 +5811,9 @@ BPF_CALL_2(bpf_sock_ops_cb_flags_set, struct bpf_sock_ops_kern *, bpf_sock,
 	struct sock *sk = bpf_sock->sk;
 	int val = argval & BPF_SOCK_OPS_ALL_CB_FLAGS;
 
+	if (!is_locked_tcp_sock_ops(bpf_sock))
+		return -EOPNOTSUPP;
+
 	if (!IS_ENABLED(CONFIG_INET) || !sk_fullsock(sk))
 		return -EINVAL;
 
@@ -7609,6 +7623,9 @@ BPF_CALL_4(bpf_sock_ops_load_hdr_opt, struct bpf_sock_ops_kern *, bpf_sock,
 	u8 search_kind, search_len, copy_len, magic_len;
 	int ret;
 
+	if (!is_locked_tcp_sock_ops(bpf_sock))
+		return -EOPNOTSUPP;
+
 	/* 2 byte is the minimal option len except TCPOPT_NOP and
 	 * TCPOPT_EOL which are useless for the bpf prog to learn
 	 * and this helper disallow loading them also.
-- 
2.43.5

[PATCH bpf-next v9 05/12] net-timestamp: prepare for isolating two modes of SO_TIMESTAMPING

[Jason Xing]

No functional changes here, only add test to see if the orig_skb
matches the usage of application SO_TIMESTAMPING. And it's good to
support two modes in parallel later in this series.

Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 net/core/skbuff.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index a441613a1e6c..46530d516909 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5539,18 +5539,37 @@ void skb_complete_tx_timestamp(struct sk_buff *skb,
 }
 EXPORT_SYMBOL_GPL(skb_complete_tx_timestamp);
 
+static bool skb_tstamp_tx_report_so_timestamping(struct sk_buff *skb,
+						 int tstype, bool sw)
+{
+	switch (tstype) {
+	case SCM_TSTAMP_SCHED:
+		return skb_shinfo(skb)->tx_flags & SKBTX_SCHED_TSTAMP;
+	case SCM_TSTAMP_SND:
+		return skb_shinfo(skb)->tx_flags & (sw ? SKBTX_SW_TSTAMP :
+						    SKBTX_HW_TSTAMP);
+	case SCM_TSTAMP_ACK:
+		return TCP_SKB_CB(skb)->txstamp_ack;
+	}
+
+	return false;
+}
+
 void __skb_tstamp_tx(struct sk_buff *orig_skb,
 		     const struct sk_buff *ack_skb,
 		     struct skb_shared_hwtstamps *hwtstamps,
 		     struct sock *sk, int tstype)
 {
+	bool tsonly, opt_stats = false, sw = hwtstamps ? false : true;
 	struct sk_buff *skb;
-	bool tsonly, opt_stats = false;
 	u32 tsflags;
 
 	if (!sk)
 		return;
 
+	if (!skb_tstamp_tx_report_so_timestamping(orig_skb, tstype, sw))
+		return;
+
 	tsflags = READ_ONCE(sk->sk_tsflags);
 	if (!hwtstamps && !(tsflags & SOF_TIMESTAMPING_OPT_TX_SWHW) &&
 	    skb_shinfo(orig_skb)->tx_flags & SKBTX_IN_PROGRESS)
-- 
2.43.5

[PATCH bpf-next v9 06/12] bpf: support SCM_TSTAMP_SCHED of SO_TIMESTAMPING

[Jason Xing]

Support SCM_TSTAMP_SCHED case. Introduce SKBTX_BPF used as
an indicator telling us whether the skb should be traced
by the bpf prog.

Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 include/linux/skbuff.h         |  6 +++++-
 include/uapi/linux/bpf.h       |  4 ++++
 net/core/dev.c                 |  3 ++-
 net/core/skbuff.c              | 20 ++++++++++++++++++++
 tools/include/uapi/linux/bpf.h |  4 ++++
 5 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index bb2b751d274a..52f6e033e704 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -489,10 +489,14 @@ enum {
 
 	/* generate software time stamp when entering packet scheduling */
 	SKBTX_SCHED_TSTAMP = 1 << 6,
+
+	/* used for bpf extension when a bpf program is loaded */
+	SKBTX_BPF = 1 << 7,
 };
 
 #define SKBTX_ANY_SW_TSTAMP	(SKBTX_SW_TSTAMP    | \
-				 SKBTX_SCHED_TSTAMP)
+				 SKBTX_SCHED_TSTAMP | \
+				 SKBTX_BPF)
 #define SKBTX_ANY_TSTAMP	(SKBTX_HW_TSTAMP | \
 				 SKBTX_HW_TSTAMP_USE_CYCLES | \
 				 SKBTX_ANY_SW_TSTAMP)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 6116eb3d1515..30d2c078966b 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -7032,6 +7032,10 @@ enum {
 					 * by the kernel or the
 					 * earlier bpf-progs.
 					 */
+	BPF_SOCK_OPS_TS_SCHED_OPT_CB,	/* Called when skb is passing through
+					 * dev layer when SK_BPF_CB_TX_TIMESTAMPING
+					 * feature is on.
+					 */
 };
 
 /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
diff --git a/net/core/dev.c b/net/core/dev.c
index afa2282f2604..d57946c96511 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4500,7 +4500,8 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev)
 	skb_reset_mac_header(skb);
 	skb_assert_len(skb);
 
-	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_SCHED_TSTAMP))
+	if (unlikely(skb_shinfo(skb)->tx_flags &
+		     (SKBTX_SCHED_TSTAMP | SKBTX_BPF)))
 		__skb_tstamp_tx(skb, NULL, NULL, skb->sk, SCM_TSTAMP_SCHED);
 
 	/* Disable soft irqs for various locks below. Also
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 46530d516909..6f55eb90a632 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5555,6 +5555,23 @@ static bool skb_tstamp_tx_report_so_timestamping(struct sk_buff *skb,
 	return false;
 }
 
+static void skb_tstamp_tx_report_bpf_timestamping(struct sk_buff *skb,
+						  struct sock *sk,
+						  int tstype)
+{
+	int op;
+
+	switch (tstype) {
+	case SCM_TSTAMP_SCHED:
+		op = BPF_SOCK_OPS_TS_SCHED_OPT_CB;
+		break;
+	default:
+		return;
+	}
+
+	bpf_skops_tx_timestamping(sk, skb, op);
+}
+
 void __skb_tstamp_tx(struct sk_buff *orig_skb,
 		     const struct sk_buff *ack_skb,
 		     struct skb_shared_hwtstamps *hwtstamps,
@@ -5567,6 +5584,9 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
 	if (!sk)
 		return;
 
+	if (skb_shinfo(orig_skb)->tx_flags & SKBTX_BPF)
+		skb_tstamp_tx_report_bpf_timestamping(orig_skb, sk, tstype);
+
 	if (!skb_tstamp_tx_report_so_timestamping(orig_skb, tstype, sw))
 		return;
 
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 70366f74ef4e..eed91b7296b7 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -7025,6 +7025,10 @@ enum {
 					 * by the kernel or the
 					 * earlier bpf-progs.
 					 */
+	BPF_SOCK_OPS_TS_SCHED_OPT_CB,	/* Called when skb is passing through
+					 * dev layer when SK_BPF_CB_TX_TIMESTAMPING
+					 * feature is on.
+					 */
 };
 
 /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
-- 
2.43.5

[PATCH bpf-next v9 07/12] bpf: support sw SCM_TSTAMP_SND of SO_TIMESTAMPING

[Jason Xing]

Support sw SCM_TSTAMP_SND case. Then users will get the software
timestamp when the driver is about to send the skb. Later,
the hardware timestamp will be supported.

Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 include/linux/skbuff.h         |  2 +-
 include/uapi/linux/bpf.h       |  4 ++++
 net/core/skbuff.c              | 10 ++++++++--
 tools/include/uapi/linux/bpf.h |  4 ++++
 4 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 52f6e033e704..76582500c5ea 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4568,7 +4568,7 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
 static inline void skb_tx_timestamp(struct sk_buff *skb)
 {
 	skb_clone_tx_timestamp(skb);
-	if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP)
+	if (skb_shinfo(skb)->tx_flags & (SKBTX_SW_TSTAMP | SKBTX_BPF))
 		skb_tstamp_tx(skb, NULL);
 }
 
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 30d2c078966b..6a1083bcf779 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -7036,6 +7036,10 @@ enum {
 					 * dev layer when SK_BPF_CB_TX_TIMESTAMPING
 					 * feature is on.
 					 */
+	BPF_SOCK_OPS_TS_SW_OPT_CB,	/* Called when skb is about to send
+					 * to the nic when SK_BPF_CB_TX_TIMESTAMPING
+					 * feature is on.
+					 */
 };
 
 /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 6f55eb90a632..74c04cbe5acd 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5557,7 +5557,8 @@ static bool skb_tstamp_tx_report_so_timestamping(struct sk_buff *skb,
 
 static void skb_tstamp_tx_report_bpf_timestamping(struct sk_buff *skb,
 						  struct sock *sk,
-						  int tstype)
+						  int tstype,
+						  bool sw)
 {
 	int op;
 
@@ -5565,6 +5566,11 @@ static void skb_tstamp_tx_report_bpf_timestamping(struct sk_buff *skb,
 	case SCM_TSTAMP_SCHED:
 		op = BPF_SOCK_OPS_TS_SCHED_OPT_CB;
 		break;
+	case SCM_TSTAMP_SND:
+		if (!sw)
+			return;
+		op = BPF_SOCK_OPS_TS_SW_OPT_CB;
+		break;
 	default:
 		return;
 	}
@@ -5585,7 +5591,7 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
 		return;
 
 	if (skb_shinfo(orig_skb)->tx_flags & SKBTX_BPF)
-		skb_tstamp_tx_report_bpf_timestamping(orig_skb, sk, tstype);
+		skb_tstamp_tx_report_bpf_timestamping(orig_skb, sk, tstype, sw);
 
 	if (!skb_tstamp_tx_report_so_timestamping(orig_skb, tstype, sw))
 		return;
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index eed91b7296b7..9bd1c7c77b17 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -7029,6 +7029,10 @@ enum {
 					 * dev layer when SK_BPF_CB_TX_TIMESTAMPING
 					 * feature is on.
 					 */
+	BPF_SOCK_OPS_TS_SW_OPT_CB,	/* Called when skb is about to send
+					 * to the nic when SK_BPF_CB_TX_TIMESTAMPING
+					 * feature is on.
+					 */
 };
 
 /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
-- 
2.43.5

[PATCH bpf-next v9 08/12] bpf: support hw SCM_TSTAMP_SND of SO_TIMESTAMPING

[Jason Xing]

Support hw SCM_TSTAMP_SND case. Then bpf program can fetch the
hwstamp from skb directly.

To avoid changing so many callers using SKBTX_HW_TSTAMP from drivers,
replace SKBTX_HW_TSTAMP with SKBTX_HW_TSTAMP_NOBPF.

Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 include/linux/skbuff.h         | 4 +++-
 include/uapi/linux/bpf.h       | 4 ++++
 net/core/skbuff.c              | 6 ++----
 net/dsa/user.c                 | 2 +-
 net/socket.c                   | 2 +-
 tools/include/uapi/linux/bpf.h | 4 ++++
 6 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 76582500c5ea..0b4f1889500d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -470,7 +470,7 @@ struct skb_shared_hwtstamps {
 /* Definitions for tx_flags in struct skb_shared_info */
 enum {
 	/* generate hardware time stamp */
-	SKBTX_HW_TSTAMP = 1 << 0,
+	SKBTX_HW_TSTAMP_NOBPF = 1 << 0,
 
 	/* generate software time stamp when queueing packet to NIC */
 	SKBTX_SW_TSTAMP = 1 << 1,
@@ -494,6 +494,8 @@ enum {
 	SKBTX_BPF = 1 << 7,
 };
 
+#define SKBTX_HW_TSTAMP		(SKBTX_HW_TSTAMP_NOBPF | SKBTX_BPF)
+
 #define SKBTX_ANY_SW_TSTAMP	(SKBTX_SW_TSTAMP    | \
 				 SKBTX_SCHED_TSTAMP | \
 				 SKBTX_BPF)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 6a1083bcf779..e71a9b53e7bc 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -7040,6 +7040,10 @@ enum {
 					 * to the nic when SK_BPF_CB_TX_TIMESTAMPING
 					 * feature is on.
 					 */
+	BPF_SOCK_OPS_TS_HW_OPT_CB,	/* Called in hardware phase when
+					 * SK_BPF_CB_TX_TIMESTAMPING feature
+					 * is on.
+					 */
 };
 
 /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 74c04cbe5acd..ca1ba4252ca5 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5547,7 +5547,7 @@ static bool skb_tstamp_tx_report_so_timestamping(struct sk_buff *skb,
 		return skb_shinfo(skb)->tx_flags & SKBTX_SCHED_TSTAMP;
 	case SCM_TSTAMP_SND:
 		return skb_shinfo(skb)->tx_flags & (sw ? SKBTX_SW_TSTAMP :
-						    SKBTX_HW_TSTAMP);
+						    SKBTX_HW_TSTAMP_NOBPF);
 	case SCM_TSTAMP_ACK:
 		return TCP_SKB_CB(skb)->txstamp_ack;
 	}
@@ -5567,9 +5567,7 @@ static void skb_tstamp_tx_report_bpf_timestamping(struct sk_buff *skb,
 		op = BPF_SOCK_OPS_TS_SCHED_OPT_CB;
 		break;
 	case SCM_TSTAMP_SND:
-		if (!sw)
-			return;
-		op = BPF_SOCK_OPS_TS_SW_OPT_CB;
+		op = sw ? BPF_SOCK_OPS_TS_SW_OPT_CB : BPF_SOCK_OPS_TS_HW_OPT_CB;
 		break;
 	default:
 		return;
diff --git a/net/dsa/user.c b/net/dsa/user.c
index 291ab1b4acc4..794fe553dd77 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -897,7 +897,7 @@ static void dsa_skb_tx_timestamp(struct dsa_user_priv *p,
 {
 	struct dsa_switch *ds = p->dp->ds;
 
-	if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
+	if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NOBPF))
 		return;
 
 	if (!ds->ops->port_txtstamp)
diff --git a/net/socket.c b/net/socket.c
index 262a28b59c7f..517de433d4bb 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -676,7 +676,7 @@ void __sock_tx_timestamp(__u32 tsflags, __u8 *tx_flags)
 	u8 flags = *tx_flags;
 
 	if (tsflags & SOF_TIMESTAMPING_TX_HARDWARE) {
-		flags |= SKBTX_HW_TSTAMP;
+		flags |= SKBTX_HW_TSTAMP_NOBPF;
 
 		/* PTP hardware clocks can provide a free running cycle counter
 		 * as a time base for virtual clocks. Tell driver to use the
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 9bd1c7c77b17..7b9652ce7e3c 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -7033,6 +7033,10 @@ enum {
 					 * to the nic when SK_BPF_CB_TX_TIMESTAMPING
 					 * feature is on.
 					 */
+	BPF_SOCK_OPS_TS_HW_OPT_CB,	/* Called in hardware phase when
+					 * SK_BPF_CB_TX_TIMESTAMPING feature
+					 * is on.
+					 */
 };
 
 /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
-- 
2.43.5

[PATCH bpf-next v9 09/12] bpf: support SCM_TSTAMP_ACK of SO_TIMESTAMPING

[Jason Xing]

Support the ACK timestamp case. Extend txstamp_ack to two bits:
1 stands for SO_TIMESTAMPING mode, 2 bpf extension. The latter
will be used later.

Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 include/net/tcp.h              | 4 ++--
 include/uapi/linux/bpf.h       | 5 +++++
 net/core/skbuff.c              | 5 ++++-
 tools/include/uapi/linux/bpf.h | 5 +++++
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 4c4dca59352b..ef30f3605e04 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -958,10 +958,10 @@ struct tcp_skb_cb {
 
 	__u8		sacked;		/* State flags for SACK.	*/
 	__u8		ip_dsfield;	/* IPv4 tos or IPv6 dsfield	*/
-	__u8		txstamp_ack:1,	/* Record TX timestamp for ack? */
+	__u8		txstamp_ack:2,	/* Record TX timestamp for ack? */
 			eor:1,		/* Is skb MSG_EOR marked? */
 			has_rxtstamp:1,	/* SKB has a RX timestamp	*/
-			unused:5;
+			unused:4;
 	__u32		ack_seq;	/* Sequence number ACK'd	*/
 	union {
 		struct {
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index e71a9b53e7bc..c04e788125a7 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -7044,6 +7044,11 @@ enum {
 					 * SK_BPF_CB_TX_TIMESTAMPING feature
 					 * is on.
 					 */
+	BPF_SOCK_OPS_TS_ACK_OPT_CB,	/* Called when all the skbs in the
+					 * same sendmsg call are acked
+					 * when SK_BPF_CB_TX_TIMESTAMPING
+					 * feature is on.
+					 */
 };
 
 /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index ca1ba4252ca5..c0f4d6f6583d 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5549,7 +5549,7 @@ static bool skb_tstamp_tx_report_so_timestamping(struct sk_buff *skb,
 		return skb_shinfo(skb)->tx_flags & (sw ? SKBTX_SW_TSTAMP :
 						    SKBTX_HW_TSTAMP_NOBPF);
 	case SCM_TSTAMP_ACK:
-		return TCP_SKB_CB(skb)->txstamp_ack;
+		return TCP_SKB_CB(skb)->txstamp_ack == 1;
 	}
 
 	return false;
@@ -5569,6 +5569,9 @@ static void skb_tstamp_tx_report_bpf_timestamping(struct sk_buff *skb,
 	case SCM_TSTAMP_SND:
 		op = sw ? BPF_SOCK_OPS_TS_SW_OPT_CB : BPF_SOCK_OPS_TS_HW_OPT_CB;
 		break;
+	case SCM_TSTAMP_ACK:
+		op = BPF_SOCK_OPS_TS_ACK_OPT_CB;
+		break;
 	default:
 		return;
 	}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 7b9652ce7e3c..d3e2988b3b4c 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -7037,6 +7037,11 @@ enum {
 					 * SK_BPF_CB_TX_TIMESTAMPING feature
 					 * is on.
 					 */
+	BPF_SOCK_OPS_TS_ACK_OPT_CB,	/* Called when all the skbs in the
+					 * same sendmsg call are acked
+					 * when SK_BPF_CB_TX_TIMESTAMPING
+					 * feature is on.
+					 */
 };
 
 /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
-- 
2.43.5

[PATCH bpf-next v9 10/12] bpf: add a new callback in tcp_tx_timestamp()

[Jason Xing]

Introduce the callback to correlate tcp_sendmsg timestamp with other
points, like SND/SW/ACK. For instance, let bpf prog trace the beginning
of tcp_sendmsg_locked() and then store the sendmsg timestamp at
the bpf_sk_storage, so that in tcp_tx_timestamp() we can correlate
the timestamp with tskey which can be found in other sending points.
More details can be found in the selftest.

Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 include/uapi/linux/bpf.h       | 5 +++++
 net/ipv4/tcp.c                 | 4 ++++
 tools/include/uapi/linux/bpf.h | 5 +++++
 3 files changed, 14 insertions(+)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index c04e788125a7..7b1a7dd68c0d 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -7049,6 +7049,11 @@ enum {
 					 * when SK_BPF_CB_TX_TIMESTAMPING
 					 * feature is on.
 					 */
+	BPF_SOCK_OPS_TS_SND_CB,		/* Called when every sendmsg syscall
+					 * is triggered. It's used to correlate
+					 * sendmsg timestamp with corresponding
+					 * tskey.
+					 */
 };
 
 /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 0d704bda6c41..4895d7a6327b 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -492,6 +492,10 @@ static void tcp_tx_timestamp(struct sock *sk, struct sockcm_cookie *sockc)
 		if (tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK)
 			shinfo->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
 	}
+
+	if (cgroup_bpf_enabled(CGROUP_SOCK_OPS) &&
+	    SK_BPF_CB_FLAG_TEST(sk, SK_BPF_CB_TX_TIMESTAMPING) && skb)
+		bpf_skops_tx_timestamping(sk, skb, BPF_SOCK_OPS_TS_SND_CB);
 }
 
 static bool tcp_stream_is_readable(struct sock *sk, int target)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index d3e2988b3b4c..2739ee0154a0 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -7042,6 +7042,11 @@ enum {
 					 * when SK_BPF_CB_TX_TIMESTAMPING
 					 * feature is on.
 					 */
+	BPF_SOCK_OPS_TS_SND_CB,		/* Called when every sendmsg syscall
+					 * is triggered. It's used to correlate
+					 * sendmsg timestamp with corresponding
+					 * tskey.
+					 */
 };
 
 /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
-- 
2.43.5

[PATCH bpf-next v9 11/12] bpf: support selective sampling for bpf timestamping

[Jason Xing]

Use __bpf_kfunc feature to allow bpf prog dynamically and selectively
to sample/track the skb. For example, the bpf prog will limit tracking
X numbers of packets and then will stop there instead of tracing
all the sendmsgs of matched flow all along.

Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 kernel/bpf/btf.c  |  1 +
 net/core/filter.c | 27 ++++++++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 8396ce1d0fba..a65e2eeffb88 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -8535,6 +8535,7 @@ static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type)
 	case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
 	case BPF_PROG_TYPE_CGROUP_SOCKOPT:
 	case BPF_PROG_TYPE_CGROUP_SYSCTL:
+	case BPF_PROG_TYPE_SOCK_OPS:
 		return BTF_KFUNC_HOOK_CGROUP;
 	case BPF_PROG_TYPE_SCHED_ACT:
 		return BTF_KFUNC_HOOK_SCHED_ACT;
diff --git a/net/core/filter.c b/net/core/filter.c
index 7f56d0bbeb00..db20a947e757 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -12102,6 +12102,21 @@ __bpf_kfunc int bpf_sk_assign_tcp_reqsk(struct __sk_buff *s, struct sock *sk,
 #endif
 }
 
+__bpf_kfunc int bpf_sock_ops_enable_tx_tstamp(struct bpf_sock_ops_kern *skops)
+{
+	struct sk_buff *skb;
+
+	if (skops->op != BPF_SOCK_OPS_TS_SND_CB)
+		return -EOPNOTSUPP;
+
+	skb = skops->skb;
+	TCP_SKB_CB(skb)->txstamp_ack = 2;
+	skb_shinfo(skb)->tx_flags |= SKBTX_BPF;
+	skb_shinfo(skb)->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
+
+	return 0;
+}
+
 __bpf_kfunc_end_defs();
 
 int bpf_dynptr_from_skb_rdonly(struct __sk_buff *skb, u64 flags,
@@ -12135,6 +12150,10 @@ BTF_KFUNCS_START(bpf_kfunc_check_set_tcp_reqsk)
 BTF_ID_FLAGS(func, bpf_sk_assign_tcp_reqsk, KF_TRUSTED_ARGS)
 BTF_KFUNCS_END(bpf_kfunc_check_set_tcp_reqsk)
 
+BTF_KFUNCS_START(bpf_kfunc_check_set_sock_ops)
+BTF_ID_FLAGS(func, bpf_sock_ops_enable_tx_tstamp, KF_TRUSTED_ARGS)
+BTF_KFUNCS_END(bpf_kfunc_check_set_sock_ops)
+
 static const struct btf_kfunc_id_set bpf_kfunc_set_skb = {
 	.owner = THIS_MODULE,
 	.set = &bpf_kfunc_check_set_skb,
@@ -12155,6 +12174,11 @@ static const struct btf_kfunc_id_set bpf_kfunc_set_tcp_reqsk = {
 	.set = &bpf_kfunc_check_set_tcp_reqsk,
 };
 
+static const struct btf_kfunc_id_set bpf_kfunc_set_sock_ops = {
+	.owner = THIS_MODULE,
+	.set = &bpf_kfunc_check_set_sock_ops,
+};
+
 static int __init bpf_kfunc_init(void)
 {
 	int ret;
@@ -12173,7 +12197,8 @@ static int __init bpf_kfunc_init(void)
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &bpf_kfunc_set_xdp);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
 					       &bpf_kfunc_set_sock_addr);
-	return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_kfunc_set_tcp_reqsk);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_kfunc_set_tcp_reqsk);
+	return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SOCK_OPS, &bpf_kfunc_set_sock_ops);
 }
 late_initcall(bpf_kfunc_init);
 
-- 
2.43.5

[PATCH bpf-next v9 12/12] selftests/bpf: add simple bpf tests in the tx path for timestamping feature

[Jason Xing]

Bpf prog calculates a couple of latency delta between each tx points
which SO_TIMESTAMPING feature has already implemented. It can be used
in the real world to diagnose the behaviour in the tx path.

Also, check the safety issues by accessing a few bpf calls in
bpf_test_access_bpf_calls().

There remains a few realistic things[1][2] to highlight:
1. in general a packet may pass through multiple qdiscs. For instance
with bonding or tunnel virtual devices in the egress path.
2. packets may be resent, in which case an ACK might precede a repeat
SCHED and SND.
3. erroneous or malicious peers may also just never send an ACK.

[1]: https://lore.kernel.org/all/67a389af981b0_14e0832949d@willemb.c.googlers.com.notmuch/
[2]: https://lore.kernel.org/all/c329a0c1-239b-4ca1-91f2-cb30b8dd2f6a@linux.dev/

Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 .../bpf/prog_tests/so_timestamping.c          |  79 +++++
 .../selftests/bpf/progs/so_timestamping.c     | 312 ++++++++++++++++++
 2 files changed, 391 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/so_timestamping.c
 create mode 100644 tools/testing/selftests/bpf/progs/so_timestamping.c

diff --git a/tools/testing/selftests/bpf/prog_tests/so_timestamping.c b/tools/testing/selftests/bpf/prog_tests/so_timestamping.c
new file mode 100644
index 000000000000..1829f93bc52e
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/so_timestamping.c
@@ -0,0 +1,79 @@
+#include "test_progs.h"
+#include "network_helpers.h"
+
+#include "so_timestamping.skel.h"
+
+#define CG_NAME "/so-timestamping-test"
+
+static const char addr4_str[] = "127.0.0.1";
+static const char addr6_str[] = "::1";
+static struct so_timestamping *skel;
+
+static void test_tcp(int family)
+{
+	struct so_timestamping__bss *bss = skel->bss;
+	char buf[] = "testing testing";
+	int sfd = -1, cfd = -1;
+	int n;
+
+	memset(bss, 0, sizeof(*bss));
+
+	sfd = start_server(family, SOCK_STREAM,
+			   family == AF_INET6 ? addr6_str : addr4_str, 0, 0);
+	if (!ASSERT_OK_FD(sfd, "start_server"))
+		goto out;
+
+	cfd = connect_to_fd(sfd, 0);
+	if (!ASSERT_OK_FD(cfd, "connect_to_fd_server"))
+		goto out;
+
+	n = write(cfd, buf, sizeof(buf));
+	if (!ASSERT_EQ(n, sizeof(buf), "send to server"))
+		goto out;
+
+	ASSERT_EQ(bss->nr_active, 1, "nr_active");
+	ASSERT_EQ(bss->nr_snd, 2, "nr_snd");
+	ASSERT_EQ(bss->nr_sched, 1, "nr_sched");
+	ASSERT_EQ(bss->nr_txsw, 1, "nr_txsw");
+	ASSERT_EQ(bss->nr_ack, 1, "nr_ack");
+
+out:
+	if (sfd >= 0)
+		close(sfd);
+	if (cfd >= 0)
+		close(cfd);
+}
+
+void test_so_timestamping(void)
+{
+	struct netns_obj *ns;
+	int cg_fd;
+
+	cg_fd = test__join_cgroup(CG_NAME);
+	if (!ASSERT_OK_FD(cg_fd, "join cgroup"))
+		return;
+
+	ns = netns_new("so_timestamping_ns", true);
+	if (!ASSERT_OK_PTR(ns, "create ns"))
+		goto done;
+
+	skel = so_timestamping__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open and load skel"))
+		goto done;
+
+	if (!ASSERT_OK(so_timestamping__attach(skel), "attach skel"))
+		goto done;
+
+	skel->links.skops_sockopt =
+		bpf_program__attach_cgroup(skel->progs.skops_sockopt, cg_fd);
+	if (!ASSERT_OK_PTR(skel->links.skops_sockopt, "attach cgroup"))
+		goto done;
+
+	test_tcp(AF_INET6);
+	test_tcp(AF_INET);
+
+done:
+	so_timestamping__destroy(skel);
+	netns_free(ns);
+	close(cg_fd);
+}
diff --git a/tools/testing/selftests/bpf/progs/so_timestamping.c b/tools/testing/selftests/bpf/progs/so_timestamping.c
new file mode 100644
index 000000000000..4974552cdecb
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/so_timestamping.c
@@ -0,0 +1,312 @@
+#include "vmlinux.h"
+#include "bpf_tracing_net.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+#include "bpf_kfuncs.h"
+#define BPF_PROG_TEST_TCP_HDR_OPTIONS
+#include "test_tcp_hdr_options.h"
+#include <errno.h>
+
+#define SK_BPF_CB_FLAGS 1009
+#define SK_BPF_CB_TX_TIMESTAMPING 1
+
+int nr_active;
+int nr_snd;
+int nr_passive;
+int nr_sched;
+int nr_txsw;
+int nr_ack;
+
+struct sockopt_test {
+	int opt;
+	int new;
+};
+
+static const struct sockopt_test sol_socket_tests[] = {
+	{ .opt = SK_BPF_CB_FLAGS, .new = SK_BPF_CB_TX_TIMESTAMPING, },
+	{ .opt = 0, },
+};
+
+struct loop_ctx {
+	void *ctx;
+	const struct sock *sk;
+};
+
+struct sk_stg {
+	__u64 sendmsg_ns;	/* record ts when sendmsg is called */
+};
+
+struct sk_tskey {
+	u64 cookie;
+	u32 tskey;
+};
+
+struct delay_info {
+	u64 sendmsg_ns;		/* record ts when sendmsg is called */
+	u32 sched_delay;	/* SCHED_OPT_CB - sendmsg_ns */
+	u32 sw_snd_delay;	/* SW_OPT_CB - SCHED_OPT_CB */
+	u32 ack_delay;		/* ACK_OPT_CB - SW_OPT_CB */
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_SK_STORAGE);
+	__uint(map_flags, BPF_F_NO_PREALLOC);
+	__type(key, int);
+	__type(value, struct sk_stg);
+} sk_stg_map SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, struct sk_tskey);
+	__type(value, struct delay_info);
+	__uint(max_entries, 1024);
+} time_map SEC(".maps");
+
+static u64 delay_tolerance_nsec = 10000000000; /* 10 second as an example */
+
+extern int bpf_sock_ops_enable_tx_tstamp(struct bpf_sock_ops_kern *skops) __ksym;
+
+static int bpf_test_sockopt_int(void *ctx, const struct sock *sk,
+				const struct sockopt_test *t,
+				int level)
+{
+	int new, opt, tmp;
+
+	opt = t->opt;
+	new = t->new;
+
+	if (bpf_setsockopt(ctx, level, opt, &new, sizeof(new)))
+		return 1;
+
+	if (bpf_getsockopt(ctx, level, opt, &tmp, sizeof(tmp)) ||
+	    tmp != new)
+		return 1;
+
+	return 0;
+}
+
+static int bpf_test_socket_sockopt(__u32 i, struct loop_ctx *lc)
+{
+	const struct sockopt_test *t;
+
+	if (i >= ARRAY_SIZE(sol_socket_tests))
+		return 1;
+
+	t = &sol_socket_tests[i];
+	if (!t->opt)
+		return 1;
+
+	return bpf_test_sockopt_int(lc->ctx, lc->sk, t, SOL_SOCKET);
+}
+
+static int bpf_test_sockopt(void *ctx, const struct sock *sk)
+{
+	struct loop_ctx lc = { .ctx = ctx, .sk = sk, };
+	int n;
+
+	n = bpf_loop(ARRAY_SIZE(sol_socket_tests), bpf_test_socket_sockopt, &lc, 0);
+	if (n != ARRAY_SIZE(sol_socket_tests))
+		return -1;
+
+	return 0;
+}
+
+static bool bpf_test_access_sockopt(void *ctx)
+{
+	const struct sockopt_test *t;
+	int tmp, ret, i = 0;
+	int level = SOL_SOCKET;
+
+	t = &sol_socket_tests[i];
+
+	for (; t->opt;) {
+		ret = bpf_setsockopt(ctx, level, t->opt, (void *)&t->new, sizeof(t->new));
+		if (ret != -EOPNOTSUPP)
+			return true;
+
+		ret = bpf_getsockopt(ctx, level, t->opt, &tmp, sizeof(tmp));
+		if (ret != -EOPNOTSUPP)
+			return true;
+
+		if (++i >= ARRAY_SIZE(sol_socket_tests))
+			break;
+	}
+
+	return false;
+}
+
+/* Adding a simple test to see if we can get an expected value */
+static bool bpf_test_access_load_hdr_opt(struct bpf_sock_ops *skops)
+{
+	struct tcp_opt reg_opt;
+	int load_flags = 0;
+	int ret;
+
+	reg_opt.kind = TCPOPT_EXP;
+	reg_opt.len = 0;
+	reg_opt.data32 = 0;
+	ret = bpf_load_hdr_opt(skops, &reg_opt, sizeof(reg_opt), load_flags);
+	if (ret != -EOPNOTSUPP)
+		return true;
+
+	return false;
+}
+
+/* Adding a simple test to see if we can get an expected value */
+static bool bpf_test_access_cb_flags_set(struct bpf_sock_ops *skops)
+{
+	int ret;
+
+	ret = bpf_sock_ops_cb_flags_set(skops, 0);
+	if (ret != -EOPNOTSUPP)
+		return true;
+
+	return false;
+}
+
+/* In the timestamping callbacks, we're not allowed to call the following
+ * BPF CALLs for the safety concern. Return false if expected.
+ */
+static bool bpf_test_access_bpf_calls(struct bpf_sock_ops *skops,
+				     const struct sock *sk)
+{
+	if (bpf_test_access_sockopt(skops))
+		return true;
+
+	if (bpf_test_access_load_hdr_opt(skops))
+		return true;
+
+	if (bpf_test_access_cb_flags_set(skops))
+		return true;
+
+	return false;
+}
+
+static bool bpf_test_delay(struct bpf_sock_ops *skops, const struct sock *sk)
+{
+	struct bpf_sock_ops_kern *skops_kern;
+	u64 timestamp = bpf_ktime_get_ns();
+	struct skb_shared_info *shinfo;
+	struct delay_info dinfo = {0};
+	struct sk_tskey key = {0};
+	struct delay_info *val;
+	struct sk_buff *skb;
+	struct sk_stg *stg;
+	u64 prior_ts, delay;
+
+	if (bpf_test_access_bpf_calls(skops, sk))
+		return false;
+
+	skops_kern = bpf_cast_to_kern_ctx(skops);
+	skb = skops_kern->skb;
+	shinfo = bpf_core_cast(skb->head + skb->end, struct skb_shared_info);
+
+	key.cookie = bpf_get_socket_cookie(skops);
+	if (!key.cookie)
+		return false;
+
+	if (skops->op == BPF_SOCK_OPS_TS_SND_CB) {
+		stg = bpf_sk_storage_get(&sk_stg_map, (void *)sk, 0, 0);
+		if (!stg)
+			return false;
+		dinfo.sendmsg_ns = stg->sendmsg_ns;
+		bpf_sock_ops_enable_tx_tstamp(skops_kern);
+		key.tskey = shinfo->tskey;
+		if (!key.tskey)
+			return false;
+		bpf_map_update_elem(&time_map, &key, &dinfo, BPF_ANY);
+		return true;
+	}
+
+	key.tskey = shinfo->tskey;
+	if (!key.tskey)
+		return false;
+
+	val = bpf_map_lookup_elem(&time_map, &key);
+	if (!val)
+		return false;
+
+	switch (skops->op) {
+	case BPF_SOCK_OPS_TS_SCHED_OPT_CB:
+		delay = val->sched_delay = timestamp - val->sendmsg_ns;
+		break;
+	case BPF_SOCK_OPS_TS_SW_OPT_CB:
+		prior_ts = val->sched_delay + val->sendmsg_ns;
+		delay = val->sw_snd_delay = timestamp - prior_ts;
+		break;
+	case BPF_SOCK_OPS_TS_ACK_OPT_CB:
+		prior_ts = val->sw_snd_delay + val->sched_delay + val->sendmsg_ns;
+		delay = val->ack_delay = timestamp - prior_ts;
+		break;
+	}
+
+	if (delay >= delay_tolerance_nsec)
+		return false;
+
+	/* Since it's the last one, remove from the map after latency check */
+	if (skops->op == BPF_SOCK_OPS_TS_ACK_OPT_CB)
+		bpf_map_delete_elem(&time_map, &key);
+
+	return true;
+}
+
+SEC("fentry/tcp_sendmsg_locked")
+int BPF_PROG(trace_tcp_sendmsg_locked, struct sock *sk, struct msghdr *msg, size_t size)
+{
+	u64 timestamp = bpf_ktime_get_ns();
+	u32 flag = sk->sk_bpf_cb_flags;
+	struct sk_stg *stg;
+
+	if (!flag)
+		return 0;
+
+	stg = bpf_sk_storage_get(&sk_stg_map, sk, 0,
+				 BPF_SK_STORAGE_GET_F_CREATE);
+	if (!stg)
+		return 0;
+
+	stg->sendmsg_ns = timestamp;
+	nr_snd += 1;
+	return 0;
+}
+
+SEC("sockops")
+int skops_sockopt(struct bpf_sock_ops *skops)
+{
+	struct bpf_sock *bpf_sk = skops->sk;
+	const struct sock *sk;
+
+	if (!bpf_sk)
+		return 1;
+
+	sk = (struct sock *)bpf_skc_to_tcp_sock(bpf_sk);
+	if (!sk)
+		return 1;
+
+	switch (skops->op) {
+	case BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB:
+		nr_active += !bpf_test_sockopt(skops, sk);
+		break;
+	case BPF_SOCK_OPS_TS_SND_CB:
+		if (bpf_test_delay(skops, sk))
+			nr_snd += 1;
+		break;
+	case BPF_SOCK_OPS_TS_SCHED_OPT_CB:
+		if (bpf_test_delay(skops, sk))
+			nr_sched += 1;
+		break;
+	case BPF_SOCK_OPS_TS_SW_OPT_CB:
+		if (bpf_test_delay(skops, sk))
+			nr_txsw += 1;
+		break;
+	case BPF_SOCK_OPS_TS_ACK_OPT_CB:
+		if (bpf_test_delay(skops, sk))
+			nr_ack += 1;
+		break;
+	}
+
+	return 1;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.43.5

Re: [PATCH bpf-next v9 09/12] bpf: support SCM_TSTAMP_ACK of SO_TIMESTAMPING

[Willem de Bruijn]

Jason Xing wrote:
> Support the ACK timestamp case. Extend txstamp_ack to two bits:
> 1 stands for SO_TIMESTAMPING mode, 2 bpf extension. The latter
> will be used later.
> 
> Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> ---
>  include/net/tcp.h              | 4 ++--
>  include/uapi/linux/bpf.h       | 5 +++++
>  net/core/skbuff.c              | 5 ++++-
>  tools/include/uapi/linux/bpf.h | 5 +++++
>  4 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 4c4dca59352b..ef30f3605e04 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -958,10 +958,10 @@ struct tcp_skb_cb {
>  
>  	__u8		sacked;		/* State flags for SACK.	*/
>  	__u8		ip_dsfield;	/* IPv4 tos or IPv6 dsfield	*/
> -	__u8		txstamp_ack:1,	/* Record TX timestamp for ack? */
> +	__u8		txstamp_ack:2,	/* Record TX timestamp for ack? */
>  			eor:1,		/* Is skb MSG_EOR marked? */
>  			has_rxtstamp:1,	/* SKB has a RX timestamp	*/
> -			unused:5;
> +			unused:4;
>  	__u32		ack_seq;	/* Sequence number ACK'd	*/
>  	union {
>  		struct {
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index e71a9b53e7bc..c04e788125a7 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -7044,6 +7044,11 @@ enum {
>  					 * SK_BPF_CB_TX_TIMESTAMPING feature
>  					 * is on.
>  					 */
> +	BPF_SOCK_OPS_TS_ACK_OPT_CB,	/* Called when all the skbs in the
> +					 * same sendmsg call are acked
> +					 * when SK_BPF_CB_TX_TIMESTAMPING
> +					 * feature is on.
> +					 */
>  };
>  
>  /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index ca1ba4252ca5..c0f4d6f6583d 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -5549,7 +5549,7 @@ static bool skb_tstamp_tx_report_so_timestamping(struct sk_buff *skb,
>  		return skb_shinfo(skb)->tx_flags & (sw ? SKBTX_SW_TSTAMP :
>  						    SKBTX_HW_TSTAMP_NOBPF);
>  	case SCM_TSTAMP_ACK:
> -		return TCP_SKB_CB(skb)->txstamp_ack;
> +		return TCP_SKB_CB(skb)->txstamp_ack == 1;

For the two to coexist, this should be txstamp_ack & 1

And in the patch that introduces the BPF bit, txstamp_ack |= 2, rather than txstamp_ack = 2.

And let's define labels rather than use constants directly: 

  #define TSTAMP_ACK_SK  0x1
  #define TSTAMP_ACK_BPF 0x2

Re: [PATCH bpf-next v9 09/12] bpf: support SCM_TSTAMP_ACK of SO_TIMESTAMPING

[Jason Xing]

On Sun, Feb 9, 2025 at 1:54 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > Support the ACK timestamp case. Extend txstamp_ack to two bits:
> > 1 stands for SO_TIMESTAMPING mode, 2 bpf extension. The latter
> > will be used later.
> >
> > Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> > ---
> >  include/net/tcp.h              | 4 ++--
> >  include/uapi/linux/bpf.h       | 5 +++++
> >  net/core/skbuff.c              | 5 ++++-
> >  tools/include/uapi/linux/bpf.h | 5 +++++
> >  4 files changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/net/tcp.h b/include/net/tcp.h
> > index 4c4dca59352b..ef30f3605e04 100644
> > --- a/include/net/tcp.h
> > +++ b/include/net/tcp.h
> > @@ -958,10 +958,10 @@ struct tcp_skb_cb {
> >
> >       __u8            sacked;         /* State flags for SACK.        */
> >       __u8            ip_dsfield;     /* IPv4 tos or IPv6 dsfield     */
> > -     __u8            txstamp_ack:1,  /* Record TX timestamp for ack? */
> > +     __u8            txstamp_ack:2,  /* Record TX timestamp for ack? */
> >                       eor:1,          /* Is skb MSG_EOR marked? */
> >                       has_rxtstamp:1, /* SKB has a RX timestamp       */
> > -                     unused:5;
> > +                     unused:4;
> >       __u32           ack_seq;        /* Sequence number ACK'd        */
> >       union {
> >               struct {
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index e71a9b53e7bc..c04e788125a7 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -7044,6 +7044,11 @@ enum {
> >                                        * SK_BPF_CB_TX_TIMESTAMPING feature
> >                                        * is on.
> >                                        */
> > +     BPF_SOCK_OPS_TS_ACK_OPT_CB,     /* Called when all the skbs in the
> > +                                      * same sendmsg call are acked
> > +                                      * when SK_BPF_CB_TX_TIMESTAMPING
> > +                                      * feature is on.
> > +                                      */
> >  };
> >
> >  /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index ca1ba4252ca5..c0f4d6f6583d 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -5549,7 +5549,7 @@ static bool skb_tstamp_tx_report_so_timestamping(struct sk_buff *skb,
> >               return skb_shinfo(skb)->tx_flags & (sw ? SKBTX_SW_TSTAMP :
> >                                                   SKBTX_HW_TSTAMP_NOBPF);
> >       case SCM_TSTAMP_ACK:
> > -             return TCP_SKB_CB(skb)->txstamp_ack;
> > +             return TCP_SKB_CB(skb)->txstamp_ack == 1;
>
> For the two to coexist, this should be txstamp_ack & 1
>
> And in the patch that introduces the BPF bit, txstamp_ack |= 2, rather than txstamp_ack = 2.
>
> And let's define labels rather than use constants directly:
>
>   #define TSTAMP_ACK_SK  0x1
>   #define TSTAMP_ACK_BPF 0x2

Thanks. Will do it.

Re: [PATCH bpf-next v9 00/12] net-timestamp: bpf extension to equip applications transparently

[Martin KaFai Lau]

On 2/8/25 2:32 AM, Jason Xing wrote:
> "Timestamping is key to debugging network stack latency. With
> SO_TIMESTAMPING, bugs that are otherwise incorrectly assumed to be
> network issues can be attributed to the kernel." This is extracted
> from the talk "SO_TIMESTAMPING: Powering Fleetwide RPC Monitoring"
> addressed by Willem de Bruijn at netdevconf 0x17).
> 
> There are a few areas that need optimization with the consideration of
> easier use and less performance impact, which I highlighted and mainly
> discussed at netconf 2024 with Willem de Bruijn and John Fastabend:
> uAPI compatibility, extra system call overhead, and the need for
> application modification. I initially managed to solve these issues
> by writing a kernel module that hooks various key functions. However,
> this approach is not suitable for the next kernel release. Therefore,
> a BPF extension was proposed. During recent period, Martin KaFai Lau
> provides invaluable suggestions about BPF along the way. Many thanks
> here!
> 
> In this series, only support foundamental codes and tx for TCP.

typo: fundamental.... This had been brought up before (in v7?).

By fundamental, I suspect you meant (?) bpf timestamping infrastructure, like: 
"This series adds the BPF networking timestamping infrastructure. This series 
also adds TX timestamping support for TCP. The RX timestamping and UDP support 
will be added in the future."

> This approach mostly relies on existing SO_TIMESTAMPING feature, users

It reuses most of the tx timestamping callback that is currently enabled by the 
SO_TIMESTAMPING. However, I don't think there is a lot of overlap in term of the 
SO_TIMESTAMPING api which does feel like API reuse when first reading this comment.

> only needs to pass certain flags through bpf_setsocktopt() to a separate
> tsflags. Please see the last selftest patch in this series.
> 
> ---
> v8
> Link: https://lore.kernel.org/all/20250128084620.57547-1-kerneljasonxing@gmail.com/
> 1. adjust some commit messages and titles
> 2. add sk cookie in selftests
> 3. handle the NULL pointer in hwstamp
> 4. use kfunc to do selective sampling
> 
> v7
> Link: https://lore.kernel.org/all/20250121012901.87763-1-kerneljasonxing@gmail.com/
> 1. target bpf-next tree
> 2. simplely and directly stop timestamping callbacks calling a few BPF
> CALLS due to safety concern.
> 3. add more new testcases and adjust the existing testcases
> 4. revise some comments of new timestamping callbacks
> 5. remove a few BPF CGROUP locks
> 
> RFC v6
> In the meantime, any suggestions and reviews are welcome!
> Link: https://lore.kernel.org/all/20250112113748.73504-1-kerneljasonxing@gmail.com/
> 1. handle those safety problem by using the correct method.
> 2. support bpf_getsockopt.
> 3. adjust the position of BPF_SOCK_OPS_TS_TCP_SND_CB
> 4. fix mishandling the hardware timestamp error
> 5. add more corresponding tests
> 
> v5
> Link: https://lore.kernel.org/all/20241207173803.90744-1-kerneljasonxing@gmail.com/
> 1. handle the safety issus when someone tries to call unrelated bpf
> helpers.
> 2. avoid adding direct function call in the hot path like
> __dev_queue_xmit()
> 3. remove reporting the hardware timestamp and tskey since they can be
> fetched through the existing helper with the help of
> bpf_skops_init_skb(), please see the selftest.
> 4. add new sendmsg callback in tcp_sendmsg, and introduce tskey_bpf used
> by bpf program to correlate tcp_sendmsg with other hook points in patch [13/15].
> 
> v4
> Link: https://lore.kernel.org/all/20241028110535.82999-1-kerneljasonxing@gmail.com/
> 1. introduce sk->sk_bpf_cb_flags to let user use bpf_setsockopt() (Martin)
> 2. introduce SKBTX_BPF to enable the bpf SO_TIMESTAMPING feature (Martin)
> 3. introduce bpf map in tests (Martin)
> 4. I choose to make this series as simple as possible, so I only support
> most cases in the tx path for TCP protocol.
> 
> v3
> Link: https://lore.kernel.org/all/20241012040651.95616-1-kerneljasonxing@gmail.com/
> 1. support UDP proto by introducing a new generation point.
> 2. for OPT_ID, introducing sk_tskey_bpf_offset to compute the delta
> between the current socket key and bpf socket key. It is desiged for
> UDP, which also applies to TCP.
> 3. support bpf_getsockopt()
> 4. use cgroup static key instead.
> 5. add one simple bpf selftest to show how it can be used.
> 6. remove the rx support from v2 because the number of patches could
> exceed the limit of one series.
> 
> V2
> Link: https://lore.kernel.org/all/20241008095109.99918-1-kerneljasonxing@gmail.com/
> 1. Introduce tsflag requestors so that we are able to extend more in the
> future. Besides, it enables TX flags for bpf extension feature separately
> without breaking users. It is suggested by Vadim Fedorenko.
> 2. introduce a static key to control the whole feature. (Willem)
> 3. Open the gate of bpf_setsockopt for the SO_TIMESTAMPING feature in
> some TX/RX cases, not all the cases.
> 
> Jason Xing (12):
>    bpf: add support for bpf_setsockopt()
>    bpf: prepare for timestamping callbacks use
>    bpf: stop unsafely accessing TCP fields in bpf callbacks
>    bpf: stop calling some sock_op BPF CALLs in new timestamping callbacks
>    net-timestamp: prepare for isolating two modes of SO_TIMESTAMPING
>    bpf: support SCM_TSTAMP_SCHED of SO_TIMESTAMPING
>    bpf: support sw SCM_TSTAMP_SND of SO_TIMESTAMPING
>    bpf: support hw SCM_TSTAMP_SND of SO_TIMESTAMPING
>    bpf: support SCM_TSTAMP_ACK of SO_TIMESTAMPING
>    bpf: add a new callback in tcp_tx_timestamp()
>    bpf: support selective sampling for bpf timestamping
>    selftests/bpf: add simple bpf tests in the tx path for timestamping
>      feature
> 
>   include/linux/filter.h                        |   1 +
>   include/linux/skbuff.h                        |  12 +-
>   include/net/sock.h                            |  10 +
>   include/net/tcp.h                             |   5 +-
>   include/uapi/linux/bpf.h                      |  30 ++
>   kernel/bpf/btf.c                              |   1 +
>   net/core/dev.c                                |   3 +-
>   net/core/filter.c                             |  75 ++++-
>   net/core/skbuff.c                             |  48 ++-
>   net/core/sock.c                               |  15 +
>   net/dsa/user.c                                |   2 +-
>   net/ipv4/tcp.c                                |   4 +
>   net/ipv4/tcp_input.c                          |   2 +
>   net/ipv4/tcp_output.c                         |   2 +
>   net/socket.c                                  |   2 +-
>   tools/include/uapi/linux/bpf.h                |  23 ++
>   .../bpf/prog_tests/so_timestamping.c          |  79 +++++
>   .../selftests/bpf/progs/so_timestamping.c     | 312 ++++++++++++++++++
>   18 files changed, 612 insertions(+), 14 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/so_timestamping.c
>   create mode 100644 tools/testing/selftests/bpf/progs/so_timestamping.c
> 

Re: [PATCH bpf-next v9 00/12] net-timestamp: bpf extension to equip applications transparently

[Jason Xing]

On Tue, Feb 11, 2025 at 7:37 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 2/8/25 2:32 AM, Jason Xing wrote:
> > "Timestamping is key to debugging network stack latency. With
> > SO_TIMESTAMPING, bugs that are otherwise incorrectly assumed to be
> > network issues can be attributed to the kernel." This is extracted
> > from the talk "SO_TIMESTAMPING: Powering Fleetwide RPC Monitoring"
> > addressed by Willem de Bruijn at netdevconf 0x17).
> >
> > There are a few areas that need optimization with the consideration of
> > easier use and less performance impact, which I highlighted and mainly
> > discussed at netconf 2024 with Willem de Bruijn and John Fastabend:
> > uAPI compatibility, extra system call overhead, and the need for
> > application modification. I initially managed to solve these issues
> > by writing a kernel module that hooks various key functions. However,
> > this approach is not suitable for the next kernel release. Therefore,
> > a BPF extension was proposed. During recent period, Martin KaFai Lau
> > provides invaluable suggestions about BPF along the way. Many thanks
> > here!
> >
> > In this series, only support foundamental codes and tx for TCP.
>
> typo: fundamental.... This had been brought up before (in v7?).

Oh, right!

>
> By fundamental, I suspect you meant (?) bpf timestamping infrastructure, like:
> "This series adds the BPF networking timestamping infrastructure. This series
> also adds TX timestamping support for TCP. The RX timestamping and UDP support
> will be added in the future."

Right!

>
> > This approach mostly relies on existing SO_TIMESTAMPING feature, users
>
> It reuses most of the tx timestamping callback that is currently enabled by the
> SO_TIMESTAMPING. However, I don't think there is a lot of overlap in term of the
> SO_TIMESTAMPING api which does feel like API reuse when first reading this comment.

I'm going to refine them. Thanks for the review!

Thanks,
Jason

>
> > only needs to pass certain flags through bpf_setsocktopt() to a separate
> > tsflags. Please see the last selftest patch in this series.
> >
> > ---
> > v8
> > Link: https://lore.kernel.org/all/20250128084620.57547-1-kerneljasonxing@gmail.com/
> > 1. adjust some commit messages and titles
> > 2. add sk cookie in selftests
> > 3. handle the NULL pointer in hwstamp
> > 4. use kfunc to do selective sampling
> >
> > v7
> > Link: https://lore.kernel.org/all/20250121012901.87763-1-kerneljasonxing@gmail.com/
> > 1. target bpf-next tree
> > 2. simplely and directly stop timestamping callbacks calling a few BPF
> > CALLS due to safety concern.
> > 3. add more new testcases and adjust the existing testcases
> > 4. revise some comments of new timestamping callbacks
> > 5. remove a few BPF CGROUP locks
> >
> > RFC v6
> > In the meantime, any suggestions and reviews are welcome!
> > Link: https://lore.kernel.org/all/20250112113748.73504-1-kerneljasonxing@gmail.com/
> > 1. handle those safety problem by using the correct method.
> > 2. support bpf_getsockopt.
> > 3. adjust the position of BPF_SOCK_OPS_TS_TCP_SND_CB
> > 4. fix mishandling the hardware timestamp error
> > 5. add more corresponding tests
> >
> > v5
> > Link: https://lore.kernel.org/all/20241207173803.90744-1-kerneljasonxing@gmail.com/
> > 1. handle the safety issus when someone tries to call unrelated bpf
> > helpers.
> > 2. avoid adding direct function call in the hot path like
> > __dev_queue_xmit()
> > 3. remove reporting the hardware timestamp and tskey since they can be
> > fetched through the existing helper with the help of
> > bpf_skops_init_skb(), please see the selftest.
> > 4. add new sendmsg callback in tcp_sendmsg, and introduce tskey_bpf used
> > by bpf program to correlate tcp_sendmsg with other hook points in patch [13/15].
> >
> > v4
> > Link: https://lore.kernel.org/all/20241028110535.82999-1-kerneljasonxing@gmail.com/
> > 1. introduce sk->sk_bpf_cb_flags to let user use bpf_setsockopt() (Martin)
> > 2. introduce SKBTX_BPF to enable the bpf SO_TIMESTAMPING feature (Martin)
> > 3. introduce bpf map in tests (Martin)
> > 4. I choose to make this series as simple as possible, so I only support
> > most cases in the tx path for TCP protocol.
> >
> > v3
> > Link: https://lore.kernel.org/all/20241012040651.95616-1-kerneljasonxing@gmail.com/
> > 1. support UDP proto by introducing a new generation point.
> > 2. for OPT_ID, introducing sk_tskey_bpf_offset to compute the delta
> > between the current socket key and bpf socket key. It is desiged for
> > UDP, which also applies to TCP.
> > 3. support bpf_getsockopt()
> > 4. use cgroup static key instead.
> > 5. add one simple bpf selftest to show how it can be used.
> > 6. remove the rx support from v2 because the number of patches could
> > exceed the limit of one series.
> >
> > V2
> > Link: https://lore.kernel.org/all/20241008095109.99918-1-kerneljasonxing@gmail.com/
> > 1. Introduce tsflag requestors so that we are able to extend more in the
> > future. Besides, it enables TX flags for bpf extension feature separately
> > without breaking users. It is suggested by Vadim Fedorenko.
> > 2. introduce a static key to control the whole feature. (Willem)
> > 3. Open the gate of bpf_setsockopt for the SO_TIMESTAMPING feature in
> > some TX/RX cases, not all the cases.
> >
> > Jason Xing (12):
> >    bpf: add support for bpf_setsockopt()
> >    bpf: prepare for timestamping callbacks use
> >    bpf: stop unsafely accessing TCP fields in bpf callbacks
> >    bpf: stop calling some sock_op BPF CALLs in new timestamping callbacks
> >    net-timestamp: prepare for isolating two modes of SO_TIMESTAMPING
> >    bpf: support SCM_TSTAMP_SCHED of SO_TIMESTAMPING
> >    bpf: support sw SCM_TSTAMP_SND of SO_TIMESTAMPING
> >    bpf: support hw SCM_TSTAMP_SND of SO_TIMESTAMPING
> >    bpf: support SCM_TSTAMP_ACK of SO_TIMESTAMPING
> >    bpf: add a new callback in tcp_tx_timestamp()
> >    bpf: support selective sampling for bpf timestamping
> >    selftests/bpf: add simple bpf tests in the tx path for timestamping
> >      feature
> >
> >   include/linux/filter.h                        |   1 +
> >   include/linux/skbuff.h                        |  12 +-
> >   include/net/sock.h                            |  10 +
> >   include/net/tcp.h                             |   5 +-
> >   include/uapi/linux/bpf.h                      |  30 ++
> >   kernel/bpf/btf.c                              |   1 +
> >   net/core/dev.c                                |   3 +-
> >   net/core/filter.c                             |  75 ++++-
> >   net/core/skbuff.c                             |  48 ++-
> >   net/core/sock.c                               |  15 +
> >   net/dsa/user.c                                |   2 +-
> >   net/ipv4/tcp.c                                |   4 +
> >   net/ipv4/tcp_input.c                          |   2 +
> >   net/ipv4/tcp_output.c                         |   2 +
> >   net/socket.c                                  |   2 +-
> >   tools/include/uapi/linux/bpf.h                |  23 ++
> >   .../bpf/prog_tests/so_timestamping.c          |  79 +++++
> >   .../selftests/bpf/progs/so_timestamping.c     | 312 ++++++++++++++++++
> >   18 files changed, 612 insertions(+), 14 deletions(-)
> >   create mode 100644 tools/testing/selftests/bpf/prog_tests/so_timestamping.c
> >   create mode 100644 tools/testing/selftests/bpf/progs/so_timestamping.c
> >
>

Re: [PATCH bpf-next v9 01/12] bpf: add support for bpf_setsockopt()

[Martin KaFai Lau]

On 2/8/25 2:32 AM, Jason Xing wrote:
> Users can write the following code to enable the bpf extension:
> int flags = SK_BPF_CB_TX_TIMESTAMPING;
> int opts = SK_BPF_CB_FLAGS;
> bpf_setsockopt(skops, SOL_SOCKET, opts, &flags, sizeof(flags));

The commit message should explain what is added/changed and why it is needed.
The above only tells how it is used, and the subject "bpf: add support for 
bpf_setsockopt()" is unclear. Add what support? Also, both get- and 
set-sockopt() are changed.

Subject: "bpf: Add networking timestamping support to bpf_get/setsockopt()"

What: The new SK_BPF_CB_FLAGS and new SK_BPF_CB_TX_TIMESTAMPING are added to 
bpf_get/setsockopt.

Why: The later patch will implement the BPF networking timestamping. The BPF 
program will use bpf_setsockopt(SK_BPF_CB_FLAGS, SK_BPF_CB_TX_TIMESTAMPING) to 
enable the BPF networking timestamping on a socket.

Re: [PATCH bpf-next v9 02/12] bpf: prepare for timestamping callbacks use

[Martin KaFai Lau]

On 2/8/25 2:32 AM, Jason Xing wrote:
> Later, four callback points to report information to user space
> based on this patch will be introduced.
> 
> As to skb initialization here, users can follow these three steps
> as below to fetch the shared info from the exported skb in the bpf
> prog:
> 1. skops_kern = bpf_cast_to_kern_ctx(skops);
> 2. skb = skops_kern->skb;
> 3. shinfo = bpf_core_cast(skb->head + skb->end, struct skb_shared_info);
> 
> More details can be seen in the last selftest patch of the series.

This BPF program example is not useful in this commit message. It is not how 
this change will be used in the kernel. People will naturally be required to 
look at the selftest to see how the bpf prog can get to the skb and tskey, etc.

The commit message should focus on explaining "what" has changed and "why" it is 
necessary. The "why" part ("four callback points to report...") is mostly 
present but could be clearer.

Subject: bpf: Prepare the sock_ops ctx and call bpf prog for TX timestamping

(What)
This patch introduces a new bpf_skops_tx_timestamping() function that prepares 
the "struct bpf_sock_ops" ctx and then executes the sockops BPF program.

(Why)
The subsequent patch will utilize bpf_skops_tx_timestamping() at the existing TX 
timestamping kernel callbacks (__sk_tstamp_tx specifically) to call the sockops 
BPF program.


> 
> Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> ---
>   include/net/sock.h |  7 +++++++
>   net/core/sock.c    | 15 +++++++++++++++
>   2 files changed, 22 insertions(+)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 7916982343c6..6f4d54faba92 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -2923,6 +2923,13 @@ int sock_set_timestamping(struct sock *sk, int optname,
>   			  struct so_timestamping timestamping);
>   
>   void sock_enable_timestamps(struct sock *sk);
> +#if defined(CONFIG_CGROUP_BPF)
> +void bpf_skops_tx_timestamping(struct sock *sk, struct sk_buff *skb, int op);
> +#else
> +static inline void bpf_skops_tx_timestamping(struct sock *sk, struct sk_buff *skb, int op)
> +{
> +}
> +#endif
>   void sock_no_linger(struct sock *sk);
>   void sock_set_keepalive(struct sock *sk);
>   void sock_set_priority(struct sock *sk, u32 priority);
> diff --git a/net/core/sock.c b/net/core/sock.c
> index eae2ae70a2e0..41db6407e360 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -948,6 +948,21 @@ int sock_set_timestamping(struct sock *sk, int optname,
>   	return 0;
>   }
>   
> +#if defined(CONFIG_CGROUP_BPF)
> +void bpf_skops_tx_timestamping(struct sock *sk, struct sk_buff *skb, int op)
> +{
> +	struct bpf_sock_ops_kern sock_ops;
> +
> +	memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp));
> +	sock_ops.op = op;
> +	sock_ops.is_fullsock = 1;
> +	sock_ops.sk = sk;
> +	bpf_skops_init_skb(&sock_ops, skb, 0);
> +	/* Timestamping bpf extension supports only TCP and UDP full socket */

nit: After our earlier discussions, it's clear that the above is_fullsock = 1; 
is always true for all sk here. This comment has become redundant. Let's remove it.

> +	__cgroup_bpf_run_filter_sock_ops(sk, &sock_ops, CGROUP_SOCK_OPS);
> +}
> +#endif
> +
>   void sock_set_keepalive(struct sock *sk)
>   {
>   	lock_sock(sk);

Re: [PATCH bpf-next v9 01/12] bpf: add support for bpf_setsockopt()

[Jason Xing]

On Tue, Feb 11, 2025 at 9:03 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 2/8/25 2:32 AM, Jason Xing wrote:
> > Users can write the following code to enable the bpf extension:
> > int flags = SK_BPF_CB_TX_TIMESTAMPING;
> > int opts = SK_BPF_CB_FLAGS;
> > bpf_setsockopt(skops, SOL_SOCKET, opts, &flags, sizeof(flags));
>
> The commit message should explain what is added/changed and why it is needed.
> The above only tells how it is used, and the subject "bpf: add support for
> bpf_setsockopt()" is unclear. Add what support? Also, both get- and
> set-sockopt() are changed.
>
> Subject: "bpf: Add networking timestamping support to bpf_get/setsockopt()"
>
> What: The new SK_BPF_CB_FLAGS and new SK_BPF_CB_TX_TIMESTAMPING are added to
> bpf_get/setsockopt.
>
> Why: The later patch will implement the BPF networking timestamping. The BPF
> program will use bpf_setsockopt(SK_BPF_CB_FLAGS, SK_BPF_CB_TX_TIMESTAMPING) to
> enable the BPF networking timestamping on a socket.

Thanks. Learning a lot on how to write a good description, I will
adjust commit messages as you suggested.

Thanks,
Jason

Re: [PATCH bpf-next v9 02/12] bpf: prepare for timestamping callbacks use

[Jason Xing]

On Tue, Feb 11, 2025 at 9:32 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 2/8/25 2:32 AM, Jason Xing wrote:
> > Later, four callback points to report information to user space
> > based on this patch will be introduced.
> >
> > As to skb initialization here, users can follow these three steps
> > as below to fetch the shared info from the exported skb in the bpf
> > prog:
> > 1. skops_kern = bpf_cast_to_kern_ctx(skops);
> > 2. skb = skops_kern->skb;
> > 3. shinfo = bpf_core_cast(skb->head + skb->end, struct skb_shared_info);
> >
> > More details can be seen in the last selftest patch of the series.
>
> This BPF program example is not useful in this commit message. It is not how
> this change will be used in the kernel. People will naturally be required to
> look at the selftest to see how the bpf prog can get to the skb and tskey, etc.
>
> The commit message should focus on explaining "what" has changed and "why" it is
> necessary. The "why" part ("four callback points to report...") is mostly
> present but could be clearer.
>
> Subject: bpf: Prepare the sock_ops ctx and call bpf prog for TX timestamping
>
> (What)
> This patch introduces a new bpf_skops_tx_timestamping() function that prepares
> the "struct bpf_sock_ops" ctx and then executes the sockops BPF program.
>
> (Why)
> The subsequent patch will utilize bpf_skops_tx_timestamping() at the existing TX
> timestamping kernel callbacks (__sk_tstamp_tx specifically) to call the sockops
> BPF program.

Thanks, Will update it.

>
>
> >
> > Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> > ---
> >   include/net/sock.h |  7 +++++++
> >   net/core/sock.c    | 15 +++++++++++++++
> >   2 files changed, 22 insertions(+)
> >
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index 7916982343c6..6f4d54faba92 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -2923,6 +2923,13 @@ int sock_set_timestamping(struct sock *sk, int optname,
> >                         struct so_timestamping timestamping);
> >
> >   void sock_enable_timestamps(struct sock *sk);
> > +#if defined(CONFIG_CGROUP_BPF)
> > +void bpf_skops_tx_timestamping(struct sock *sk, struct sk_buff *skb, int op);
> > +#else
> > +static inline void bpf_skops_tx_timestamping(struct sock *sk, struct sk_buff *skb, int op)
> > +{
> > +}
> > +#endif
> >   void sock_no_linger(struct sock *sk);
> >   void sock_set_keepalive(struct sock *sk);
> >   void sock_set_priority(struct sock *sk, u32 priority);
> > diff --git a/net/core/sock.c b/net/core/sock.c
> > index eae2ae70a2e0..41db6407e360 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -948,6 +948,21 @@ int sock_set_timestamping(struct sock *sk, int optname,
> >       return 0;
> >   }
> >
> > +#if defined(CONFIG_CGROUP_BPF)
> > +void bpf_skops_tx_timestamping(struct sock *sk, struct sk_buff *skb, int op)
> > +{
> > +     struct bpf_sock_ops_kern sock_ops;
> > +
> > +     memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp));
> > +     sock_ops.op = op;
> > +     sock_ops.is_fullsock = 1;
> > +     sock_ops.sk = sk;
> > +     bpf_skops_init_skb(&sock_ops, skb, 0);
> > +     /* Timestamping bpf extension supports only TCP and UDP full socket */
>
> nit: After our earlier discussions, it's clear that the above is_fullsock = 1;
> is always true for all sk here. This comment has become redundant. Let's remove it.

Will do it.

Thanks,
Jason

>
> > +     __cgroup_bpf_run_filter_sock_ops(sk, &sock_ops, CGROUP_SOCK_OPS);
> > +}
> > +#endif
> > +
> >   void sock_set_keepalive(struct sock *sk)
> >   {
> >       lock_sock(sk);
>

Re: [PATCH bpf-next v9 03/12] bpf: stop unsafely accessing TCP fields in bpf callbacks

[Martin KaFai Lau]

On 2/8/25 2:32 AM, Jason Xing wrote:
> The "is_locked_tcp_sock" flag is added to indicate that the callback
> site has a tcp_sock locked.

It should mention that the later TX timestamping callbacks will not own the 
lock. This is what this patch is primarily for. We know the background, but 
future code readers may not. We will eventually become the readers of this patch 
in a few years' time.

> 
> Apply the new member is_locked_tcp_sock in the existing callbacks

It is hard to read "Apply the new member....". "Apply" could mean a few things. 
"Set to 1" is clearer.


> where is_fullsock is set to 1 can stop UDP socket accessing struct

The UDP part is future proof. This set does not support UDP which has to be 
clear in the commit message. This has been brought up before also.

> tcp_sock and stop TCP socket without sk lock protecting does the
> similar thing, or else it could be catastrophe leading to panic.
> 
> To keep it simple, instead of distinguishing between read and write
> access, users aren't allowed all read/write access to the tcp_sock
> through the older bpf_sock_ops ctx. The new timestamping callbacks
> can use newer helpers to read everything from a sk (e.g. bpf_core_cast),
> so nothing is lost.

(Subject):
bpf: Prevent unsafe access to the sock fields in the BPF timestamping callback

(Why):
The subsequent patch will implement BPF TX timestamping. It will call the 
sockops BPF program without holding the sock lock.

This breaks the current assumption that all sock ops programs will hold the sock 
lock. The sock's fields of the uapi's bpf_sock_ops requires this assumption.

(What and How):
To address this,
a new "u8 is_locked_tcp_sock;" field is added. This patch sets it in the current 
sock_ops callbacks. The "is_fullsock" test is then replaced by the 
"is_locked_tcp_sock" test during sock_ops_convert_ctx_access().

The new TX timestamping callbacks added in the subsequent patch will not have 
this set. This will prevent unsafe access from the new timestamping callbacks.

Potentially, we could allow read-only access. However, this would require 
identifying which callback is read-safe-only and also requires additional BPF 
instruction rewrites in the covert_ctx. Since the BPF program can always read 
everything from a socket (e.g., by using bpf_core_cast), this patch keeps it 
simple and disables all read and write access to any socket fields through the 
bpf_sock_ops UAPI from the new TX timestamping callback.

Moreover, note that some of the fields in bpf_sock_ops are specific to tcp_sock, 
and sock_ops currently only supports tcp_sock. In the future, UDP timestamping 
will be added, which will also break this assumption. The same idea used in this 
patch will be reused. Considering that the current sock_ops only supports 
tcp_sock, the variable is named is_locked_"tcp"_sock.

Re: [PATCH bpf-next v9 04/12] bpf: stop calling some sock_op BPF CALLs in new timestamping callbacks

[Martin KaFai Lau]

On 2/8/25 2:32 AM, Jason Xing wrote:
> Considering the potential invalid access issues, calling
> bpf_sock_ops_setsockopt/getsockopt, bpf_sock_ops_cb_flags_set,
> and the bpf_sock_ops_load_hdr_opt in the new timestamping
> callbacks will return -EOPNOTSUPP error value.

The "why" part is mostly missing. Why they are not safe to be used in the TX 
timestamping callbacks?

> 
> It also prevents the UDP socket trying to access TCP fields in
> the bpf extension for SO_TIMESTAMPING for the same consideration.
Let's remove this UDP part to avoid confusion. UDP has very little to do with 
disabling the helpers here.

"BPF_CALL" in the subject is not clear either. "BPF_CALL" can mean many things, 
such as calling BPF helpers, calling BPF kfuncs, or calling its own BPF 
subprograms, etc. In this case, it is the calling BPF helpers.

(Subject)
bpf: Disable unsafe helpers in TX timestamping callbacks

(Why)
New TX timestamping sock_ops callbacks will be added in the subsequent patch. 
Some of the existing BPF helpers will not be safe to be used in the TX 
timestamping callbacks.

The bpf_sock_ops_setsockopt, bpf_sock_ops_getsockopt, and 
bpf_sock_ops_cb_flags_set require owning the sock lock. TX timestamping 
callbacks will not own the lock.

The bpf_sock_ops_load_hdr_opt needs the skb->data pointing to the TCP header. 
This will not be true in the TX timestamping callbacks.

(What and How)
At the beginning of these helpers, this patch checks the bpf_sock->op to ensure 
these helpers are used by the existing sock_ops callbacks only.

Re: [PATCH bpf-next v9 06/12] bpf: support SCM_TSTAMP_SCHED of SO_TIMESTAMPING

[Martin KaFai Lau]

On 2/8/25 2:32 AM, Jason Xing wrote:
> Support SCM_TSTAMP_SCHED case. Introduce SKBTX_BPF used as
> an indicator telling us whether the skb should be traced
> by the bpf prog.

The BPF side does not exactly support SCM_TSTAMP_SCHED as a report value.

What this patch does is:

Add a new sock_ops callback, BPF_SOCK_OPS_TS_SCHED_OPT_CB. This callback will 
occur at the same timestamping point as the user space's SCM_TSTAMP_SCHED. The
BPF program can use it to get the same SCM_TSTAMP_SCHED timestamp without 
modifying the user-space application.

A new SKBTX_BPF flag is added to mark skb_shinfo(skb)->tx_flags, ensuring that 
the new BPF timestamping and the current user space's SO_TIMESTAMPING do not 
interfere with each other.

I would remove most of the SO_TIMESTAMPING comments from the commit messages. 
The timestamping points are the same but there is not much overlapping on the 
API side.

Subject could be:
bpf: Add BPF_SOCK_OPS_TS_SCHED_OPT_CB callback

[ The same probably for patch 7-9. ]

> 
> Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> ---
>   include/linux/skbuff.h         |  6 +++++-
>   include/uapi/linux/bpf.h       |  4 ++++
>   net/core/dev.c                 |  3 ++-
>   net/core/skbuff.c              | 20 ++++++++++++++++++++
>   tools/include/uapi/linux/bpf.h |  4 ++++
>   5 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index bb2b751d274a..52f6e033e704 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -489,10 +489,14 @@ enum {
>   
>   	/* generate software time stamp when entering packet scheduling */
>   	SKBTX_SCHED_TSTAMP = 1 << 6,
> +
> +	/* used for bpf extension when a bpf program is loaded */
> +	SKBTX_BPF = 1 << 7,
>   };
>   
>   #define SKBTX_ANY_SW_TSTAMP	(SKBTX_SW_TSTAMP    | \
> -				 SKBTX_SCHED_TSTAMP)
> +				 SKBTX_SCHED_TSTAMP | \
> +				 SKBTX_BPF)
>   #define SKBTX_ANY_TSTAMP	(SKBTX_HW_TSTAMP | \
>   				 SKBTX_HW_TSTAMP_USE_CYCLES | \
>   				 SKBTX_ANY_SW_TSTAMP)
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 6116eb3d1515..30d2c078966b 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -7032,6 +7032,10 @@ enum {
>   					 * by the kernel or the
>   					 * earlier bpf-progs.
>   					 */
> +	BPF_SOCK_OPS_TS_SCHED_OPT_CB,	/* Called when skb is passing through
> +					 * dev layer when SK_BPF_CB_TX_TIMESTAMPING
> +					 * feature is on.
> +					 */
>   };
>   
>   /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
> diff --git a/net/core/dev.c b/net/core/dev.c
> index afa2282f2604..d57946c96511 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4500,7 +4500,8 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev)
>   	skb_reset_mac_header(skb);
>   	skb_assert_len(skb);
>   
> -	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_SCHED_TSTAMP))
> +	if (unlikely(skb_shinfo(skb)->tx_flags &
> +		     (SKBTX_SCHED_TSTAMP | SKBTX_BPF)))
>   		__skb_tstamp_tx(skb, NULL, NULL, skb->sk, SCM_TSTAMP_SCHED);
>   
>   	/* Disable soft irqs for various locks below. Also
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 46530d516909..6f55eb90a632 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -5555,6 +5555,23 @@ static bool skb_tstamp_tx_report_so_timestamping(struct sk_buff *skb,
>   	return false;
>   }
>   
> +static void skb_tstamp_tx_report_bpf_timestamping(struct sk_buff *skb,
> +						  struct sock *sk,
> +						  int tstype)
> +{
> +	int op;
> +
> +	switch (tstype) {
> +	case SCM_TSTAMP_SCHED:
> +		op = BPF_SOCK_OPS_TS_SCHED_OPT_CB;
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	bpf_skops_tx_timestamping(sk, skb, op);
> +}
> +
>   void __skb_tstamp_tx(struct sk_buff *orig_skb,
>   		     const struct sk_buff *ack_skb,
>   		     struct skb_shared_hwtstamps *hwtstamps,
> @@ -5567,6 +5584,9 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
>   	if (!sk)
>   		return;
>   
> +	if (skb_shinfo(orig_skb)->tx_flags & SKBTX_BPF)
> +		skb_tstamp_tx_report_bpf_timestamping(orig_skb, sk, tstype);
> +
>   	if (!skb_tstamp_tx_report_so_timestamping(orig_skb, tstype, sw))
>   		return;
>   
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 70366f74ef4e..eed91b7296b7 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -7025,6 +7025,10 @@ enum {
>   					 * by the kernel or the
>   					 * earlier bpf-progs.
>   					 */
> +	BPF_SOCK_OPS_TS_SCHED_OPT_CB,	/* Called when skb is passing through
> +					 * dev layer when SK_BPF_CB_TX_TIMESTAMPING
> +					 * feature is on.
> +					 */
>   };
>   
>   /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect

Re: [PATCH bpf-next v9 06/12] bpf: support SCM_TSTAMP_SCHED of SO_TIMESTAMPING

[Jason Xing]

On Tue, Feb 11, 2025 at 3:12 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 2/8/25 2:32 AM, Jason Xing wrote:
> > Support SCM_TSTAMP_SCHED case. Introduce SKBTX_BPF used as
> > an indicator telling us whether the skb should be traced
> > by the bpf prog.
>
> The BPF side does not exactly support SCM_TSTAMP_SCHED as a report value.
>
> What this patch does is:
>
> Add a new sock_ops callback, BPF_SOCK_OPS_TS_SCHED_OPT_CB. This callback will
> occur at the same timestamping point as the user space's SCM_TSTAMP_SCHED. The
> BPF program can use it to get the same SCM_TSTAMP_SCHED timestamp without
> modifying the user-space application.
>
> A new SKBTX_BPF flag is added to mark skb_shinfo(skb)->tx_flags, ensuring that
> the new BPF timestamping and the current user space's SO_TIMESTAMPING do not
> interfere with each other.
>
> I would remove most of the SO_TIMESTAMPING comments from the commit messages.
> The timestamping points are the same but there is not much overlapping on the
> API side.
>
> Subject could be:
> bpf: Add BPF_SOCK_OPS_TS_SCHED_OPT_CB callback
>
> [ The same probably for patch 7-9. ]

Thanks. I will similarly adjust them as well :)

Thanks,
Jason

Re: [PATCH bpf-next v9 11/12] bpf: support selective sampling for bpf timestamping

[Martin KaFai Lau]

On 2/8/25 2:32 AM, Jason Xing wrote:
> Use __bpf_kfunc feature to allow bpf prog dynamically and selectively

s/Use/Add/

Remove "dynamically". A kfunc can only be called dynamically at runtime.

Like:

"Add the bpf_sock_ops_enable_tx_tstamp kfunc to allow BPF programs to 
selectively enable TX timestamping on a skb during tcp_sendmsg..."

> to sample/track the skb. For example, the bpf prog will limit tracking
> X numbers of packets and then will stop there instead of tracing
> all the sendmsgs of matched flow all along.
 > > Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> ---
>   kernel/bpf/btf.c  |  1 +
>   net/core/filter.c | 27 ++++++++++++++++++++++++++-
>   2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 8396ce1d0fba..a65e2eeffb88 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -8535,6 +8535,7 @@ static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type)
>   	case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
>   	case BPF_PROG_TYPE_CGROUP_SOCKOPT:
>   	case BPF_PROG_TYPE_CGROUP_SYSCTL:
> +	case BPF_PROG_TYPE_SOCK_OPS:
>   		return BTF_KFUNC_HOOK_CGROUP;
>   	case BPF_PROG_TYPE_SCHED_ACT:
>   		return BTF_KFUNC_HOOK_SCHED_ACT;
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 7f56d0bbeb00..db20a947e757 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -12102,6 +12102,21 @@ __bpf_kfunc int bpf_sk_assign_tcp_reqsk(struct __sk_buff *s, struct sock *sk,
>   #endif
>   }
>   
> +__bpf_kfunc int bpf_sock_ops_enable_tx_tstamp(struct bpf_sock_ops_kern *skops)

I am ok to always enable txstamp_ack here. Please still add a second "u64 flags" 
argument such that future disable/enable is still possible.

> +{
> +	struct sk_buff *skb;
> +
> +	if (skops->op != BPF_SOCK_OPS_TS_SND_CB)
 > +		return -EOPNOTSUPP;> +
> +	skb = skops->skb;
> +	TCP_SKB_CB(skb)->txstamp_ack = 2;

Willem (thanks!) has already mentioned there is a bug.

This also brought up that a test is missing: the bpf timestamping and user 
space's SO_TIMESTAMPING can work without interfering others. The current test 
only has SK_BPF_CB_TX_TIMESTAMPING on. A test is needed when both 
SK_BPF_CB_TX_TIMESTAMPING and the user space's SO_TIMESTAMPING are on. The 
expectation is both of them will work together.

> +	skb_shinfo(skb)->tx_flags |= SKBTX_BPF;
> +	skb_shinfo(skb)->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
> +
> +	return 0;
> +}
> +
>   __bpf_kfunc_end_defs();
>   
>   int bpf_dynptr_from_skb_rdonly(struct __sk_buff *skb, u64 flags,
> @@ -12135,6 +12150,10 @@ BTF_KFUNCS_START(bpf_kfunc_check_set_tcp_reqsk)
>   BTF_ID_FLAGS(func, bpf_sk_assign_tcp_reqsk, KF_TRUSTED_ARGS)
>   BTF_KFUNCS_END(bpf_kfunc_check_set_tcp_reqsk)
>   
> +BTF_KFUNCS_START(bpf_kfunc_check_set_sock_ops)
> +BTF_ID_FLAGS(func, bpf_sock_ops_enable_tx_tstamp, KF_TRUSTED_ARGS)
> +BTF_KFUNCS_END(bpf_kfunc_check_set_sock_ops)
> +
>   static const struct btf_kfunc_id_set bpf_kfunc_set_skb = {
>   	.owner = THIS_MODULE,
>   	.set = &bpf_kfunc_check_set_skb,
> @@ -12155,6 +12174,11 @@ static const struct btf_kfunc_id_set bpf_kfunc_set_tcp_reqsk = {
>   	.set = &bpf_kfunc_check_set_tcp_reqsk,
>   };
>   
> +static const struct btf_kfunc_id_set bpf_kfunc_set_sock_ops = {
> +	.owner = THIS_MODULE,
> +	.set = &bpf_kfunc_check_set_sock_ops,
> +};
> +
>   static int __init bpf_kfunc_init(void)
>   {
>   	int ret;
> @@ -12173,7 +12197,8 @@ static int __init bpf_kfunc_init(void)
>   	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &bpf_kfunc_set_xdp);
>   	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
>   					       &bpf_kfunc_set_sock_addr);
> -	return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_kfunc_set_tcp_reqsk);
> +	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_kfunc_set_tcp_reqsk);
> +	return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SOCK_OPS, &bpf_kfunc_set_sock_ops);
>   }
>   late_initcall(bpf_kfunc_init);
>   

Re: [PATCH bpf-next v9 11/12] bpf: support selective sampling for bpf timestamping

[Jason Xing]

On Tue, Feb 11, 2025 at 3:41 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 2/8/25 2:32 AM, Jason Xing wrote:
> > Use __bpf_kfunc feature to allow bpf prog dynamically and selectively
>
> s/Use/Add/
>
> Remove "dynamically". A kfunc can only be called dynamically at runtime.
>
> Like:
>
> "Add the bpf_sock_ops_enable_tx_tstamp kfunc to allow BPF programs to
> selectively enable TX timestamping on a skb during tcp_sendmsg..."

Will adjust it.

>
> > to sample/track the skb. For example, the bpf prog will limit tracking
> > X numbers of packets and then will stop there instead of tracing
> > all the sendmsgs of matched flow all along.
>  > > Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> > ---
> >   kernel/bpf/btf.c  |  1 +
> >   net/core/filter.c | 27 ++++++++++++++++++++++++++-
> >   2 files changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 8396ce1d0fba..a65e2eeffb88 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -8535,6 +8535,7 @@ static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type)
> >       case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
> >       case BPF_PROG_TYPE_CGROUP_SOCKOPT:
> >       case BPF_PROG_TYPE_CGROUP_SYSCTL:
> > +     case BPF_PROG_TYPE_SOCK_OPS:
> >               return BTF_KFUNC_HOOK_CGROUP;
> >       case BPF_PROG_TYPE_SCHED_ACT:
> >               return BTF_KFUNC_HOOK_SCHED_ACT;
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 7f56d0bbeb00..db20a947e757 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -12102,6 +12102,21 @@ __bpf_kfunc int bpf_sk_assign_tcp_reqsk(struct __sk_buff *s, struct sock *sk,
> >   #endif
> >   }
> >
> > +__bpf_kfunc int bpf_sock_ops_enable_tx_tstamp(struct bpf_sock_ops_kern *skops)
>
> I am ok to always enable txstamp_ack here. Please still add a second "u64 flags"
> argument such that future disable/enable is still possible.

Ok. Will do it.

>
> > +{
> > +     struct sk_buff *skb;
> > +
> > +     if (skops->op != BPF_SOCK_OPS_TS_SND_CB)
>  > +            return -EOPNOTSUPP;> +
> > +     skb = skops->skb;
> > +     TCP_SKB_CB(skb)->txstamp_ack = 2;
>
> Willem (thanks!) has already mentioned there is a bug.
>
> This also brought up that a test is missing: the bpf timestamping and user
> space's SO_TIMESTAMPING can work without interfering others. The current test
> only has SK_BPF_CB_TX_TIMESTAMPING on. A test is needed when both
> SK_BPF_CB_TX_TIMESTAMPING and the user space's SO_TIMESTAMPING are on. The
> expectation is both of them will work together.

Yeah, I did miss this particular test. Let me figure out how to test
it in a proper way.

Thanks,
Jason

>
> > +     skb_shinfo(skb)->tx_flags |= SKBTX_BPF;
> > +     skb_shinfo(skb)->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
> > +
> > +     return 0;
> > +}
> > +
> >   __bpf_kfunc_end_defs();
> >
> >   int bpf_dynptr_from_skb_rdonly(struct __sk_buff *skb, u64 flags,
> > @@ -12135,6 +12150,10 @@ BTF_KFUNCS_START(bpf_kfunc_check_set_tcp_reqsk)
> >   BTF_ID_FLAGS(func, bpf_sk_assign_tcp_reqsk, KF_TRUSTED_ARGS)
> >   BTF_KFUNCS_END(bpf_kfunc_check_set_tcp_reqsk)
> >
> > +BTF_KFUNCS_START(bpf_kfunc_check_set_sock_ops)
> > +BTF_ID_FLAGS(func, bpf_sock_ops_enable_tx_tstamp, KF_TRUSTED_ARGS)
> > +BTF_KFUNCS_END(bpf_kfunc_check_set_sock_ops)
> > +
> >   static const struct btf_kfunc_id_set bpf_kfunc_set_skb = {
> >       .owner = THIS_MODULE,
> >       .set = &bpf_kfunc_check_set_skb,
> > @@ -12155,6 +12174,11 @@ static const struct btf_kfunc_id_set bpf_kfunc_set_tcp_reqsk = {
> >       .set = &bpf_kfunc_check_set_tcp_reqsk,
> >   };
> >
> > +static const struct btf_kfunc_id_set bpf_kfunc_set_sock_ops = {
> > +     .owner = THIS_MODULE,
> > +     .set = &bpf_kfunc_check_set_sock_ops,
> > +};
> > +
> >   static int __init bpf_kfunc_init(void)
> >   {
> >       int ret;
> > @@ -12173,7 +12197,8 @@ static int __init bpf_kfunc_init(void)
> >       ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &bpf_kfunc_set_xdp);
> >       ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
> >                                              &bpf_kfunc_set_sock_addr);
> > -     return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_kfunc_set_tcp_reqsk);
> > +     ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_kfunc_set_tcp_reqsk);
> > +     return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SOCK_OPS, &bpf_kfunc_set_sock_ops);
> >   }
> >   late_initcall(bpf_kfunc_init);
> >
>

Re: [PATCH bpf-next v9 12/12] selftests/bpf: add simple bpf tests in the tx path for timestamping feature

[Martin KaFai Lau]

On 2/8/25 2:32 AM, Jason Xing wrote:
> ---
>   .../bpf/prog_tests/so_timestamping.c          |  79 +++++
>   .../selftests/bpf/progs/so_timestamping.c     | 312 ++++++++++++++++++

A bike shedding. s/so_timestamping.c/net_timestamping.c/

> diff --git a/tools/testing/selftests/bpf/progs/so_timestamping.c b/tools/testing/selftests/bpf/progs/so_timestamping.c
> new file mode 100644
> index 000000000000..4974552cdecb
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/so_timestamping.c
> @@ -0,0 +1,312 @@
> +#include "vmlinux.h"
> +#include "bpf_tracing_net.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include "bpf_misc.h"
> +#include "bpf_kfuncs.h"
> +#define BPF_PROG_TEST_TCP_HDR_OPTIONS
> +#include "test_tcp_hdr_options.h"
> +#include <errno.h>
> +
> +#define SK_BPF_CB_FLAGS 1009
> +#define SK_BPF_CB_TX_TIMESTAMPING 1
> +
> +int nr_active;
> +int nr_snd;
> +int nr_passive;
> +int nr_sched;
> +int nr_txsw;
> +int nr_ack;
> +
> +struct sockopt_test {
> +	int opt;
> +	int new;
> +};
> +
> +static const struct sockopt_test sol_socket_tests[] = {
> +	{ .opt = SK_BPF_CB_FLAGS, .new = SK_BPF_CB_TX_TIMESTAMPING, },
> +	{ .opt = 0, },
> +};
> +
> +struct loop_ctx {
> +	void *ctx;
> +	const struct sock *sk;
> +};
> +
> +struct sk_stg {
> +	__u64 sendmsg_ns;	/* record ts when sendmsg is called */
> +};
> +
> +struct sk_tskey {
> +	u64 cookie;
> +	u32 tskey;
> +};
> +
> +struct delay_info {
> +	u64 sendmsg_ns;		/* record ts when sendmsg is called */
> +	u32 sched_delay;	/* SCHED_OPT_CB - sendmsg_ns */
> +	u32 sw_snd_delay;	/* SW_OPT_CB - SCHED_OPT_CB */
> +	u32 ack_delay;		/* ACK_OPT_CB - SW_OPT_CB */
> +};
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_SK_STORAGE);
> +	__uint(map_flags, BPF_F_NO_PREALLOC);
> +	__type(key, int);
> +	__type(value, struct sk_stg);
> +} sk_stg_map SEC(".maps");
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_HASH);
> +	__type(key, struct sk_tskey);
> +	__type(value, struct delay_info);
> +	__uint(max_entries, 1024);
> +} time_map SEC(".maps");
> +
> +static u64 delay_tolerance_nsec = 10000000000; /* 10 second as an example */
> +
> +extern int bpf_sock_ops_enable_tx_tstamp(struct bpf_sock_ops_kern *skops) __ksym;
> +
> +static int bpf_test_sockopt_int(void *ctx, const struct sock *sk,
> +				const struct sockopt_test *t,
> +				int level)

This should be the only one that is needed even when supporting the future RX 
timestamping.

TX and RX timestamping need to be tested independently. Looping it will either 
enabling them together or disabling them together. It cannot test whether RX 
will work by itself.

Thus, the bpf_loop won't help. Lets remove it to simplify the test.

> +{
> +	int new, opt, tmp;
> +
> +	opt = t->opt;
> +	new = t->new;
> +
> +	if (bpf_setsockopt(ctx, level, opt, &new, sizeof(new)))
> +		return 1;
> +
> +	if (bpf_getsockopt(ctx, level, opt, &tmp, sizeof(tmp)) ||
> +	    tmp != new)
> +		return 1;
> +
> +	return 0;
> +}
> +
> +static int bpf_test_socket_sockopt(__u32 i, struct loop_ctx *lc)
> +{
> +	const struct sockopt_test *t;
> +
> +	if (i >= ARRAY_SIZE(sol_socket_tests))
> +		return 1;
> +
> +	t = &sol_socket_tests[i];
> +	if (!t->opt)
> +		return 1;
> +
> +	return bpf_test_sockopt_int(lc->ctx, lc->sk, t, SOL_SOCKET);
> +}
> +
> +static int bpf_test_sockopt(void *ctx, const struct sock *sk)
> +{
> +	struct loop_ctx lc = { .ctx = ctx, .sk = sk, };
> +	int n;
> +
> +	n = bpf_loop(ARRAY_SIZE(sol_socket_tests), bpf_test_socket_sockopt, &lc, 0);
> +	if (n != ARRAY_SIZE(sol_socket_tests))
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static bool bpf_test_access_sockopt(void *ctx)
> +{
> +	const struct sockopt_test *t;
> +	int tmp, ret, i = 0;
> +	int level = SOL_SOCKET;
> +
> +	t = &sol_socket_tests[i];
> +
> +	for (; t->opt;) {

It really does not need a loop here. It only needs to test "one" optname to 
ensure it is -EOPNOTSUPP.

> +		ret = bpf_setsockopt(ctx, level, t->opt, (void *)&t->new, sizeof(t->new));
> +		if (ret != -EOPNOTSUPP)
> +			return true;
> +
> +		ret = bpf_getsockopt(ctx, level, t->opt, &tmp, sizeof(tmp));
> +		if (ret != -EOPNOTSUPP)
> +			return true;
> +
> +		if (++i >= ARRAY_SIZE(sol_socket_tests))
> +			break;
> +	}
> +
> +	return false;
> +}
> +
> +/* Adding a simple test to see if we can get an expected value */
> +static bool bpf_test_access_load_hdr_opt(struct bpf_sock_ops *skops)
> +{
> +	struct tcp_opt reg_opt;

Just noticed this one. Use a plain u8 array. Then no need to include the 
"test_tcp_hdr_options.h" from an unrelated test.

> +	int load_flags = 0;
> +	int ret;
> +
> +	reg_opt.kind = TCPOPT_EXP;

The kind could be any integer, e.g. 2.

> +	reg_opt.len = 0;
> +	reg_opt.data32 = 0;
> +	ret = bpf_load_hdr_opt(skops, &reg_opt, sizeof(reg_opt), load_flags);
> +	if (ret != -EOPNOTSUPP)
> +		return true;
> +
> +	return false;
> +}

Re: [PATCH bpf-next v9 03/12] bpf: stop unsafely accessing TCP fields in bpf callbacks

[Jason Xing]

On Tue, Feb 11, 2025 at 2:34 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 2/8/25 2:32 AM, Jason Xing wrote:
> > The "is_locked_tcp_sock" flag is added to indicate that the callback
> > site has a tcp_sock locked.
>
> It should mention that the later TX timestamping callbacks will not own the
> lock. This is what this patch is primarily for. We know the background, but
> future code readers may not. We will eventually become the readers of this patch
> in a few years' time.
>
> >
> > Apply the new member is_locked_tcp_sock in the existing callbacks
>
> It is hard to read "Apply the new member....". "Apply" could mean a few things.
> "Set to 1" is clearer.
>
>
> > where is_fullsock is set to 1 can stop UDP socket accessing struct
>
> The UDP part is future proof. This set does not support UDP which has to be
> clear in the commit message. This has been brought up before also.
>
> > tcp_sock and stop TCP socket without sk lock protecting does the
> > similar thing, or else it could be catastrophe leading to panic.
> >
> > To keep it simple, instead of distinguishing between read and write
> > access, users aren't allowed all read/write access to the tcp_sock
> > through the older bpf_sock_ops ctx. The new timestamping callbacks
> > can use newer helpers to read everything from a sk (e.g. bpf_core_cast),
> > so nothing is lost.
>
> (Subject):
> bpf: Prevent unsafe access to the sock fields in the BPF timestamping callback
>
> (Why):
> The subsequent patch will implement BPF TX timestamping. It will call the
> sockops BPF program without holding the sock lock.
>
> This breaks the current assumption that all sock ops programs will hold the sock
> lock. The sock's fields of the uapi's bpf_sock_ops requires this assumption.
>
> (What and How):
> To address this,
> a new "u8 is_locked_tcp_sock;" field is added. This patch sets it in the current
> sock_ops callbacks. The "is_fullsock" test is then replaced by the
> "is_locked_tcp_sock" test during sock_ops_convert_ctx_access().
>
> The new TX timestamping callbacks added in the subsequent patch will not have
> this set. This will prevent unsafe access from the new timestamping callbacks.
>
> Potentially, we could allow read-only access. However, this would require
> identifying which callback is read-safe-only and also requires additional BPF
> instruction rewrites in the covert_ctx. Since the BPF program can always read
> everything from a socket (e.g., by using bpf_core_cast), this patch keeps it
> simple and disables all read and write access to any socket fields through the
> bpf_sock_ops UAPI from the new TX timestamping callback.
>
> Moreover, note that some of the fields in bpf_sock_ops are specific to tcp_sock,
> and sock_ops currently only supports tcp_sock. In the future, UDP timestamping
> will be added, which will also break this assumption. The same idea used in this
> patch will be reused. Considering that the current sock_ops only supports
> tcp_sock, the variable is named is_locked_"tcp"_sock.

Thanks so much for polishing the commit message. I appreciate it!

Will adjust it.

Thanks,
Jason

Re: [PATCH bpf-next v9 04/12] bpf: stop calling some sock_op BPF CALLs in new timestamping callbacks

[Jason Xing]

On Tue, Feb 11, 2025 at 2:55 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 2/8/25 2:32 AM, Jason Xing wrote:
> > Considering the potential invalid access issues, calling
> > bpf_sock_ops_setsockopt/getsockopt, bpf_sock_ops_cb_flags_set,
> > and the bpf_sock_ops_load_hdr_opt in the new timestamping
> > callbacks will return -EOPNOTSUPP error value.
>
> The "why" part is mostly missing. Why they are not safe to be used in the TX
> timestamping callbacks?
>
> >
> > It also prevents the UDP socket trying to access TCP fields in
> > the bpf extension for SO_TIMESTAMPING for the same consideration.
> Let's remove this UDP part to avoid confusion. UDP has very little to do with
> disabling the helpers here.
>
> "BPF_CALL" in the subject is not clear either. "BPF_CALL" can mean many things,
> such as calling BPF helpers, calling BPF kfuncs, or calling its own BPF
> subprograms, etc. In this case, it is the calling BPF helpers.
>
> (Subject)
> bpf: Disable unsafe helpers in TX timestamping callbacks
>
> (Why)
> New TX timestamping sock_ops callbacks will be added in the subsequent patch.
> Some of the existing BPF helpers will not be safe to be used in the TX
> timestamping callbacks.
>
> The bpf_sock_ops_setsockopt, bpf_sock_ops_getsockopt, and
> bpf_sock_ops_cb_flags_set require owning the sock lock. TX timestamping
> callbacks will not own the lock.
>
> The bpf_sock_ops_load_hdr_opt needs the skb->data pointing to the TCP header.
> This will not be true in the TX timestamping callbacks.
>
> (What and How)
> At the beginning of these helpers, this patch checks the bpf_sock->op to ensure
> these helpers are used by the existing sock_ops callbacks only.

Many thanks here! I will use them in the commit message.

Thanks,
Jason

Re: [PATCH bpf-next v9 12/12] selftests/bpf: add simple bpf tests in the tx path for timestamping feature

[Jason Xing]

On Tue, Feb 11, 2025 at 4:05 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 2/8/25 2:32 AM, Jason Xing wrote:
> > ---
> >   .../bpf/prog_tests/so_timestamping.c          |  79 +++++
> >   .../selftests/bpf/progs/so_timestamping.c     | 312 ++++++++++++++++++
>
> A bike shedding. s/so_timestamping.c/net_timestamping.c/

Will rename them.

>
> > diff --git a/tools/testing/selftests/bpf/progs/so_timestamping.c b/tools/testing/selftests/bpf/progs/so_timestamping.c
> > new file mode 100644
> > index 000000000000..4974552cdecb
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/so_timestamping.c
> > @@ -0,0 +1,312 @@
> > +#include "vmlinux.h"
> > +#include "bpf_tracing_net.h"
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +#include "bpf_misc.h"
> > +#include "bpf_kfuncs.h"
> > +#define BPF_PROG_TEST_TCP_HDR_OPTIONS
> > +#include "test_tcp_hdr_options.h"
> > +#include <errno.h>
> > +
> > +#define SK_BPF_CB_FLAGS 1009
> > +#define SK_BPF_CB_TX_TIMESTAMPING 1
> > +
> > +int nr_active;
> > +int nr_snd;
> > +int nr_passive;
> > +int nr_sched;
> > +int nr_txsw;
> > +int nr_ack;
> > +
> > +struct sockopt_test {
> > +     int opt;
> > +     int new;
> > +};
> > +
> > +static const struct sockopt_test sol_socket_tests[] = {
> > +     { .opt = SK_BPF_CB_FLAGS, .new = SK_BPF_CB_TX_TIMESTAMPING, },
> > +     { .opt = 0, },
> > +};
> > +
> > +struct loop_ctx {
> > +     void *ctx;
> > +     const struct sock *sk;
> > +};
> > +
> > +struct sk_stg {
> > +     __u64 sendmsg_ns;       /* record ts when sendmsg is called */
> > +};
> > +
> > +struct sk_tskey {
> > +     u64 cookie;
> > +     u32 tskey;
> > +};
> > +
> > +struct delay_info {
> > +     u64 sendmsg_ns;         /* record ts when sendmsg is called */
> > +     u32 sched_delay;        /* SCHED_OPT_CB - sendmsg_ns */
> > +     u32 sw_snd_delay;       /* SW_OPT_CB - SCHED_OPT_CB */
> > +     u32 ack_delay;          /* ACK_OPT_CB - SW_OPT_CB */
> > +};
> > +
> > +struct {
> > +     __uint(type, BPF_MAP_TYPE_SK_STORAGE);
> > +     __uint(map_flags, BPF_F_NO_PREALLOC);
> > +     __type(key, int);
> > +     __type(value, struct sk_stg);
> > +} sk_stg_map SEC(".maps");
> > +
> > +struct {
> > +     __uint(type, BPF_MAP_TYPE_HASH);
> > +     __type(key, struct sk_tskey);
> > +     __type(value, struct delay_info);
> > +     __uint(max_entries, 1024);
> > +} time_map SEC(".maps");
> > +
> > +static u64 delay_tolerance_nsec = 10000000000; /* 10 second as an example */
> > +
> > +extern int bpf_sock_ops_enable_tx_tstamp(struct bpf_sock_ops_kern *skops) __ksym;
> > +
> > +static int bpf_test_sockopt_int(void *ctx, const struct sock *sk,
> > +                             const struct sockopt_test *t,
> > +                             int level)
>
> This should be the only one that is needed even when supporting the future RX
> timestamping.
>
> TX and RX timestamping need to be tested independently. Looping it will either
> enabling them together or disabling them together. It cannot test whether RX
> will work by itself.
>
> Thus, the bpf_loop won't help. Lets remove it to simplify the test.

Got it. Will remove it.

>
> > +{
> > +     int new, opt, tmp;
> > +
> > +     opt = t->opt;
> > +     new = t->new;
> > +
> > +     if (bpf_setsockopt(ctx, level, opt, &new, sizeof(new)))
> > +             return 1;
> > +
> > +     if (bpf_getsockopt(ctx, level, opt, &tmp, sizeof(tmp)) ||
> > +         tmp != new)
> > +             return 1;
> > +
> > +     return 0;
> > +}
> > +
> > +static int bpf_test_socket_sockopt(__u32 i, struct loop_ctx *lc)
> > +{
> > +     const struct sockopt_test *t;
> > +
> > +     if (i >= ARRAY_SIZE(sol_socket_tests))
> > +             return 1;
> > +
> > +     t = &sol_socket_tests[i];
> > +     if (!t->opt)
> > +             return 1;
> > +
> > +     return bpf_test_sockopt_int(lc->ctx, lc->sk, t, SOL_SOCKET);
> > +}
> > +
> > +static int bpf_test_sockopt(void *ctx, const struct sock *sk)
> > +{
> > +     struct loop_ctx lc = { .ctx = ctx, .sk = sk, };
> > +     int n;
> > +
> > +     n = bpf_loop(ARRAY_SIZE(sol_socket_tests), bpf_test_socket_sockopt, &lc, 0);
> > +     if (n != ARRAY_SIZE(sol_socket_tests))
> > +             return -1;
> > +
> > +     return 0;
> > +}
> > +
> > +static bool bpf_test_access_sockopt(void *ctx)
> > +{
> > +     const struct sockopt_test *t;
> > +     int tmp, ret, i = 0;
> > +     int level = SOL_SOCKET;
> > +
> > +     t = &sol_socket_tests[i];
> > +
> > +     for (; t->opt;) {
>
> It really does not need a loop here. It only needs to test "one" optname to
> ensure it is -EOPNOTSUPP.
>
> > +             ret = bpf_setsockopt(ctx, level, t->opt, (void *)&t->new, sizeof(t->new));
> > +             if (ret != -EOPNOTSUPP)
> > +                     return true;
> > +
> > +             ret = bpf_getsockopt(ctx, level, t->opt, &tmp, sizeof(tmp));
> > +             if (ret != -EOPNOTSUPP)
> > +                     return true;
> > +
> > +             if (++i >= ARRAY_SIZE(sol_socket_tests))
> > +                     break;
> > +     }
> > +
> > +     return false;
> > +}
> > +
> > +/* Adding a simple test to see if we can get an expected value */
> > +static bool bpf_test_access_load_hdr_opt(struct bpf_sock_ops *skops)
> > +{
> > +     struct tcp_opt reg_opt;
>
> Just noticed this one. Use a plain u8 array. Then no need to include the
> "test_tcp_hdr_options.h" from an unrelated test.

Will update it.

Thanks,
Jason

>
> > +     int load_flags = 0;
> > +     int ret;
> > +
> > +     reg_opt.kind = TCPOPT_EXP;
>
> The kind could be any integer, e.g. 2.
>
> > +     reg_opt.len = 0;
> > +     reg_opt.data32 = 0;
> > +     ret = bpf_load_hdr_opt(skops, &reg_opt, sizeof(reg_opt), load_flags);
> > +     if (ret != -EOPNOTSUPP)
> > +             return true;
> > +
> > +     return false;
> > +}