netdev - Re: [PATCH v4 net-next 6/7] 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] [day] [month] [year] [list]

Date:   Fri, 19 May 2017 10:52:24 -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>
Subject: Re: [PATCH v4 net-next 6/7] net: allow simultaneous SW and HW
 transmit timestamping

On Fri, May 19, 2017 at 6:00 AM, Miroslav Lichvar <mlichvar@...hat.com> wrote:
> On Thu, May 18, 2017 at 04:16:26PM -0400, Willem de Bruijn wrote:
>> On Thu, May 18, 2017 at 9:06 AM, Miroslav Lichvar <mlichvar@...hat.com> wrote:
>> > +/* On transmit, software and hardware timestamps are returned independently.
>> > + * As the two skb clones share the hardware timestamp, which may be updated
>> > + * before the software timestamp is received, a hardware TX timestamp may be
>> > + * returned only if there is no software TX timestamp. A false software
>> > + * timestamp made for SOCK_RCVTSTAMP when a real timestamp is missing must
>> > + * be ignored.
>>
>> Please expand on why this case can be ignored. It is quite subtle. How about
>> something like
>>
>> *
>> * A false software timestamp is one made inside the __sock_recv_timestamp
>> * call itself. These are generated whenever SO_TIMESTAMP(NS) is enabled
>> * on the socket, even when the timestamp reported is for another option, such
>> * as hardware tx timestamp.
>> *
>> * Ignore these when deciding whether a timestamp source is hw or sw.
>> */
>
> That seems a bit too verbose to me. :) Would the following work?
>
> /* On transmit, software and hardware timestamps are returned independently.
>  * As the two skb clones share the hardware timestamp, which may be updated
>  * before the software timestamp is received, a hardware TX timestamp may be
>  * returned only if there is no software TX timestamp. Ignore false software
>  * timestamps, which may be made in the __sock_recv_timestamp() call when the
>  * option SO_TIMESTAMP(NS) is enabled on the socket, even when the skb has a
>  * hardware timestamp.
>  */

Looks great, thanks.

>
>> > +static bool skb_is_swtx_tstamp(const struct sk_buff *skb,
>> > +                              const struct sock *sk, int false_tstamp)
>> > +{
>> > +       if (false_tstamp && sk->sk_tsflags & SOF_TIMESTAMPING_OPT_TX_SWHW)
>>
>> Also, why is it ignored only for the new mode?
>
> Good point. That should not be there. The function can be now reduced
> to a single line again. I originally tried a different approach,
> disabling false timestamps in the new mode, but then I thought it's
> better to not complicate it unnecessarily and keep it consistent.
>
> --
> Miroslav Lichvar