diff mbox series

[net-next,v2,09/12] net-timestamp: add tx OPT_ID_TCP support for bpf case

Message ID 20241012040651.95616-10-kerneljasonxing@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net-timestamp: bpf extension to equip applications transparently | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 18 this patch: 18
netdev/build_tools success Errors and warnings before: 157 (+0) this patch: 157 (+0)
netdev/cc_maintainers warning 1 maintainers not CCed: horms@kernel.org
netdev/build_clang fail Errors and warnings before: 36 this patch: 39
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 2609 this patch: 2609
netdev/checkpatch warning WARNING: line length of 83 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 73 this patch: 73
netdev/source_inline success Was 0 now: 0

Commit Message

Jason Xing Oct. 12, 2024, 4:06 a.m. UTC 
From: Jason Xing <kernelxing@tencent.com>

We can set OPT_ID|OPT_ID_TCP before we initialize the last skb
from each sendmsg. We only set the socket once like how we use
setsockopt() with OPT_ID|OPT_ID_TCP flags.

Note: we will check if non-bpf _and_ bpf sk_tsflags have OPT_ID
flag. If either of them has been set before, we will not initialize
the key any more, or else it will affect the existing printing
from applications or BPF program behaviour.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 include/net/sock.h |  1 +
 net/core/filter.c  |  5 +++++
 net/core/skbuff.c  | 14 ++++++++++----
 net/core/sock.c    | 29 +++++++++++++++++++++--------
 4 files changed, 37 insertions(+), 12 deletions(-)

Comments

Willem de Bruijn Oct. 15, 2024, 1:38 a.m. UTC | #1 
Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> We can set OPT_ID|OPT_ID_TCP before we initialize the last skb
> from each sendmsg. We only set the socket once like how we use
> setsockopt() with OPT_ID|OPT_ID_TCP flags.
> 
> Note: we will check if non-bpf _and_ bpf sk_tsflags have OPT_ID
> flag. If either of them has been set before, we will not initialize
> the key any more, 

Where and how is this achieved?

Also be aware of the subtle distinction between passing OPT_ID_TCP
along with OPT_ID or not.
Jason Xing Oct. 15, 2024, 2:25 a.m. UTC | #2 
On Tue, Oct 15, 2024 at 9:38 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > We can set OPT_ID|OPT_ID_TCP before we initialize the last skb
> > from each sendmsg. We only set the socket once like how we use
> > setsockopt() with OPT_ID|OPT_ID_TCP flags.
> >
> > Note: we will check if non-bpf _and_ bpf sk_tsflags have OPT_ID
> > flag. If either of them has been set before, we will not initialize
> > the key any more,
>
> Where and how is this achieved?

Please see this patch and you will find the following codes.
+       tsflags |= (sk->sk_tsflags[SOCKETOPT_TS_REQUESTOR] |
+                   sk->sk_tsflags[BPFPROG_TS_REQUESTOR]);

But the difference/problem is that the non-bpf feature only init it
when connect() is done, but the bpf feature could do it at the
beginning of connect(). If running txtimestamp -l 1000, the former
will generate 999 for turkey while the latter 1000.

>
> Also be aware of the subtle distinction between passing OPT_ID_TCP
> along with OPT_ID or not.
>
>
Willem de Bruijn Oct. 15, 2024, 2:38 a.m. UTC | #3 
Jason Xing wrote:
> On Tue, Oct 15, 2024 at 9:38 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jason Xing wrote:
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > We can set OPT_ID|OPT_ID_TCP before we initialize the last skb
> > > from each sendmsg. We only set the socket once like how we use
> > > setsockopt() with OPT_ID|OPT_ID_TCP flags.
> > >
> > > Note: we will check if non-bpf _and_ bpf sk_tsflags have OPT_ID
> > > flag. If either of them has been set before, we will not initialize
> > > the key any more,
> >
> > Where and how is this achieved?
> 
> Please see this patch and you will find the following codes.
> +       tsflags |= (sk->sk_tsflags[SOCKETOPT_TS_REQUESTOR] |
> +                   sk->sk_tsflags[BPFPROG_TS_REQUESTOR]);

I saw that, but it's not a condition that stops reinitializing. Which
I think is the intent, based on "If either of them has been set
before, we will not initialize the key anymore"?

