[PATCH net-next v2 0/2] timestamp: control SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[PATCH net-next v2 0/2] timestamp: control SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Prior to this series, when one socket is set SOF_TIMESTAMPING_RX_SOFTWARE
which measn the whole system turns on this button, other sockets that only
have SOF_TIMESTAMPING_SOFTWARE will be affected and then print the rx
timestamp information even without SOF_TIMESTAMPING_RX_SOFTWARE flag.
In such a case, the rxtimestamp.c selftest surely fails, please see
testcase 6.

In a normal case, if we only set SOF_TIMESTAMPING_SOFTWARE flag, we
can't get the rx timestamp because there is no path leading to turn on
netstamp_needed_key button in net_enable_timestamp(). That is to say, if
the user only sets SOF_TIMESTAMPING_SOFTWARE, we don't expect we are
able to fetch the timestamp from the skb.

More than this, we can find there are some other ways to turn on
netstamp_needed_key, which will happenly allow users to get tstamp in
the receive path. Please see net_enable_timestamp().

How to solve it?

setsockopt interface is used to control each socket separately but in
this case it is affected by other sockets. For timestamp itself, it's
not feasible to convert netstamp_needed_key into a per-socket button
because when the receive stack just handling the skb from driver doesn't
know which socket the skb belongs to.

According to the original design, we should not use both generation flag
(SOF_TIMESTAMPING_RX_SOFTWARE) and report flag (SOF_TIMESTAMPING_SOFTWARE)
together to test if the application is allowed to receive the timestamp
report in the receive path. But it doesn't hold for receive timestamping
case. We have to make an exception.

So we have to test the generation flag when the applications do recvmsg:
if we set both of flags, it means we want the timestamp; if not, it means
we don't expect to see the timestamp even the skb carries.

As we can see, this patch makes the SOF_TIMESTAMPING_RX_SOFTWARE under
setsockopt control. And it's a per-socket fine-grained now.

v2
Link: https://lore.kernel.org/all/20240825152440.93054-1-kerneljasonxing@gmail.com/
Discussed with Willem
1. update the documentation accordingly
2. add more comments in each patch
3. remove the previous test statements in __sock_recv_timestamp()

Jason Xing (2):
  tcp: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket
  net: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

 Documentation/networking/timestamping.rst |  7 +++++++
 include/net/sock.h                        |  7 ++++---
 net/bluetooth/hci_sock.c                  |  4 ++--
 net/core/sock.c                           |  2 +-
 net/ipv4/ip_sockglue.c                    |  2 +-
 net/ipv4/ping.c                           |  2 +-
 net/ipv4/tcp.c                            | 11 +++++++++--
 net/ipv6/datagram.c                       |  4 ++--
 net/l2tp/l2tp_ip.c                        |  2 +-
 net/l2tp/l2tp_ip6.c                       |  2 +-
 net/nfc/llcp_sock.c                       |  2 +-
 net/rxrpc/recvmsg.c                       |  2 +-
 net/socket.c                              | 11 ++++++++---
 net/unix/af_unix.c                        |  2 +-
 14 files changed, 40 insertions(+), 20 deletions(-)

-- 
2.37.3

[PATCH net-next v2 1/2] tcp: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Normally, if we want to record and print the rx timestamp after
tcp_recvmsg_locked(), we must enable both SOF_TIMESTAMPING_SOFTWARE
and SOF_TIMESTAMPING_RX_SOFTWARE flags, from which we also can notice
through running rxtimestamp binary in selftests (see testcase 7).

However, there is one particular case that fails the selftests with
"./rxtimestamp: Expected swtstamp to not be set." error printing in
testcase 6.

How does it happen? When we keep running a thread starting a socket
and set SOF_TIMESTAMPING_RX_SOFTWARE option first, then running
./rxtimestamp, it will fail. The reason is the former thread
switching on netstamp_needed_key that makes the feature global,
every skb going through netif_receive_skb_list_internal() function
will get a current timestamp in net_timestamp_check(). So the skb
will have timestamp regardless of whether its socket option has
SOF_TIMESTAMPING_RX_SOFTWARE or not.

After this patch, we can pass the selftest and control each socket
as we want when using rx timestamp feature.

Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 net/ipv4/tcp.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 8514257f4ecd..5e88c765b9a1 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2235,6 +2235,7 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
 			struct scm_timestamping_internal *tss)
 {
 	int new_tstamp = sock_flag(sk, SOCK_TSTAMP_NEW);
+	u32 tsflags = READ_ONCE(sk->sk_tsflags);
 	bool has_timestamping = false;
 
 	if (tss->ts[0].tv_sec || tss->ts[0].tv_nsec) {
@@ -2274,14 +2275,20 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
 			}
 		}
 
-		if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
+		/* We have to use the generation flag here to test if we
+		 * allow the corresponding application to receive the rx
+		 * timestamp. Only using report flag does not hold for
+		 * receive timestamping case.
+		 */
+		if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
+		    tsflags & SOF_TIMESTAMPING_RX_SOFTWARE)
 			has_timestamping = true;
 		else
 			tss->ts[0] = (struct timespec64) {0};
 	}
 
 	if (tss->ts[2].tv_sec || tss->ts[2].tv_nsec) {
-		if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_RAW_HARDWARE)
+		if (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)
 			has_timestamping = true;
 		else
 			tss->ts[2] = (struct timespec64) {0};
-- 
2.37.3

Re: [PATCH net-next v2 1/2] tcp: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[Willem de Bruijn]

Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> Normally, if we want to record and print the rx timestamp after
> tcp_recvmsg_locked(), we must enable both SOF_TIMESTAMPING_SOFTWARE
> and SOF_TIMESTAMPING_RX_SOFTWARE flags, from which we also can notice
> through running rxtimestamp binary in selftests (see testcase 7).
> 
> However, there is one particular case that fails the selftests with
> "./rxtimestamp: Expected swtstamp to not be set." error printing in
> testcase 6.
> 
> How does it happen? When we keep running a thread starting a socket
> and set SOF_TIMESTAMPING_RX_SOFTWARE option first, then running
> ./rxtimestamp, it will fail. The reason is the former thread
> switching on netstamp_needed_key that makes the feature global,
> every skb going through netif_receive_skb_list_internal() function
> will get a current timestamp in net_timestamp_check(). So the skb
> will have timestamp regardless of whether its socket option has
> SOF_TIMESTAMPING_RX_SOFTWARE or not.
> 
> After this patch, we can pass the selftest and control each socket
> as we want when using rx timestamp feature.
> 
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  net/ipv4/tcp.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 8514257f4ecd..5e88c765b9a1 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2235,6 +2235,7 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
>  			struct scm_timestamping_internal *tss)
>  {
>  	int new_tstamp = sock_flag(sk, SOCK_TSTAMP_NEW);
> +	u32 tsflags = READ_ONCE(sk->sk_tsflags);
>  	bool has_timestamping = false;
>  
>  	if (tss->ts[0].tv_sec || tss->ts[0].tv_nsec) {
> @@ -2274,14 +2275,20 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
>  			}
>  		}
>  
> -		if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> +		/* We have to use the generation flag here to test if we
> +		 * allow the corresponding application to receive the rx
> +		 * timestamp. Only using report flag does not hold for
> +		 * receive timestamping case.
> +		 */

Nit: what does "does not hold" mean here? I don't think a casual reader
will be able to parse this comment and understand it.

Perhaps something along the lines of

"Test both reporting and generation flag, to filter out false
positives where the process asked only for tx software timestamps and
another process enabled receive software timestamp generation."

> +		if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> +		    tsflags & SOF_TIMESTAMPING_RX_SOFTWARE)
>  			has_timestamping = true;
>  		else
>  			tss->ts[0] = (struct timespec64) {0};
>  	}
>  
>  	if (tss->ts[2].tv_sec || tss->ts[2].tv_nsec) {
> -		if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_RAW_HARDWARE)
> +		if (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)
>  			has_timestamping = true;
>  		else
>  			tss->ts[2] = (struct timespec64) {0};
> -- 
> 2.37.3
> 

Re: [PATCH net-next v2 1/2] tcp: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[Jason Xing]

