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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <a64b3bfd-8a85-4523-aad8-e4b534448a0b@intel.com>
Date: Fri, 4 Oct 2024 16:14:52 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Vadim Fedorenko <vadfed@...a.com>, Vadim Fedorenko
	<vadim.fedorenko@...ux.dev>, 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 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.

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.