netdev - Re: [PATCH v1 net-next 5/6] net: allow simultaneous SW and HW transmit timestamping

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF=yD-+GSK491AWQx8=6yd3=-HHwxdWq677ubwdjbV5AXzRbog@mail.gmail.com>
Date:   Wed, 26 Apr 2017 20:00:02 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Miroslav Lichvar <mlichvar@...hat.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        Richard Cochran <richardcochran@...il.com>,
        Willem de Bruijn <willemb@...gle.com>,
        Soheil Hassas Yeganeh <soheil@...gle.com>,
        "Keller, Jacob E" <jacob.e.keller@...el.com>,
        Denny Page <dennypage@...com>, Jiri Benc <jbenc@...hat.com>
Subject: Re: [PATCH v1 net-next 5/6] net: allow simultaneous SW and HW
 transmit timestamping

> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 81ef53f..42bff22 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3300,8 +3300,7 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
>
>  static inline void sw_tx_timestamp(struct sk_buff *skb)
>  {
> -       if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP &&
> -           !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
> +       if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP)
>                 skb_tstamp_tx(skb, NULL);
>  }
>
> diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
> index 8fcae35..d251972 100644
> --- a/include/uapi/linux/net_tstamp.h
> +++ b/include/uapi/linux/net_tstamp.h
> @@ -27,8 +27,9 @@ enum {
>         SOF_TIMESTAMPING_OPT_TSONLY = (1<<11),
>         SOF_TIMESTAMPING_OPT_STATS = (1<<12),
>         SOF_TIMESTAMPING_OPT_PKTINFO = (1<<13),
> +       SOF_TIMESTAMPING_OPT_TX_SWHW = (1<<14),
>
> -       SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_PKTINFO,
> +       SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_TX_SWHW,
>         SOF_TIMESTAMPING_MASK = (SOF_TIMESTAMPING_LAST - 1) |
>                                  SOF_TIMESTAMPING_LAST
>  };
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 58604c1..db5aa19 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3874,6 +3874,10 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
>         if (!sk)
>                 return;
>
> +       if (!hwtstamps && !(sk->sk_tsflags & SOF_TIMESTAMPING_OPT_TX_SWHW) &&
> +           skb_shinfo(orig_skb)->tx_flags & SKBTX_IN_PROGRESS)
> +               return;
> +

This check should only happen for software transmit timestamps, so simpler to
revise the check in sw_tx_timestamp above to

  if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP &&
-        !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
+      (!(skb_shinfo(orig_skb)->tx_flags & SKBTX_IN_PROGRESS)) ||
+      (skb->sk && skb->sk->sk_tsflags & SOF_TIMESTAMPING_OPT_TX_SWHW)

> diff --git a/net/socket.c b/net/socket.c
> index 68b9304..14b7688 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -720,6 +720,7 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
>                 empty = 0;
>         if (shhwtstamps &&
>             (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> +           (empty || !skb_is_err_queue(skb)) &&
>             ktime_to_timespec_cond(shhwtstamps->hwtstamp, tss.ts + 2)) {

I find skb->tstamp == 0 easier to understand than the condition on empty.

Indeed, this is so non-obvious that I would suggest another helper function
skb_is_hwtx_tstamp with a concise comment about the race condition
between tx software and hardware timestamps (as in the last sentence of
the commit message).