On Thu, Aug 29, 2024 at 10:16 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Normally, if we want to record and print the rx timestamp after
> > tcp_recvmsg_locked(), we must enable both SOF_TIMESTAMPING_SOFTWARE
> > and SOF_TIMESTAMPING_RX_SOFTWARE flags, from which we also can notice
> > through running rxtimestamp binary in selftests (see testcase 7).
> >
> > However, there is one particular case that fails the selftests with
> > "./rxtimestamp: Expected swtstamp to not be set." error printing in
> > testcase 6.
> >
> > How does it happen? When we keep running a thread starting a socket
> > and set SOF_TIMESTAMPING_RX_SOFTWARE option first, then running
> > ./rxtimestamp, it will fail. The reason is the former thread
> > switching on netstamp_needed_key that makes the feature global,
> > every skb going through netif_receive_skb_list_internal() function
> > will get a current timestamp in net_timestamp_check(). So the skb
> > will have timestamp regardless of whether its socket option has
> > SOF_TIMESTAMPING_RX_SOFTWARE or not.
> >
> > After this patch, we can pass the selftest and control each socket
> > as we want when using rx timestamp feature.
> >
> > Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> >  net/ipv4/tcp.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 8514257f4ecd..5e88c765b9a1 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -2235,6 +2235,7 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
> >                       struct scm_timestamping_internal *tss)
> >  {
> >       int new_tstamp = sock_flag(sk, SOCK_TSTAMP_NEW);
> > +     u32 tsflags = READ_ONCE(sk->sk_tsflags);
> >       bool has_timestamping = false;
> >
> >       if (tss->ts[0].tv_sec || tss->ts[0].tv_nsec) {
> > @@ -2274,14 +2275,20 @@ void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
> >                       }
> >               }
> >
> > -             if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_SOFTWARE)
> > +             /* We have to use the generation flag here to test if we
> > +              * allow the corresponding application to receive the rx
> > +              * timestamp. Only using report flag does not hold for
> > +              * receive timestamping case.
> > +              */
>
> Nit: what does "does not hold" mean here? I don't think a casual reader
> will be able to parse this comment and understand it.

“hold for” can be a fixed collocation, which means "be suitable for"?
I'm not that sure. I was trying to say "only using the report flag
cannot meet our needs" something like this.

>
> Perhaps something along the lines of
>
> "Test both reporting and generation flag, to filter out false
> positives where the process asked only for tx software timestamps and
> another process enabled receive software timestamp generation."

Thanks, it's much better than mine. I will use it.

Thanks,
Jason

>
> > +             if (tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> > +                 tsflags & SOF_TIMESTAMPING_RX_SOFTWARE)
> >                       has_timestamping = true;
> >               else
> >                       tss->ts[0] = (struct timespec64) {0};
> >       }
> >
> >       if (tss->ts[2].tv_sec || tss->ts[2].tv_nsec) {
> > -             if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_RAW_HARDWARE)
> > +             if (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)
> >                       has_timestamping = true;
> >               else
> >                       tss->ts[2] = (struct timespec64) {0};
> > --
> > 2.37.3
> >
>
>

[PATCH net-next v2 2/2] net: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Like the previous patch in this series, we need to make sure that
we both set SOF_TIMESTAMPING_SOFTWARE and SOF_TIMESTAMPING_RX_SOFTWARE
flags together so that we can let the user parse the rx timestamp.

One more important and special thing is that we should take care of
errqueue recv path because we rely on errqueue to get our timestamps
for sendmsg(). Or else, If the user wants to read when setting
SOF_TIMESTAMPING_TX_ACK, something like this, we cannot get timestamps,
for example, in TCP case. So we should consider those
SOF_TIMESTAMPING_TX_* flags.

After this patch, we are able to pass the testcase 6 for IP and UDP
cases when running ./rxtimestamp binary.

Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 Documentation/networking/timestamping.rst |  7 +++++++
 include/net/sock.h                        |  7 ++++---
 net/bluetooth/hci_sock.c                  |  4 ++--
 net/core/sock.c                           |  2 +-
 net/ipv4/ip_sockglue.c                    |  2 +-
 net/ipv4/ping.c                           |  2 +-
 net/ipv6/datagram.c                       |  4 ++--
 net/l2tp/l2tp_ip.c                        |  2 +-
 net/l2tp/l2tp_ip6.c                       |  2 +-
 net/nfc/llcp_sock.c                       |  2 +-
 net/rxrpc/recvmsg.c                       |  2 +-
 net/socket.c                              | 11 ++++++++---
 net/unix/af_unix.c                        |  2 +-
 13 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
index 5e93cd71f99f..93378b78c6dd 100644
--- a/Documentation/networking/timestamping.rst
+++ b/Documentation/networking/timestamping.rst
@@ -160,6 +160,13 @@ SOF_TIMESTAMPING_RAW_HARDWARE:
   Report hardware timestamps as generated by
   SOF_TIMESTAMPING_TX_HARDWARE when available.
 
+Please note: previously, if an application starts first which turns on
+netstamp_needed_key, then another one only passing SOF_TIMESTAMPING_SOFTWARE
+could also get rx timestamp. Now we handle this case and will not get
+rx timestamp under this circumstance. We encourage that for each socket
+we should use the SOF_TIMESTAMPING_RX_SOFTWARE generation flag to time
+stamp the skb and use SOF_TIMESTAMPING_SOFTWARE report flag to tell
+the application.
 
 1.3.3 Timestamp Options
 ^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/include/net/sock.h b/include/net/sock.h
index cce23ac4d514..b8535692f340 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2600,12 +2600,13 @@ static inline void sock_write_timestamp(struct sock *sk, ktime_t kt)
 }
 
 void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
-			   struct sk_buff *skb);
+			   struct sk_buff *skb, bool errqueue);
 void __sock_recv_wifi_status(struct msghdr *msg, struct sock *sk,
 			     struct sk_buff *skb);
 
 static inline void
-sock_recv_timestamp(struct msghdr *msg, struct sock *sk, struct sk_buff *skb)
+sock_recv_timestamp(struct msghdr *msg, struct sock *sk, struct sk_buff *skb,
+		    bool errqueue)
 {
 	struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
 	u32 tsflags = READ_ONCE(sk->sk_tsflags);
@@ -2621,7 +2622,7 @@ sock_recv_timestamp(struct msghdr *msg, struct sock *sk, struct sk_buff *skb)
 	    (kt && tsflags & SOF_TIMESTAMPING_SOFTWARE) ||
 	    (hwtstamps->hwtstamp &&
 	     (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)))
-		__sock_recv_timestamp(msg, sk, skb);
+		__sock_recv_timestamp(msg, sk, skb, errqueue);
 	else
 		sock_write_timestamp(sk, kt);
 
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 69c2ba1e843e..c1b73c5a370b 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -1586,11 +1586,11 @@ static int hci_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 		break;
 	case HCI_CHANNEL_USER:
 	case HCI_CHANNEL_MONITOR:
-		sock_recv_timestamp(msg, sk, skb);
+		sock_recv_timestamp(msg, sk, skb, false);
 		break;
 	default:
 		if (hci_mgmt_chan_find(hci_pi(sk)->channel))
-			sock_recv_timestamp(msg, sk, skb);
+			sock_recv_timestamp(msg, sk, skb, false);
 		break;
 	}
 
diff --git a/net/core/sock.c b/net/core/sock.c
index 9abc4fe25953..d969a4901300 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -3677,7 +3677,7 @@ int sock_recv_errqueue(struct sock *sk, struct msghdr *msg, int len,
 	if (err)
 		goto out_free_skb;
 
-	sock_recv_timestamp(msg, sk, skb);
+	sock_recv_timestamp(msg, sk, skb, true);
 
 	serr = SKB_EXT_ERR(skb);
 	put_cmsg(msg, level, type, sizeof(serr->ee), &serr->ee);
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index cf377377b52d..b79f859c34bf 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -547,7 +547,7 @@ int ip_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len)
 		kfree_skb(skb);
 		return err;
 	}
-	sock_recv_timestamp(msg, sk, skb);
+	sock_recv_timestamp(msg, sk, skb, true);
 
 	serr = SKB_EXT_ERR(skb);
 
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 619ddc087957..1cf7b0eecd63 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -880,7 +880,7 @@ int ping_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int flags,
 	if (err)
 		goto done;
 
-	sock_recv_timestamp(msg, sk, skb);
+	sock_recv_timestamp(msg, sk, skb, false);
 
 	/* Copy the address and add cmsg data. */
 	if (family == AF_INET) {
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index fff78496803d..1e4c11b2d0ce 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -479,7 +479,7 @@ int ipv6_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len)
 		kfree_skb(skb);
 		return err;
 	}
-	sock_recv_timestamp(msg, sk, skb);
+	sock_recv_timestamp(msg, sk, skb, true);
 
 	serr = SKB_EXT_ERR(skb);
 