Reinitialization is actually supported behavior.

                if (val & SOF_TIMESTAMPING_OPT_ID &&
                    !(sk->sk_tsflags & SOF_TIMESTAMPING_OPT_ID)) {

But the sk_tsflags bit may be repeatedly set and cleared.

Anyway, the current patch sets it if either requests it?

+	tsflags |= (sk->sk_tsflags[SOCKETOPT_TS_REQUESTOR] |
+		    sk->sk_tsflags[BPFPROG_TS_REQUESTOR]);
 	if (val & SOF_TIMESTAMPING_OPT_ID &&
-	    !(sk->sk_tsflags[SOCKETOPT_TS_REQUESTOR] & SOF_TIMESTAMPING_OPT_ID)) {
+	    !(tsflags & SOF_TIMESTAMPING_OPT_ID)) {

 
> But the difference/problem is that the non-bpf feature only init it
> when connect() is done, but the bpf feature could do it at the
> beginning of connect(). If running txtimestamp -l 1000, the former
> will generate 999 for turkey while the latter 1000.
Jason Xing Oct. 15, 2024, 2:59 a.m. UTC | #4 
On Tue, Oct 15, 2024 at 10:38 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > On Tue, Oct 15, 2024 at 9:38 AM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Jason Xing wrote:
> > > > From: Jason Xing <kernelxing@tencent.com>
> > > >
> > > > We can set OPT_ID|OPT_ID_TCP before we initialize the last skb
> > > > from each sendmsg. We only set the socket once like how we use
> > > > setsockopt() with OPT_ID|OPT_ID_TCP flags.
> > > >
> > > > Note: we will check if non-bpf _and_ bpf sk_tsflags have OPT_ID
> > > > flag. If either of them has been set before, we will not initialize
> > > > the key any more,
> > >
> > > Where and how is this achieved?
> >
> > Please see this patch and you will find the following codes.
> > +       tsflags |= (sk->sk_tsflags[SOCKETOPT_TS_REQUESTOR] |
> > +                   sk->sk_tsflags[BPFPROG_TS_REQUESTOR]);
>
> I saw that, but it's not a condition that stops reinitializing. Which
> I think is the intent, based on "If either of them has been set
> before, we will not initialize the key anymore"?

Yep, based on that sentence. If we find sk_tsflags is initialized,
then we will not do the same thing to sk_tskey again when we use bpf
method.

>
> Reinitialization is actually supported behavior.
>
>                 if (val & SOF_TIMESTAMPING_OPT_ID &&
>                     !(sk->sk_tsflags & SOF_TIMESTAMPING_OPT_ID)) {
>
> But the sk_tsflags bit may be repeatedly set and cleared.

This line "!(sk->sk_tsflags & SOF_TIMESTAMPING_OPT_ID)) {" was removed
and replaced in the new function sock_set_tskey(). So it could avoid
re-initialization.

>
> Anyway, the current patch sets it if either requests it?

Yep, either of the ways (bpf and non-bpf) can init it.
kernel test robot Oct. 15, 2024, 8:40 a.m. UTC | #5 
Hi Jason,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Jason-Xing/net-timestamp-introduce-socket-tsflag-requestors/20241012-121010
base:   net-next/main
patch link:    https://lore.kernel.org/r/20241012040651.95616-10-kerneljasonxing%40gmail.com
patch subject: [PATCH net-next v2 09/12] net-timestamp: add tx OPT_ID_TCP support for bpf case
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20241015/202410151628.hcAdeahi-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241015/202410151628.hcAdeahi-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410151628.hcAdeahi-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> net/core/sock.c:926:2: warning: variable 'tsflags' is uninitialized when used here [-Wuninitialized]
     926 |         tsflags |= (sk->sk_tsflags[SOCKETOPT_TS_REQUESTOR] |
         |         ^~~~~~~
   net/core/sock.c:920:13: note: initialize the variable 'tsflags' to silence this warning
     920 |         u32 tsflags;
         |                    ^
         |                     = 0
   1 warning generated.


vim +/tsflags +926 net/core/sock.c

   917	
   918	int sock_set_tskey(struct sock *sk, int val, int type)
   919	{
   920		u32 tsflags;
   921	
   922		if (val & SOF_TIMESTAMPING_OPT_ID_TCP &&
   923		    !(val & SOF_TIMESTAMPING_OPT_ID))
   924			return -EINVAL;
   925	
 > 926		tsflags |= (sk->sk_tsflags[SOCKETOPT_TS_REQUESTOR] |
   927			    sk->sk_tsflags[BPFPROG_TS_REQUESTOR]);
   928		if (val & SOF_TIMESTAMPING_OPT_ID &&
   929		    !(tsflags & SOF_TIMESTAMPING_OPT_ID)) {
   930			if (sk_is_tcp(sk)) {
   931				if ((1 << sk->sk_state) &
   932				    (TCPF_CLOSE | TCPF_LISTEN))
   933					return -EINVAL;
   934				if (val & SOF_TIMESTAMPING_OPT_ID_TCP)
   935					atomic_set(&sk->sk_tskey, tcp_sk(sk)->write_seq);
   936				else
   937					atomic_set(&sk->sk_tskey, tcp_sk(sk)->snd_una);
   938			} else {
   939				atomic_set(&sk->sk_tskey, 0);
   940			}
   941		}
   942	
   943		return 0;
   944	}
   945
Jason Xing Oct. 15, 2024, 9:36 a.m. UTC | #6 
On Tue, Oct 15, 2024 at 4:41 PM kernel test robot <lkp@intel.com> wrote:
>
> Hi Jason,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on net-next/main]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Jason-Xing/net-timestamp-introduce-socket-tsflag-requestors/20241012-121010
> base:   net-next/main
> patch link:    https://lore.kernel.org/r/20241012040651.95616-10-kerneljasonxing%40gmail.com
> patch subject: [PATCH net-next v2 09/12] net-timestamp: add tx OPT_ID_TCP support for bpf case
> config: x86_64-kexec (https://download.01.org/0day-ci/archive/20241015/202410151628.hcAdeahi-lkp@intel.com/config)
> compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241015/202410151628.hcAdeahi-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202410151628.hcAdeahi-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
> >> net/core/sock.c:926:2: warning: variable 'tsflags' is uninitialized when used here [-Wuninitialized]
>      926 |         tsflags |= (sk->sk_tsflags[SOCKETOPT_TS_REQUESTOR] |
>          |         ^~~~~~~
>    net/core/sock.c:920:13: note: initialize the variable 'tsflags' to silence this warning
>      920 |         u32 tsflags;
>          |                    ^
>          |                     = 0
>    1 warning generated.

Thanks! I will fix it!
diff mbox series

Patch

diff --git a/include/net/sock.h b/include/net/sock.h
index b7c51b95c92d..2b4ac289c8fa 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2893,6 +2893,7 @@  DECLARE_STATIC_KEY_FALSE(bpf_tstamp_control);
 void sock_set_timestamp(struct sock *sk, int optname, bool valbool);
 int sock_get_timestamping(struct so_timestamping *timestamping,
 			  sockptr_t optval, unsigned int optlen);
+int sock_set_tskey(struct sock *sk, int val, int type);
 int sock_set_timestamping(struct sock *sk, int optname,
 			  struct so_timestamping timestamping);
 
diff --git a/net/core/filter.c b/net/core/filter.c
index 08135f538c99..3b4afaa273d9 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5210,6 +5210,7 @@  static int bpf_sock_set_timestamping(struct sock *sk,
 				     struct so_timestamping *timestamping)
 {
 	u32 flags = timestamping->flags;
+	int ret;
 
 	if (flags & ~SOF_TIMESTAMPING_MASK)
 		return -EINVAL;
@@ -5218,6 +5219,10 @@  static int bpf_sock_set_timestamping(struct sock *sk,
 	      SOF_TIMESTAMPING_TX_ACK)))
 		return -EINVAL;
 
+	ret = sock_set_tskey(sk, flags, BPFPROG_TS_REQUESTOR);
+	if (ret)
+		return ret;
+
 	WRITE_ONCE(sk->sk_tsflags[BPFPROG_TS_REQUESTOR], flags);
 	static_branch_enable(&bpf_tstamp_control);
 
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index e18305b03a01..1ef379a87f88 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5619,7 +5619,7 @@  static void skb_tstamp_tx_output(struct sk_buff *orig_skb,
 	__skb_complete_tx_timestamp(skb, sk, tstype, opt_stats);
 }
 
-static void bpf_skb_tstamp_tx_output(struct sock *sk, int tstype,
+static void bpf_skb_tstamp_tx_output(struct sock *sk, struct sk_buff *skb, int tstype,
 				     struct skb_shared_hwtstamps *hwtstamps)
 {
 	struct tcp_sock *tp;
@@ -5635,7 +5635,7 @@  static void bpf_skb_tstamp_tx_output(struct sock *sk, int tstype,
 	tp = tcp_sk(sk);
 	if (BPF_SOCK_OPS_TEST_FLAG(tp, BPF_SOCK_OPS_TX_TIMESTAMPING_OPT_CB_FLAG)) {
 		struct timespec64 tstamp;
-		u32 cb_flag;
+		u32 cb_flag, key = 0;
 
 		switch (tstype) {
 		case SCM_TSTAMP_SCHED:
@@ -5651,11 +5651,17 @@  static void bpf_skb_tstamp_tx_output(struct sock *sk, int tstype,
 			return;
 		}
 
+		if (sk_is_tcp(sk)) {
+			key = skb_shinfo(skb)->tskey;
+			key -= atomic_read(&sk->sk_tskey);
+		}
+
 		if (hwtstamps)
 			tstamp = ktime_to_timespec64(hwtstamps->hwtstamp);
 		else
 			tstamp = ktime_to_timespec64(ktime_get_real());
-		tcp_call_bpf_2arg(sk, cb_flag, tstamp.tv_sec, tstamp.tv_nsec);
+
+		tcp_call_bpf_3arg(sk, cb_flag, key, tstamp.tv_sec, tstamp.tv_nsec);
 	}
 }
 
@@ -5668,7 +5674,7 @@  void __skb_tstamp_tx(struct sk_buff *orig_skb,
 		return;
 
 	if (static_branch_unlikely(&bpf_tstamp_control))
-		bpf_skb_tstamp_tx_output(sk, tstype, hwtstamps);
+		bpf_skb_tstamp_tx_output(sk, orig_skb, tstype, hwtstamps);
 
 	skb_tstamp_tx_output(orig_skb, ack_skb, hwtstamps, sk, tstype);
 }
diff --git a/net/core/sock.c b/net/core/sock.c
index a6e0d51a5f72..c15edbd382d5 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -915,21 +915,18 @@  int sock_get_timestamping(struct so_timestamping *timestamping,
 	return 0;
 }
 
-int sock_set_timestamping(struct sock *sk, int optname,
-			  struct so_timestamping timestamping)
+int sock_set_tskey(struct sock *sk, int val, int type)
 {
-	int val = timestamping.flags;
-	int ret;
-
-	if (val & ~SOF_TIMESTAMPING_MASK)
-		return -EINVAL;
+	u32 tsflags;
 
 	if (val & SOF_TIMESTAMPING_OPT_ID_TCP &&
 	    !(val & SOF_TIMESTAMPING_OPT_ID))
 		return -EINVAL;
 
+	tsflags |= (sk->sk_tsflags[SOCKETOPT_TS_REQUESTOR] |
+		    sk->sk_tsflags[BPFPROG_TS_REQUESTOR]);
 	if (val & SOF_TIMESTAMPING_OPT_ID &&
-	    !(sk->sk_tsflags[SOCKETOPT_TS_REQUESTOR] & SOF_TIMESTAMPING_OPT_ID)) {
+	    !(tsflags & SOF_TIMESTAMPING_OPT_ID)) {
 		if (sk_is_tcp(sk)) {
 			if ((1 << sk->sk_state) &
 			    (TCPF_CLOSE | TCPF_LISTEN))
@@ -943,6 +940,22 @@  int sock_set_timestamping(struct sock *sk, int optname,
 		}
 	}
 
+	return 0;
+}
+
+int sock_set_timestamping(struct sock *sk, int optname,
+			  struct so_timestamping timestamping)
+{
+	int val = timestamping.flags;
+	int ret;
+
+	if (val & ~SOF_TIMESTAMPING_MASK)
+		return -EINVAL;
+
+	ret = sock_set_tskey(sk, val, SOCKETOPT_TS_REQUESTOR);
+	if (ret)
+		return ret;
+
 	if (val & SOF_TIMESTAMPING_OPT_STATS &&
 	    !(val & SOF_TIMESTAMPING_OPT_TSONLY))
 		return -EINVAL;