Re: [PATCH net v1 3/3] tcp: use skb_get_hwtstamp() for hardware timestamps

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Kohei Enju wrote:
> Since commit 97dc7cd92ac6 ("ptp: Support late timestamp determination"),
> skb_shared_hwtstamps may contain netdev_data instead of hwtstamp. TCP
> receive timestamping can then interpret the stored value as a ktime_t
> and report bogus hardware timestamps to userspace.
> 
> Use skb_get_hwtstamp() instead of reading hwtstamp directly, so TCP
> sockets follow the same hardware timestamp resolution path as the socket
> layer. When coalescing SKBs, resolve late timestamps before copying them
> to the merged skb.

Why? Does this preclude supporting SOF_TIMESTAMPING_BIND_PHC for such
sockets at a later time?

It is just as easy to coalesce the cookie as the htwtstamp.

That also avoids the need to mask out SKBTX_HW_TSTAMP_NETDEV.

> Additionally, recognize SKBTX_HW_TSTAMP_NETDEV as
> indicating a receive timestamp is present.

What is the reason for this extra condition?

> Note that skb_get_hwtstamp() is called with cycles == false, since TCP
> hasn't honored SOF_TIMESTAMPING_BIND_PHC so far, and this patch doesn't
> change that behavior.
> 
> Fixes: 97dc7cd92ac6 ("ptp: Support late timestamp determination")
> Signed-off-by: Kohei Enju <kohei@enjuk.jp>
> ---
>  include/net/tcp.h    | 2 +-
>  net/ipv4/tcp_input.c | 3 ++-
>  net/ipv4/tcp_ipv4.c  | 6 ++++--
>  net/ipv6/tcp_ipv6.c  | 3 ++-
>  4 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index ecbadcb3a744..7b5fcee97079 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -524,7 +524,7 @@ tcp_update_recv_tstamps(struct sk_buff *skb,
>  			struct scm_timestamping_internal *tss)
>  {
>  	tss->ts[0] = skb->tstamp;
> -	tss->ts[2] = skb_hwtstamps(skb)->hwtstamp;
> +	tss->ts[2] = skb_get_hwtstamp(skb, false, NULL);
>  }
>  
>  void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index d5c9e65d9760..9fd473559b58 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5237,7 +5237,8 @@ static bool tcp_try_coalesce(struct sock *sk,
>  	if (TCP_SKB_CB(from)->has_rxtstamp) {
>  		TCP_SKB_CB(to)->has_rxtstamp = true;
>  		to->tstamp = from->tstamp;
> -		skb_hwtstamps(to)->hwtstamp = skb_hwtstamps(from)->hwtstamp;
> +		skb_hwtstamps(to)->hwtstamp = skb_get_hwtstamp(from, false, NULL);
> +		skb_shinfo(to)->tx_flags &= ~SKBTX_HW_TSTAMP_NETDEV;
>  	}
>  
>  	return true;
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 8fc24c3743c5..c35d82317764 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1993,7 +1993,8 @@ enum skb_drop_reason tcp_add_backlog(struct sock *sk, struct sk_buff *skb)
>  		if (TCP_SKB_CB(skb)->has_rxtstamp) {
>  			TCP_SKB_CB(tail)->has_rxtstamp = true;
>  			tail->tstamp = skb->tstamp;
> -			skb_hwtstamps(tail)->hwtstamp = skb_hwtstamps(skb)->hwtstamp;
> +			skb_hwtstamps(tail)->hwtstamp = skb_get_hwtstamp(skb, false, NULL);
> +			skb_shinfo(tail)->tx_flags &= ~SKBTX_HW_TSTAMP_NETDEV;
>  		}
>  
>  		/* Not as strict as GRO. We only need to carry mss max value */
> @@ -2062,7 +2063,8 @@ static void tcp_v4_fill_cb(struct sk_buff *skb, const struct iphdr *iph,
>  	TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
>  	TCP_SKB_CB(skb)->sacked	 = 0;
>  	TCP_SKB_CB(skb)->has_rxtstamp =
> -			skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
> +			skb->tstamp || skb_hwtstamps(skb)->hwtstamp ||
> +			(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NETDEV);
>  }
>  
>  /*
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 2c3f7a739709..3361ed7f94de 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1704,7 +1704,8 @@ static void tcp_v6_fill_cb(struct sk_buff *skb, const struct ipv6hdr *hdr,
>  	TCP_SKB_CB(skb)->ip_dsfield = ipv6_get_dsfield(hdr);
>  	TCP_SKB_CB(skb)->sacked = 0;
>  	TCP_SKB_CB(skb)->has_rxtstamp =
> -			skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
> +			skb->tstamp || skb_hwtstamps(skb)->hwtstamp ||
> +			(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NETDEV);
>  }
>  
>  INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
> -- 
> 2.53.0
>