netdev - Re: [PATCH net-next v3 3/5] eth: fbnic: add RX packets timestamping support

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]

Message-ID: <09283978-f414-4c77-b48e-f5586fa67edf@linux.dev>
Date: Tue, 8 Oct 2024 17:47:05 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Jacob Keller <jacob.e.keller@...el.com>, Vadim Fedorenko
 <vadfed@...a.com>, Jakub Kicinski <kuba@...nel.org>,
 David Ahern <dsahern@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 "David S. Miller" <davem@...emloft.net>,
 Alexander Duyck <alexanderduyck@...com>
Cc: netdev@...r.kernel.org, Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH net-next v3 3/5] eth: fbnic: add RX packets timestamping
 support

On 05/10/2024 00:14, Jacob Keller wrote:
> 
> 
> On 10/3/2024 5:39 AM, Vadim Fedorenko wrote:
>> Add callbacks to support timestamping configuration via ethtool.
>> Add processing of RX timestamps.
>>
>> Signed-off-by: Vadim Fedorenko <vadfed@...a.com>
>>   
>> +/**
>> + * fbnic_ts40_to_ns() - convert descriptor timestamp to PHC time
>> + * @fbn: netdev priv of the FB NIC
>> + * @ts40: timestamp read from a descriptor
>> + *
>> + * Return: u64 value of PHC time in nanoseconds
>> + *
>> + * Convert truncated 40 bit device timestamp as read from a descriptor
>> + * to the full PHC time in nanoseconds.
>> + */
>> +static __maybe_unused u64 fbnic_ts40_to_ns(struct fbnic_net *fbn, u64 ts40)
>> +{
>> +	unsigned int s;
>> +	u64 time_ns;
>> +	s64 offset;
>> +	u8 ts_top;
>> +	u32 high;
>> +
>> +	do {
>> +		s = u64_stats_fetch_begin(&fbn->time_seq);
>> +		offset = READ_ONCE(fbn->time_offset);
>> +	} while (u64_stats_fetch_retry(&fbn->time_seq, s));
>> +
>> +	high = READ_ONCE(fbn->time_high);
>> +
>> +	/* Bits 63..40 from periodic clock reads, 39..0 from ts40 */
>> +	time_ns = (u64)(high >> 8) << 40 | ts40;
>> +
>> +	/* Compare bits 32-39 between periodic reads and ts40,
>> +	 * see if HW clock may have wrapped since last read
>> +	 */
>> +	ts_top = ts40 >> 32;
>> +	if (ts_top < (u8)high && (u8)high - ts_top > U8_MAX / 2)
>> +		time_ns += 1ULL << 40;
>> +
>> +	return time_ns + offset;
>> +}
>> +
> 
> This logic doesn't seem to match the logic used by the cyclecounter
> code, and Its not clear to me if this safe against a race between
> time_high updating and the packet timestamp arriving.
> 
> the timestamp could arrive either before or after the time_high update,
> and the logic needs to ensure the appropriate high bits are applied in
> both cases.

To avoid this race condition we decided to make sure that incoming
timestamps are always later then cached high bits. That will make the
logic above correct.

> Again, I think your use case makes sense to just implement with a
> timecounter and cyclecounter, since you're not modifying the hardware
> cycle counter and are leaving it as free-running.

After discussion with Jakub we decided to keep simple logic without
timecounter + cyclecounter, as it's pretty straight forward.