netdev - Re: Extending socket timestamping API for NTP

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 27 Feb 2017 19:01:54 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Miroslav Lichvar <mlichvar@...hat.com>
Cc:     "Keller, Jacob E" <jacob.e.keller@...el.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Richard Cochran <richardcochran@...il.com>,
        Jiri Benc <jbenc@...hat.com>, Denny Page <dennypage@...com>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: Extending socket timestamping API for NTP

On Mon, Feb 27, 2017 at 10:23 AM, Miroslav Lichvar <mlichvar@...hat.com> wrote:
> On Tue, Feb 07, 2017 at 02:32:04PM -0800, Willem de Bruijn wrote:
>> >> 4) allow sockets to use both SW and HW TX timestamping at the same time
>> >>
>> >>    When using a socket which is not bound to a specific interface, it
>> >>    would be nice to get transmit SW timestamps when HW timestamps are
>> >>    missing. I suspect it's difficult to predict if a HW timestamp will
>> >>    be available. Maybe it would be acceptable to get from the error
>> >>    queue two messages per transmission if the interface supports both
>> >>    SW and HW timestamping?
>> >
>> >
>> > This seems useful,
>>
>> Agreed, as long as it is optional so that it does not change the
>> behavior for existing applications.
>
> Do you think it is safe to assume that no application enabled both SW
> and HW TX timestamping?

We cannot rule out that a process set both flags.

> Do we need a new option for this?

Similar to OPT_TSONLY or OPT_ID, but to signal the intent of
receiving both timestamps. Yes, agreed.

>> > but not sure how best to implement it.
>>
>> It might be sufficient to just remove the second line in sw_tx_timestamp
>>
>> 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))
>>                 skb_tstamp_tx(skb, NULL);
>> }
>
> With this change I'm getting two error messages per transmission, but
> it looks like it may need some additional changes.
>
> If the first error message is received after the HW timestamp was
> captured,

When does this happen? The first timestamp is generated from
skb_tx_timestamp in the device driver's ndo_start_xmit before
passing the packet to the NIC, the second when the device
driver cleans the tx descriptor on completion.

Is this for drivers that do not have skb_tx_timestamp, as you
mention below? Then the solution is to add that call.

> it contains both timestamps as the HW timestamp is in the
> shared info of the skb. Is it possible it could contain a partially
> updated HW timestamp? I'm not sure how locking works here. Is
> scm_timestamping actually allowed to contain more than one timestamp?
> The timestamping.txt document says "Only one field is non-zero at any
> time.", but that wasn't true even before if both SW and HW RX
> timestamping was enabled.
>
> If SO_TIMESTAMP{,NS} is enabled, ts[0] in the second error message
> will contain a bogus SW timestamp added by __sock_recv_timestamp() for
> a "Race occurred between timestamp enabling and packet receiving".  Is

Good point. That should not be set on transmit timestamps.

> there a guarantee applications will get a timestamp for all messages
> after enabling SO_TIMESTAMP? The original code is older than the git
> repo, so I'm not sure what was the reason for this. To me it would
> make more sense to not add any SCM_TIMESTAMP (and SW timestamp in
> SCM_TIMESTAMPING) when the the timestamp is missing. If that's not
> always acceptable, maybe it could be restricted to sockets that have
> HW timestamping enabled?

I would limit scope to tx timestamping and leave rx semantics as is.

> Some drivers don't call skb_tx_timestamp() when HW timestamp was
> requested. From a cursory look it is e1000e, xgbe, sxgbe, and stmmac.
> This should hopefully be an easy fix.

Indeed. that should be added, then.