@@ -568,7 +568,7 @@ int ipv6_recv_rxpmtu(struct sock *sk, struct msghdr *msg, int len,
 	if (err)
 		goto out_free_skb;
 
-	sock_recv_timestamp(msg, sk, skb);
+	sock_recv_timestamp(msg, sk, skb, false);
 
 	memcpy(&mtu_info, IP6CBMTU(skb), sizeof(mtu_info));
 
diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index 4bc24fddfd52..164c8ed7124e 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -567,7 +567,7 @@ static int l2tp_ip_recvmsg(struct sock *sk, struct msghdr *msg,
 	if (err)
 		goto done;
 
-	sock_recv_timestamp(msg, sk, skb);
+	sock_recv_timestamp(msg, sk, skb, false);
 
 	/* Copy the address. */
 	if (sin) {
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index f4c1da070826..b0bb0a1f772e 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -712,7 +712,7 @@ static int l2tp_ip6_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 	if (err)
 		goto done;
 
-	sock_recv_timestamp(msg, sk, skb);
+	sock_recv_timestamp(msg, sk, skb, false);
 
 	/* Copy the address. */
 	if (lsa) {
diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
index 57a2f97004e1..5c6e671643f6 100644
--- a/net/nfc/llcp_sock.c
+++ b/net/nfc/llcp_sock.c
@@ -869,7 +869,7 @@ static int llcp_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 		return -EFAULT;
 	}
 
-	sock_recv_timestamp(msg, sk, skb);
+	sock_recv_timestamp(msg, sk, skb, false);
 
 	if (sk->sk_type == SOCK_DGRAM && msg->msg_name) {
 		struct nfc_llcp_ui_cb *ui_cb = nfc_llcp_ui_skb_cb(skb);
diff --git a/net/rxrpc/recvmsg.c b/net/rxrpc/recvmsg.c
index a482f88c5fc5..18fa392011fb 100644
--- a/net/rxrpc/recvmsg.c
+++ b/net/rxrpc/recvmsg.c
@@ -200,7 +200,7 @@ static int rxrpc_recvmsg_data(struct socket *sock, struct rxrpc_call *call,
 					    sp->hdr.serial, seq);
 
 		if (msg)
-			sock_recv_timestamp(msg, sock->sk, skb);
+			sock_recv_timestamp(msg, sock->sk, skb, false);
 
 		if (rx_pkt_offset == 0) {
 			ret2 = rxrpc_verify_data(call, skb);
diff --git a/net/socket.c b/net/socket.c
index fcbdd5bc47ac..c02fb9b615b2 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -893,7 +893,7 @@ static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb,
  * called from sock_recv_timestamp() if sock_flag(sk, SOCK_RCVTSTAMP)
  */
 void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
-	struct sk_buff *skb)
+			   struct sk_buff *skb, bool errqueue)
 {
 	int need_software_tstamp = sock_flag(sk, SOCK_RCVTSTAMP);
 	int new_tstamp = sock_flag(sk, SOCK_TSTAMP_NEW);
@@ -946,7 +946,12 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
 
 	memset(&tss, 0, sizeof(tss));
 	tsflags = READ_ONCE(sk->sk_tsflags);
-	if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
+	/* We have to use the generation flag here to test if we allow the
+	 * corresponding application to receive the rx timestamp. Only
+	 * using report flag does not hold for receive timestamping case.
+	 */
+	if ((tsflags & SOF_TIMESTAMPING_SOFTWARE &&
+	     (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE || errqueue)) &&
 	    ktime_to_timespec64_cond(skb->tstamp, tss.ts + 0))
 		empty = 0;
 	if (shhwtstamps &&
@@ -1024,7 +1029,7 @@ static void sock_recv_mark(struct msghdr *msg, struct sock *sk,
 void __sock_recv_cmsgs(struct msghdr *msg, struct sock *sk,
 		       struct sk_buff *skb)
 {
-	sock_recv_timestamp(msg, sk, skb);
+	sock_recv_timestamp(msg, sk, skb, false);
 	sock_recv_drops(msg, sk, skb);
 	sock_recv_mark(msg, sk, skb);
 }
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index a1894019ebd5..bb33f2994618 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2481,7 +2481,7 @@ int __unix_dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t size,
 		goto out_free;
 
 	if (sock_flag(sk, SOCK_RCVTSTAMP))
-		__sock_recv_timestamp(msg, sk, skb);
+		__sock_recv_timestamp(msg, sk, skb, false);
 
 	memset(&scm, 0, sizeof(scm));
 
-- 
2.37.3

Re: [PATCH net-next v2 2/2] net: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[Jason Xing]

On Thu, Aug 29, 2024 at 12:01 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> Like the previous patch in this series, we need to make sure that
> we both set SOF_TIMESTAMPING_SOFTWARE and SOF_TIMESTAMPING_RX_SOFTWARE
> flags together so that we can let the user parse the rx timestamp.
>
> One more important and special thing is that we should take care of
> errqueue recv path because we rely on errqueue to get our timestamps
> for sendmsg(). Or else, If the user wants to read when setting
> SOF_TIMESTAMPING_TX_ACK, something like this, we cannot get timestamps,
> for example, in TCP case. So we should consider those
> SOF_TIMESTAMPING_TX_* flags.
>
> After this patch, we are able to pass the testcase 6 for IP and UDP
> cases when running ./rxtimestamp binary.
>
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  Documentation/networking/timestamping.rst |  7 +++++++
>  include/net/sock.h                        |  7 ++++---
>  net/bluetooth/hci_sock.c                  |  4 ++--
>  net/core/sock.c                           |  2 +-
>  net/ipv4/ip_sockglue.c                    |  2 +-
>  net/ipv4/ping.c                           |  2 +-
>  net/ipv6/datagram.c                       |  4 ++--
>  net/l2tp/l2tp_ip.c                        |  2 +-
>  net/l2tp/l2tp_ip6.c                       |  2 +-
>  net/nfc/llcp_sock.c                       |  2 +-
>  net/rxrpc/recvmsg.c                       |  2 +-
>  net/socket.c                              | 11 ++++++++---
>  net/unix/af_unix.c                        |  2 +-
>  13 files changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> index 5e93cd71f99f..93378b78c6dd 100644
> --- a/Documentation/networking/timestamping.rst
> +++ b/Documentation/networking/timestamping.rst
> @@ -160,6 +160,13 @@ SOF_TIMESTAMPING_RAW_HARDWARE:
>    Report hardware timestamps as generated by
>    SOF_TIMESTAMPING_TX_HARDWARE when available.
>
> +Please note: previously, if an application starts first which turns on
> +netstamp_needed_key, then another one only passing SOF_TIMESTAMPING_SOFTWARE
> +could also get rx timestamp. Now we handle this case and will not get
> +rx timestamp under this circumstance. We encourage that for each socket
> +we should use the SOF_TIMESTAMPING_RX_SOFTWARE generation flag to time
> +stamp the skb and use SOF_TIMESTAMPING_SOFTWARE report flag to tell
> +the application.
>
>  1.3.3 Timestamp Options
>  ^^^^^^^^^^^^^^^^^^^^^^^
> diff --git a/include/net/sock.h b/include/net/sock.h
> index cce23ac4d514..b8535692f340 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -2600,12 +2600,13 @@ static inline void sock_write_timestamp(struct sock *sk, ktime_t kt)
>  }
>
>  void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
> -                          struct sk_buff *skb);
> +                          struct sk_buff *skb, bool errqueue);
>  void __sock_recv_wifi_status(struct msghdr *msg, struct sock *sk,
>                              struct sk_buff *skb);
>
>  static inline void
> -sock_recv_timestamp(struct msghdr *msg, struct sock *sk, struct sk_buff *skb)
> +sock_recv_timestamp(struct msghdr *msg, struct sock *sk, struct sk_buff *skb,
> +                   bool errqueue)
>  {
>         struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
>         u32 tsflags = READ_ONCE(sk->sk_tsflags);
> @@ -2621,7 +2622,7 @@ sock_recv_timestamp(struct msghdr *msg, struct sock *sk, struct sk_buff *skb)
>             (kt && tsflags & SOF_TIMESTAMPING_SOFTWARE) ||
>             (hwtstamps->hwtstamp &&
>              (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)))
> -               __sock_recv_timestamp(msg, sk, skb);
> +               __sock_recv_timestamp(msg, sk, skb, errqueue);
>         else
>                 sock_write_timestamp(sk, kt);
>
> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> index 69c2ba1e843e..c1b73c5a370b 100644
> --- a/net/bluetooth/hci_sock.c
> +++ b/net/bluetooth/hci_sock.c
> @@ -1586,11 +1586,11 @@ static int hci_sock_recvmsg(struct socket *sock, struct msghdr *msg,
>                 break;
>         case HCI_CHANNEL_USER:
>         case HCI_CHANNEL_MONITOR:
> -               sock_recv_timestamp(msg, sk, skb);
> +               sock_recv_timestamp(msg, sk, skb, false);
>                 break;
>         default:
>                 if (hci_mgmt_chan_find(hci_pi(sk)->channel))
> -                       sock_recv_timestamp(msg, sk, skb);
> +                       sock_recv_timestamp(msg, sk, skb, false);
>                 break;
>         }
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 9abc4fe25953..d969a4901300 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -3677,7 +3677,7 @@ int sock_recv_errqueue(struct sock *sk, struct msghdr *msg, int len,
>         if (err)
>                 goto out_free_skb;
>
> -       sock_recv_timestamp(msg, sk, skb);
> +       sock_recv_timestamp(msg, sk, skb, true);
>
>         serr = SKB_EXT_ERR(skb);
>         put_cmsg(msg, level, type, sizeof(serr->ee), &serr->ee);
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index cf377377b52d..b79f859c34bf 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -547,7 +547,7 @@ int ip_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len)
>                 kfree_skb(skb);
>                 return err;
>         }
> -       sock_recv_timestamp(msg, sk, skb);
> +       sock_recv_timestamp(msg, sk, skb, true);
>
>         serr = SKB_EXT_ERR(skb);
>
> diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
> index 619ddc087957..1cf7b0eecd63 100644
> --- a/net/ipv4/ping.c
> +++ b/net/ipv4/ping.c
> @@ -880,7 +880,7 @@ int ping_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int flags,
>         if (err)
>                 goto done;
>
> -       sock_recv_timestamp(msg, sk, skb);
> +       sock_recv_timestamp(msg, sk, skb, false);
>
>         /* Copy the address and add cmsg data. */
>         if (family == AF_INET) {
> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
> index fff78496803d..1e4c11b2d0ce 100644
> --- a/net/ipv6/datagram.c
> +++ b/net/ipv6/datagram.c
> @@ -479,7 +479,7 @@ int ipv6_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len)
>                 kfree_skb(skb);
>                 return err;
>         }
> -       sock_recv_timestamp(msg, sk, skb);
> +       sock_recv_timestamp(msg, sk, skb, true);
>
>         serr = SKB_EXT_ERR(skb);
>
> @@ -568,7 +568,7 @@ int ipv6_recv_rxpmtu(struct sock *sk, struct msghdr *msg, int len,
>         if (err)
>                 goto out_free_skb;
>
> -       sock_recv_timestamp(msg, sk, skb);
> +       sock_recv_timestamp(msg, sk, skb, false);
>
>         memcpy(&mtu_info, IP6CBMTU(skb), sizeof(mtu_info));
>
> diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
> index 4bc24fddfd52..164c8ed7124e 100644
> --- a/net/l2tp/l2tp_ip.c
> +++ b/net/l2tp/l2tp_ip.c
> @@ -567,7 +567,7 @@ static int l2tp_ip_recvmsg(struct sock *sk, struct msghdr *msg,
>         if (err)
>                 goto done;
>
> -       sock_recv_timestamp(msg, sk, skb);
> +       sock_recv_timestamp(msg, sk, skb, false);
>
>         /* Copy the address. */
>         if (sin) {
> diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
> index f4c1da070826..b0bb0a1f772e 100644
> --- a/net/l2tp/l2tp_ip6.c
> +++ b/net/l2tp/l2tp_ip6.c
> @@ -712,7 +712,7 @@ static int l2tp_ip6_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>         if (err)
>                 goto done;
>
> -       sock_recv_timestamp(msg, sk, skb);
> +       sock_recv_timestamp(msg, sk, skb, false);
>
>         /* Copy the address. */
>         if (lsa) {
> diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
> index 57a2f97004e1..5c6e671643f6 100644
> --- a/net/nfc/llcp_sock.c
> +++ b/net/nfc/llcp_sock.c
> @@ -869,7 +869,7 @@ static int llcp_sock_recvmsg(struct socket *sock, struct msghdr *msg,
>                 return -EFAULT;
>         }
>
> -       sock_recv_timestamp(msg, sk, skb);
> +       sock_recv_timestamp(msg, sk, skb, false);
>
>         if (sk->sk_type == SOCK_DGRAM && msg->msg_name) {
>                 struct nfc_llcp_ui_cb *ui_cb = nfc_llcp_ui_skb_cb(skb);
> diff --git a/net/rxrpc/recvmsg.c b/net/rxrpc/recvmsg.c
> index a482f88c5fc5..18fa392011fb 100644
> --- a/net/rxrpc/recvmsg.c
> +++ b/net/rxrpc/recvmsg.c
> @@ -200,7 +200,7 @@ static int rxrpc_recvmsg_data(struct socket *sock, struct rxrpc_call *call,
>                                             sp->hdr.serial, seq);
>
>                 if (msg)
> -                       sock_recv_timestamp(msg, sock->sk, skb);
> +                       sock_recv_timestamp(msg, sock->sk, skb, false);
>
>                 if (rx_pkt_offset == 0) {
>                         ret2 = rxrpc_verify_data(call, skb);
> diff --git a/net/socket.c b/net/socket.c
> index fcbdd5bc47ac..c02fb9b615b2 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -893,7 +893,7 @@ static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb,
>   * called from sock_recv_timestamp() if sock_flag(sk, SOCK_RCVTSTAMP)
>   */
>  void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
> -       struct sk_buff *skb)
> +                          struct sk_buff *skb, bool errqueue)
>  {
>         int need_software_tstamp = sock_flag(sk, SOCK_RCVTSTAMP);
>         int new_tstamp = sock_flag(sk, SOCK_TSTAMP_NEW);
> @@ -946,7 +946,12 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
>
>         memset(&tss, 0, sizeof(tss));
>         tsflags = READ_ONCE(sk->sk_tsflags);
> -       if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
> +       /* We have to use the generation flag here to test if we allow the
> +        * corresponding application to receive the rx timestamp. Only
> +        * using report flag does not hold for receive timestamping case.
> +        */
> +       if ((tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> +            (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE || errqueue)) &&

Hello Willem,

After considering this part implemented in sock_recv_timestamp() in
the previous version over and over again, I think I need to add back
what I removed in sock_recv_timestamp(), because:
supposing we only set SOF_TIMESTAMPING_SOFTWARE, we will go into
__sock_recv_timestamp and do nothing but return, then we will miss
setting sk->sk_stamp in sock_write_timestamp().
In that case, the socket will miss two chances to set sk_stamp.

sk_stamp stands for the timestamp of the last packet we receive, it is
necessary to set sk_stamp in sock_recv_timestamp() one way or another.

I wonder if I understand correctly?

Please also help me review the remaining code, thanks.

Thanks,
Jason


>             ktime_to_timespec64_cond(skb->tstamp, tss.ts + 0))
>                 empty = 0;
>         if (shhwtstamps &&
> @@ -1024,7 +1029,7 @@ static void sock_recv_mark(struct msghdr *msg, struct sock *sk,
>  void __sock_recv_cmsgs(struct msghdr *msg, struct sock *sk,
>                        struct sk_buff *skb)
>  {
> -       sock_recv_timestamp(msg, sk, skb);
> +       sock_recv_timestamp(msg, sk, skb, false);
>         sock_recv_drops(msg, sk, skb);
>         sock_recv_mark(msg, sk, skb);
>  }
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index a1894019ebd5..bb33f2994618 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -2481,7 +2481,7 @@ int __unix_dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t size,
>                 goto out_free;
>
>         if (sock_flag(sk, SOCK_RCVTSTAMP))
> -               __sock_recv_timestamp(msg, sk, skb);
> +               __sock_recv_timestamp(msg, sk, skb, false);
>
>         memset(&scm, 0, sizeof(scm));
>
> --
> 2.37.3
>

Re: [PATCH net-next v2 2/2] net: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[Willem de Bruijn]

Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> Like the previous patch in this series, we need to make sure that
> we both set SOF_TIMESTAMPING_SOFTWARE and SOF_TIMESTAMPING_RX_SOFTWARE
> flags together so that we can let the user parse the rx timestamp.
> 
> One more important and special thing is that we should take care of
> errqueue recv path because we rely on errqueue to get our timestamps
> for sendmsg(). Or else, If the user wants to read when setting
> SOF_TIMESTAMPING_TX_ACK, something like this, we cannot get timestamps,
> for example, in TCP case. So we should consider those
> SOF_TIMESTAMPING_TX_* flags.
> 
> After this patch, we are able to pass the testcase 6 for IP and UDP
> cases when running ./rxtimestamp binary.
> 
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  Documentation/networking/timestamping.rst |  7 +++++++
>  include/net/sock.h                        |  7 ++++---
>  net/bluetooth/hci_sock.c                  |  4 ++--
>  net/core/sock.c                           |  2 +-
>  net/ipv4/ip_sockglue.c                    |  2 +-
>  net/ipv4/ping.c                           |  2 +-
>  net/ipv6/datagram.c                       |  4 ++--
>  net/l2tp/l2tp_ip.c                        |  2 +-
>  net/l2tp/l2tp_ip6.c                       |  2 +-
>  net/nfc/llcp_sock.c                       |  2 +-
>  net/rxrpc/recvmsg.c                       |  2 +-
>  net/socket.c                              | 11 ++++++++---
>  net/unix/af_unix.c                        |  2 +-
>  13 files changed, 31 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> index 5e93cd71f99f..93378b78c6dd 100644
> --- a/Documentation/networking/timestamping.rst
> +++ b/Documentation/networking/timestamping.rst
> @@ -160,6 +160,13 @@ SOF_TIMESTAMPING_RAW_HARDWARE:
>    Report hardware timestamps as generated by
>    SOF_TIMESTAMPING_TX_HARDWARE when available.
>  
> +Please note: previously, if an application starts first which turns on
> +netstamp_needed_key, then another one only passing SOF_TIMESTAMPING_SOFTWARE
> +could also get rx timestamp. Now we handle this case and will not get
> +rx timestamp under this circumstance. We encourage that for each socket
> +we should use the SOF_TIMESTAMPING_RX_SOFTWARE generation flag to time
> +stamp the skb and use SOF_TIMESTAMPING_SOFTWARE report flag to tell
> +the application.

Don't mention previously. Readers will not be aware of when this
Documentation was added.

Also, nit: no "Please note". Else every paragraph can start with that,
as each statement should be noteworthy.

>  
>  1.3.3 Timestamp Options
>  ^^^^^^^^^^^^^^^^^^^^^^^
> diff --git a/include/net/sock.h b/include/net/sock.h
> index cce23ac4d514..b8535692f340 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -2600,12 +2600,13 @@ static inline void sock_write_timestamp(struct sock *sk, ktime_t kt)
>  }
>  
>  void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
> -			   struct sk_buff *skb);
> +			   struct sk_buff *skb, bool errqueue);
>  void __sock_recv_wifi_status(struct msghdr *msg, struct sock *sk,
>  			     struct sk_buff *skb);

I suspect that the direction, ingress or egress, and thus whether the
timestamp is to be queued on the error queue or not, can be inferred
without exceptions from skb->pkt_type == PACKET_OUTGOING.

That would avoid all this boilerplate argument passing.

Re: [PATCH net-next v2 2/2] net: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[Jason Xing]

On Thu, Aug 29, 2024 at 10:21 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Like the previous patch in this series, we need to make sure that
> > we both set SOF_TIMESTAMPING_SOFTWARE and SOF_TIMESTAMPING_RX_SOFTWARE
> > flags together so that we can let the user parse the rx timestamp.
> >
> > One more important and special thing is that we should take care of
> > errqueue recv path because we rely on errqueue to get our timestamps
> > for sendmsg(). Or else, If the user wants to read when setting
> > SOF_TIMESTAMPING_TX_ACK, something like this, we cannot get timestamps,
> > for example, in TCP case. So we should consider those
> > SOF_TIMESTAMPING_TX_* flags.
> >
> > After this patch, we are able to pass the testcase 6 for IP and UDP
> > cases when running ./rxtimestamp binary.
> >
> > Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> >  Documentation/networking/timestamping.rst |  7 +++++++
> >  include/net/sock.h                        |  7 ++++---
> >  net/bluetooth/hci_sock.c                  |  4 ++--
> >  net/core/sock.c                           |  2 +-
> >  net/ipv4/ip_sockglue.c                    |  2 +-
> >  net/ipv4/ping.c                           |  2 +-
> >  net/ipv6/datagram.c                       |  4 ++--
> >  net/l2tp/l2tp_ip.c                        |  2 +-
> >  net/l2tp/l2tp_ip6.c                       |  2 +-
> >  net/nfc/llcp_sock.c                       |  2 +-
> >  net/rxrpc/recvmsg.c                       |  2 +-
> >  net/socket.c                              | 11 ++++++++---
> >  net/unix/af_unix.c                        |  2 +-
> >  13 files changed, 31 insertions(+), 18 deletions(-)
> >
> > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> > index 5e93cd71f99f..93378b78c6dd 100644
> > --- a/Documentation/networking/timestamping.rst
> > +++ b/Documentation/networking/timestamping.rst
> > @@ -160,6 +160,13 @@ SOF_TIMESTAMPING_RAW_HARDWARE:
> >    Report hardware timestamps as generated by
> >    SOF_TIMESTAMPING_TX_HARDWARE when available.
> >
> > +Please note: previously, if an application starts first which turns on
> > +netstamp_needed_key, then another one only passing SOF_TIMESTAMPING_SOFTWARE
> > +could also get rx timestamp. Now we handle this case and will not get
> > +rx timestamp under this circumstance. We encourage that for each socket
> > +we should use the SOF_TIMESTAMPING_RX_SOFTWARE generation flag to time
> > +stamp the skb and use SOF_TIMESTAMPING_SOFTWARE report flag to tell
> > +the application.
>
> Don't mention previously. Readers will not be aware of when this
> Documentation was added.

Got it, I will remove it :)

>
> Also, nit: no "Please note". Else every paragraph can start with that,
> as each statement should be noteworthy.

I see.

>
> >
> >  1.3.3 Timestamp Options
> >  ^^^^^^^^^^^^^^^^^^^^^^^
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index cce23ac4d514..b8535692f340 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -2600,12 +2600,13 @@ static inline void sock_write_timestamp(struct sock *sk, ktime_t kt)
> >  }
> >
> >  void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
> > -                        struct sk_buff *skb);
> > +                        struct sk_buff *skb, bool errqueue);
> >  void __sock_recv_wifi_status(struct msghdr *msg, struct sock *sk,
> >                            struct sk_buff *skb);
>
> I suspect that the direction, ingress or egress, and thus whether the
> timestamp is to be queued on the error queue or not, can be inferred
> without exceptions from skb->pkt_type == PACKET_OUTGOING.
>
> That would avoid all this boilerplate argument passing.

Ah, good suggestions!! I will update it in the V3.

Thanks,
Jason

Re: [PATCH net-next v2 2/2] net: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[Jakub Kicinski]

On Thu, 29 Aug 2024 00:01:45 +0800 Jason Xing wrote:
> One more important and special thing is that we should take care of
> errqueue recv path because we rely on errqueue to get our timestamps
> for sendmsg(). Or else, If the user wants to read when setting
> SOF_TIMESTAMPING_TX_ACK, something like this, we cannot get timestamps,
> for example, in TCP case. So we should consider those
> SOF_TIMESTAMPING_TX_* flags.

I read this 3 times, I don't know what you're trying to say.

Re: [PATCH net-next v2 2/2] net: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[Jason Xing]

Hello Jakub,

On Fri, Aug 30, 2024 at 3:29 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 29 Aug 2024 00:01:45 +0800 Jason Xing wrote:
> > One more important and special thing is that we should take care of
> > errqueue recv path because we rely on errqueue to get our timestamps
> > for sendmsg(). Or else, If the user wants to read when setting
> > SOF_TIMESTAMPING_TX_ACK, something like this, we cannot get timestamps,
> > for example, in TCP case. So we should consider those
> > SOF_TIMESTAMPING_TX_* flags.
>
> I read this 3 times, I don't know what you're trying to say.

Sorry about that. It looks like I really need more time to improve my
written English...

I was trying to say:
In this case, we expect to control whether we can report the rx
timestamp in this function. But we also need to handle the egress
path, or else reporting the tx timestamp will fail. Egress path and
ingress path will finally call sock_recv_timestamp(). We have to
distinguish them. Errqueue is a good indicator to reflect the flow
direction.

Never mind. I'm going to replace the series with a safer alternative
solution, which was suggested by Willem a few hours ago.

Thanks,
Jason

Re: [PATCH net-next v2 2/2] net: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[Jakub Kicinski]

On Fri, 30 Aug 2024 08:40:47 +0800 Jason Xing wrote:
> In this case, we expect to control whether we can report the rx
> timestamp in this function. But we also need to handle the egress
> path, or else reporting the tx timestamp will fail. Egress path and
> ingress path will finally call sock_recv_timestamp(). We have to
> distinguish them. Errqueue is a good indicator to reflect the flow
> direction.

That is better, FWIW, thanks

Re: [PATCH net-next v2 0/2] timestamp: control SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[Willem de Bruijn]

Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> Prior to this series, when one socket is set SOF_TIMESTAMPING_RX_SOFTWARE
> which measn the whole system turns on this button, other sockets that only
> have SOF_TIMESTAMPING_SOFTWARE will be affected and then print the rx
> timestamp information even without SOF_TIMESTAMPING_RX_SOFTWARE flag.
> In such a case, the rxtimestamp.c selftest surely fails, please see
> testcase 6.
> 
> In a normal case, if we only set SOF_TIMESTAMPING_SOFTWARE flag, we
> can't get the rx timestamp because there is no path leading to turn on
> netstamp_needed_key button in net_enable_timestamp(). That is to say, if
> the user only sets SOF_TIMESTAMPING_SOFTWARE, we don't expect we are
> able to fetch the timestamp from the skb.

I already happened to stumble upon a counterexample.

The below code requests software timestamps, but does not set the
generate flag. I suspect because they assume a PTP daemon (sfptpd)
running that has already enabled that.

https://github.com/Xilinx-CNS/onload/blob/master/src/tests/onload/hwtimestamping/rx_timestamping.c

I suspect that there will be more of such examples in practice. In
which case we should scuttle this. Please do a search online for
SOF_TIMESTAMPING_SOFTWARE to scan for this pattern.
 
> More than this, we can find there are some other ways to turn on
> netstamp_needed_key, which will happenly allow users to get tstamp in
> the receive path. Please see net_enable_timestamp().
> 
> How to solve it?
> 
> setsockopt interface is used to control each socket separately but in
> this case it is affected by other sockets. For timestamp itself, it's
> not feasible to convert netstamp_needed_key into a per-socket button
> because when the receive stack just handling the skb from driver doesn't
> know which socket the skb belongs to.
> 
> According to the original design, we should not use both generation flag
> (SOF_TIMESTAMPING_RX_SOFTWARE) and report flag (SOF_TIMESTAMPING_SOFTWARE)
> together to test if the application is allowed to receive the timestamp
> report in the receive path. But it doesn't hold for receive timestamping
> case. We have to make an exception.
> 
> So we have to test the generation flag when the applications do recvmsg:
> if we set both of flags, it means we want the timestamp; if not, it means
> we don't expect to see the timestamp even the skb carries.
> 
> As we can see, this patch makes the SOF_TIMESTAMPING_RX_SOFTWARE under
> setsockopt control. And it's a per-socket fine-grained now.
> 
> v2
> Link: https://lore.kernel.org/all/20240825152440.93054-1-kerneljasonxing@gmail.com/
> Discussed with Willem
> 1. update the documentation accordingly
> 2. add more comments in each patch
> 3. remove the previous test statements in __sock_recv_timestamp()
> 
> Jason Xing (2):
>   tcp: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket
>   net: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket
> 
>  Documentation/networking/timestamping.rst |  7 +++++++
>  include/net/sock.h                        |  7 ++++---
>  net/bluetooth/hci_sock.c                  |  4 ++--
>  net/core/sock.c                           |  2 +-
>  net/ipv4/ip_sockglue.c                    |  2 +-
>  net/ipv4/ping.c                           |  2 +-
>  net/ipv4/tcp.c                            | 11 +++++++++--
>  net/ipv6/datagram.c                       |  4 ++--
>  net/l2tp/l2tp_ip.c                        |  2 +-
>  net/l2tp/l2tp_ip6.c                       |  2 +-
>  net/nfc/llcp_sock.c                       |  2 +-
>  net/rxrpc/recvmsg.c                       |  2 +-
>  net/socket.c                              | 11 ++++++++---
>  net/unix/af_unix.c                        |  2 +-
>  14 files changed, 40 insertions(+), 20 deletions(-)
> 
> -- 
> 2.37.3
> 

Re: [PATCH net-next v2 0/2] timestamp: control SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[Jason Xing]

On Thu, Aug 29, 2024 at 10:14 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Prior to this series, when one socket is set SOF_TIMESTAMPING_RX_SOFTWARE
> > which measn the whole system turns on this button, other sockets that only
> > have SOF_TIMESTAMPING_SOFTWARE will be affected and then print the rx
> > timestamp information even without SOF_TIMESTAMPING_RX_SOFTWARE flag.
> > In such a case, the rxtimestamp.c selftest surely fails, please see
> > testcase 6.
> >
> > In a normal case, if we only set SOF_TIMESTAMPING_SOFTWARE flag, we
> > can't get the rx timestamp because there is no path leading to turn on
> > netstamp_needed_key button in net_enable_timestamp(). That is to say, if
> > the user only sets SOF_TIMESTAMPING_SOFTWARE, we don't expect we are
> > able to fetch the timestamp from the skb.
>
> I already happened to stumble upon a counterexample.
>
> The below code requests software timestamps, but does not set the
> generate flag. I suspect because they assume a PTP daemon (sfptpd)
> running that has already enabled that.

To be honest, I took a quick search through the whole onload program
and then suspected the use of timestamp looks really weird.

1. I searched the SOF_TIMESTAMPING_RX_SOFTWARE flag and found there is
no other related place that actually uses it.
2. please also see the tx_timestamping.c file[1]. The author similarly
only turns on SOF_TIMESTAMPING_SOFTWARE report flag without turning on
any useful generation flag we are familiar with, like
SOF_TIMESTAMPING_TX_SOFTWARE, SOF_TIMESTAMPING_TX_SCHED,
SOF_TIMESTAMPING_TX_ACK.

[1]: https://github.com/Xilinx-CNS/onload/blob/master/src/tests/onload/hwtimestamping/tx_timestamping.c#L247

>
> https://github.com/Xilinx-CNS/onload/blob/master/src/tests/onload/hwtimestamping/rx_timestamping.c
>
> I suspect that there will be more of such examples in practice. In
> which case we should scuttle this. Please do a search online for
> SOF_TIMESTAMPING_SOFTWARE to scan for this pattern.

I feel that only the buggy program or some program particularly takes
advantage of the global netstamp_needed_key...

>
> > More than this, we can find there are some other ways to turn on
> > netstamp_needed_key, which will happenly allow users to get tstamp in
> > the receive path. Please see net_enable_timestamp().
> >
> > How to solve it?
> >
> > setsockopt interface is used to control each socket separately but in
> > this case it is affected by other sockets. For timestamp itself, it's
> > not feasible to convert netstamp_needed_key into a per-socket button
> > because when the receive stack just handling the skb from driver doesn't
> > know which socket the skb belongs to.
> >
> > According to the original design, we should not use both generation flag
> > (SOF_TIMESTAMPING_RX_SOFTWARE) and report flag (SOF_TIMESTAMPING_SOFTWARE)
> > together to test if the application is allowed to receive the timestamp
> > report in the receive path. But it doesn't hold for receive timestamping
> > case. We have to make an exception.
> >
> > So we have to test the generation flag when the applications do recvmsg:
> > if we set both of flags, it means we want the timestamp; if not, it means
> > we don't expect to see the timestamp even the skb carries.
> >
> > As we can see, this patch makes the SOF_TIMESTAMPING_RX_SOFTWARE under
> > setsockopt control. And it's a per-socket fine-grained now.
> >
> > v2
> > Link: https://lore.kernel.org/all/20240825152440.93054-1-kerneljasonxing@gmail.com/
> > Discussed with Willem
> > 1. update the documentation accordingly
> > 2. add more comments in each patch
> > 3. remove the previous test statements in __sock_recv_timestamp()
> >
> > Jason Xing (2):
> >   tcp: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket
> >   net: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket
> >
> >  Documentation/networking/timestamping.rst |  7 +++++++
> >  include/net/sock.h                        |  7 ++++---
> >  net/bluetooth/hci_sock.c                  |  4 ++--
> >  net/core/sock.c                           |  2 +-
> >  net/ipv4/ip_sockglue.c                    |  2 +-
> >  net/ipv4/ping.c                           |  2 +-
> >  net/ipv4/tcp.c                            | 11 +++++++++--
> >  net/ipv6/datagram.c                       |  4 ++--
> >  net/l2tp/l2tp_ip.c                        |  2 +-
> >  net/l2tp/l2tp_ip6.c                       |  2 +-
> >  net/nfc/llcp_sock.c                       |  2 +-
> >  net/rxrpc/recvmsg.c                       |  2 +-
> >  net/socket.c                              | 11 ++++++++---
> >  net/unix/af_unix.c                        |  2 +-
> >  14 files changed, 40 insertions(+), 20 deletions(-)
> >
> > --
> > 2.37.3
> >
>
>

Re: [PATCH net-next v2 0/2] timestamp: control SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[Willem de Bruijn]

Jason Xing wrote:
> On Thu, Aug 29, 2024 at 10:14 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jason Xing wrote:
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > Prior to this series, when one socket is set SOF_TIMESTAMPING_RX_SOFTWARE
> > > which measn the whole system turns on this button, other sockets that only
> > > have SOF_TIMESTAMPING_SOFTWARE will be affected and then print the rx
> > > timestamp information even without SOF_TIMESTAMPING_RX_SOFTWARE flag.
> > > In such a case, the rxtimestamp.c selftest surely fails, please see
> > > testcase 6.
> > >
> > > In a normal case, if we only set SOF_TIMESTAMPING_SOFTWARE flag, we
> > > can't get the rx timestamp because there is no path leading to turn on
> > > netstamp_needed_key button in net_enable_timestamp(). That is to say, if
> > > the user only sets SOF_TIMESTAMPING_SOFTWARE, we don't expect we are
> > > able to fetch the timestamp from the skb.
> >
> > I already happened to stumble upon a counterexample.
> >
> > The below code requests software timestamps, but does not set the
> > generate flag. I suspect because they assume a PTP daemon (sfptpd)
> > running that has already enabled that.
> 
> To be honest, I took a quick search through the whole onload program
> and then suspected the use of timestamp looks really weird.
> 
> 1. I searched the SOF_TIMESTAMPING_RX_SOFTWARE flag and found there is
> no other related place that actually uses it.
> 2. please also see the tx_timestamping.c file[1]. The author similarly
> only turns on SOF_TIMESTAMPING_SOFTWARE report flag without turning on
> any useful generation flag we are familiar with, like
> SOF_TIMESTAMPING_TX_SOFTWARE, SOF_TIMESTAMPING_TX_SCHED,
> SOF_TIMESTAMPING_TX_ACK.
> 
> [1]: https://github.com/Xilinx-CNS/onload/blob/master/src/tests/onload/hwtimestamping/tx_timestamping.c#L247
> 
> >
> > https://github.com/Xilinx-CNS/onload/blob/master/src/tests/onload/hwtimestamping/rx_timestamping.c
> >
> > I suspect that there will be more of such examples in practice. In
> > which case we should scuttle this. Please do a search online for
> > SOF_TIMESTAMPING_SOFTWARE to scan for this pattern.
> 
> I feel that only the buggy program or some program particularly takes
> advantage of the global netstamp_needed_key...

My point is that I just happen to stumble on one open source example
of this behavior.

That is a strong indication that other applications may make the same
implicit assumption. Both open source, and the probably many more non
public users.

Rule #1 is to not break users.

Given that we even have proof that we would break users, we cannot
make this change, sorry.

A safer alternative is to define a new timestamp option flag that
opt-in enables this filter-if-SOF_TIMESTAMPING_RX_SOFTWARE is not
set behavior.

Re: [PATCH net-next v2 0/2] timestamp: control SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[Jason Xing]

On Fri, Aug 30, 2024 at 12:23 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > On Thu, Aug 29, 2024 at 10:14 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Jason Xing wrote:
> > > > From: Jason Xing <kernelxing@tencent.com>
> > > >
> > > > Prior to this series, when one socket is set SOF_TIMESTAMPING_RX_SOFTWARE
> > > > which measn the whole system turns on this button, other sockets that only
> > > > have SOF_TIMESTAMPING_SOFTWARE will be affected and then print the rx
> > > > timestamp information even without SOF_TIMESTAMPING_RX_SOFTWARE flag.
> > > > In such a case, the rxtimestamp.c selftest surely fails, please see
> > > > testcase 6.
> > > >
> > > > In a normal case, if we only set SOF_TIMESTAMPING_SOFTWARE flag, we
> > > > can't get the rx timestamp because there is no path leading to turn on
> > > > netstamp_needed_key button in net_enable_timestamp(). That is to say, if
> > > > the user only sets SOF_TIMESTAMPING_SOFTWARE, we don't expect we are
> > > > able to fetch the timestamp from the skb.
> > >
> > > I already happened to stumble upon a counterexample.
> > >
> > > The below code requests software timestamps, but does not set the
> > > generate flag. I suspect because they assume a PTP daemon (sfptpd)
> > > running that has already enabled that.
> >
> > To be honest, I took a quick search through the whole onload program
> > and then suspected the use of timestamp looks really weird.
> >
> > 1. I searched the SOF_TIMESTAMPING_RX_SOFTWARE flag and found there is
> > no other related place that actually uses it.
> > 2. please also see the tx_timestamping.c file[1]. The author similarly
> > only turns on SOF_TIMESTAMPING_SOFTWARE report flag without turning on
> > any useful generation flag we are familiar with, like
> > SOF_TIMESTAMPING_TX_SOFTWARE, SOF_TIMESTAMPING_TX_SCHED,
> > SOF_TIMESTAMPING_TX_ACK.
> >
> > [1]: https://github.com/Xilinx-CNS/onload/blob/master/src/tests/onload/hwtimestamping/tx_timestamping.c#L247
> >
> > >
> > > https://github.com/Xilinx-CNS/onload/blob/master/src/tests/onload/hwtimestamping/rx_timestamping.c
> > >
> > > I suspect that there will be more of such examples in practice. In
> > > which case we should scuttle this. Please do a search online for
> > > SOF_TIMESTAMPING_SOFTWARE to scan for this pattern.
> >
> > I feel that only the buggy program or some program particularly takes
> > advantage of the global netstamp_needed_key...
>
> My point is that I just happen to stumble on one open source example
> of this behavior.
>
> That is a strong indication that other applications may make the same
> implicit assumption. Both open source, and the probably many more non
> public users.
>
> Rule #1 is to not break users.

Yes, I know it.

>
> Given that we even have proof that we would break users, we cannot
> make this change, sorry.

Okay. Your concern indeed makes sense. Sigh, I just finished the v3
patch series :S

>
> A safer alternative is to define a new timestamp option flag that
> opt-in enables this filter-if-SOF_TIMESTAMPING_RX_SOFTWARE is not
> set behavior.

At the first glance, It sounds like it's a little bit similar to
SOF_TIMESTAMPING_OPT_ID_TCP that is used to replace
SOF_TIMESTAMPING_OPT_ID in the bytestream case for robustness
consideration.

Are you suggesting that if we can use the new report flag combined
with SOF_TIMESTAMPING_SOFTWARE, the application will not get a rx
timestamp report, right? The new flag goes the opposite way compared
with SOF_TIMESTAMPING_RX_SOFTWARE, indicating we don't expect a rx sw
report.

If that is so, what would you recommend to name the new flag which is
a report flag (not a generation flag)? How about calling
"SOF_TIMESTAMPING_RX_SOFTWARE_CTRL". I tried, but my English
vocabulary doesn't help, sorry :(

Thanks,
Jason

Re: [PATCH net-next v2 0/2] timestamp: control SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[Willem de Bruijn]

Jason Xing wrote:
> On Fri, Aug 30, 2024 at 12:23 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jason Xing wrote:
> > > On Thu, Aug 29, 2024 at 10:14 PM Willem de Bruijn
> > > <willemdebruijn.kernel@gmail.com> wrote:
> > > >
> > > > Jason Xing wrote:
> > > > > From: Jason Xing <kernelxing@tencent.com>
> > > > >
> > > > > Prior to this series, when one socket is set SOF_TIMESTAMPING_RX_SOFTWARE
> > > > > which measn the whole system turns on this button, other sockets that only
> > > > > have SOF_TIMESTAMPING_SOFTWARE will be affected and then print the rx
> > > > > timestamp information even without SOF_TIMESTAMPING_RX_SOFTWARE flag.
> > > > > In such a case, the rxtimestamp.c selftest surely fails, please see
> > > > > testcase 6.
> > > > >
> > > > > In a normal case, if we only set SOF_TIMESTAMPING_SOFTWARE flag, we
> > > > > can't get the rx timestamp because there is no path leading to turn on
> > > > > netstamp_needed_key button in net_enable_timestamp(). That is to say, if
> > > > > the user only sets SOF_TIMESTAMPING_SOFTWARE, we don't expect we are
> > > > > able to fetch the timestamp from the skb.
> > > >
> > > > I already happened to stumble upon a counterexample.
> > > >
> > > > The below code requests software timestamps, but does not set the
> > > > generate flag. I suspect because they assume a PTP daemon (sfptpd)
> > > > running that has already enabled that.
> > >
> > > To be honest, I took a quick search through the whole onload program
> > > and then suspected the use of timestamp looks really weird.
> > >
> > > 1. I searched the SOF_TIMESTAMPING_RX_SOFTWARE flag and found there is
> > > no other related place that actually uses it.
> > > 2. please also see the tx_timestamping.c file[1]. The author similarly
> > > only turns on SOF_TIMESTAMPING_SOFTWARE report flag without turning on
> > > any useful generation flag we are familiar with, like
> > > SOF_TIMESTAMPING_TX_SOFTWARE, SOF_TIMESTAMPING_TX_SCHED,
> > > SOF_TIMESTAMPING_TX_ACK.
> > >
> > > [1]: https://github.com/Xilinx-CNS/onload/blob/master/src/tests/onload/hwtimestamping/tx_timestamping.c#L247
> > >
> > > >
> > > > https://github.com/Xilinx-CNS/onload/blob/master/src/tests/onload/hwtimestamping/rx_timestamping.c
> > > >
> > > > I suspect that there will be more of such examples in practice. In
> > > > which case we should scuttle this. Please do a search online for
> > > > SOF_TIMESTAMPING_SOFTWARE to scan for this pattern.
> > >
> > > I feel that only the buggy program or some program particularly takes
> > > advantage of the global netstamp_needed_key...
> >
> > My point is that I just happen to stumble on one open source example
> > of this behavior.
> >
> > That is a strong indication that other applications may make the same
> > implicit assumption. Both open source, and the probably many more non
> > public users.
> >
> > Rule #1 is to not break users.
> 
> Yes, I know it.
> 
> >
> > Given that we even have proof that we would break users, we cannot
> > make this change, sorry.
> 
> Okay. Your concern indeed makes sense. Sigh, I just finished the v3
> patch series :S
> 
> >
> > A safer alternative is to define a new timestamp option flag that
> > opt-in enables this filter-if-SOF_TIMESTAMPING_RX_SOFTWARE is not
> > set behavior.
> 
> At the first glance, It sounds like it's a little bit similar to
> SOF_TIMESTAMPING_OPT_ID_TCP that is used to replace
> SOF_TIMESTAMPING_OPT_ID in the bytestream case for robustness
> consideration.
>
> Are you suggesting that if we can use the new report flag combined
> with SOF_TIMESTAMPING_SOFTWARE, the application will not get a rx
> timestamp report, right? The new flag goes the opposite way compared
> with SOF_TIMESTAMPING_RX_SOFTWARE, indicating we don't expect a rx sw
> report.
> 
> If that is so, what would you recommend to name the new flag which is
> a report flag (not a generation flag)? How about calling
> "SOF_TIMESTAMPING_RX_SOFTWARE_CTRL". I tried, but my English
> vocabulary doesn't help, sorry :(

Something like this?

@@ -947,6 +947,8 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
        memset(&tss, 0, sizeof(tss));
        tsflags = READ_ONCE(sk->sk_tsflags);
        if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
+           (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
+            !tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)

Re: [PATCH net-next v2 0/2] timestamp: control SOF_TIMESTAMPING_RX_SOFTWARE feature per socket

[Jason Xing]

On Fri, Aug 30, 2024 at 2:15 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > On Fri, Aug 30, 2024 at 12:23 AM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Jason Xing wrote:
> > > > On Thu, Aug 29, 2024 at 10:14 PM Willem de Bruijn
> > > > <willemdebruijn.kernel@gmail.com> wrote:
> > > > >
> > > > > Jason Xing wrote:
> > > > > > From: Jason Xing <kernelxing@tencent.com>
> > > > > >
> > > > > > Prior to this series, when one socket is set SOF_TIMESTAMPING_RX_SOFTWARE
> > > > > > which measn the whole system turns on this button, other sockets that only
> > > > > > have SOF_TIMESTAMPING_SOFTWARE will be affected and then print the rx
> > > > > > timestamp information even without SOF_TIMESTAMPING_RX_SOFTWARE flag.
> > > > > > In such a case, the rxtimestamp.c selftest surely fails, please see
> > > > > > testcase 6.
> > > > > >
> > > > > > In a normal case, if we only set SOF_TIMESTAMPING_SOFTWARE flag, we
> > > > > > can't get the rx timestamp because there is no path leading to turn on
> > > > > > netstamp_needed_key button in net_enable_timestamp(). That is to say, if
> > > > > > the user only sets SOF_TIMESTAMPING_SOFTWARE, we don't expect we are
> > > > > > able to fetch the timestamp from the skb.
> > > > >
> > > > > I already happened to stumble upon a counterexample.
> > > > >
> > > > > The below code requests software timestamps, but does not set the
> > > > > generate flag. I suspect because they assume a PTP daemon (sfptpd)
> > > > > running that has already enabled that.
> > > >
> > > > To be honest, I took a quick search through the whole onload program
> > > > and then suspected the use of timestamp looks really weird.
> > > >
> > > > 1. I searched the SOF_TIMESTAMPING_RX_SOFTWARE flag and found there is
> > > > no other related place that actually uses it.
> > > > 2. please also see the tx_timestamping.c file[1]. The author similarly
> > > > only turns on SOF_TIMESTAMPING_SOFTWARE report flag without turning on
> > > > any useful generation flag we are familiar with, like
> > > > SOF_TIMESTAMPING_TX_SOFTWARE, SOF_TIMESTAMPING_TX_SCHED,
> > > > SOF_TIMESTAMPING_TX_ACK.
> > > >
> > > > [1]: https://github.com/Xilinx-CNS/onload/blob/master/src/tests/onload/hwtimestamping/tx_timestamping.c#L247
> > > >
> > > > >
> > > > > https://github.com/Xilinx-CNS/onload/blob/master/src/tests/onload/hwtimestamping/rx_timestamping.c
> > > > >
> > > > > I suspect that there will be more of such examples in practice. In
> > > > > which case we should scuttle this. Please do a search online for
> > > > > SOF_TIMESTAMPING_SOFTWARE to scan for this pattern.
> > > >
> > > > I feel that only the buggy program or some program particularly takes
> > > > advantage of the global netstamp_needed_key...
> > >
> > > My point is that I just happen to stumble on one open source example
> > > of this behavior.
> > >
> > > That is a strong indication that other applications may make the same
> > > implicit assumption. Both open source, and the probably many more non
> > > public users.
> > >
> > > Rule #1 is to not break users.
> >
> > Yes, I know it.
> >
> > >
> > > Given that we even have proof that we would break users, we cannot
> > > make this change, sorry.
> >
> > Okay. Your concern indeed makes sense. Sigh, I just finished the v3
> > patch series :S
> >
> > >
> > > A safer alternative is to define a new timestamp option flag that
> > > opt-in enables this filter-if-SOF_TIMESTAMPING_RX_SOFTWARE is not
> > > set behavior.
> >
> > At the first glance, It sounds like it's a little bit similar to
> > SOF_TIMESTAMPING_OPT_ID_TCP that is used to replace
> > SOF_TIMESTAMPING_OPT_ID in the bytestream case for robustness
> > consideration.
> >
> > Are you suggesting that if we can use the new report flag combined
> > with SOF_TIMESTAMPING_SOFTWARE, the application will not get a rx
> > timestamp report, right? The new flag goes the opposite way compared
> > with SOF_TIMESTAMPING_RX_SOFTWARE, indicating we don't expect a rx sw
> > report.
> >
> > If that is so, what would you recommend to name the new flag which is
> > a report flag (not a generation flag)? How about calling
> > "SOF_TIMESTAMPING_RX_SOFTWARE_CTRL". I tried, but my English
> > vocabulary doesn't help, sorry :(
>
> Something like this?
>
> @@ -947,6 +947,8 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
>         memset(&tss, 0, sizeof(tss));
>         tsflags = READ_ONCE(sk->sk_tsflags);
>         if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
> +           (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE ||
> +            !tsflags & SOF_TIMESTAMPING_OPT_RX_SOFTWARE_FILTER)
>

Yes, at least right now I think so. It can work, I can picture it in my mind.

In this way, we will face three possible situations:
1. setting SOF_TIMESTAMPING_SOFTWARE only, it behaves like before.
2. setting SOF_TIMESTAMPING_SOFTWARE|SOF_TIMESTAMPING_RX_SOFTWARE, it
will surely allow users to get the rx timestamp.
3. setting SOF_TIMESTAMPING_SOFTWARE|new_flag while the skb is
timestamped, it will stop reporting the _rx_ timestamp.

Having the new flag can provide a chance for users to stop reporting
the rx timestamp.

Well, It's too late for me (2:00 AM), sorry :( I need to do more tests
and then get back to you tomorrow.

Thanks for your good suggestion, Willem :) It's really a safer and
better suggestion. I have to sleep...

Jason