netdev - Re: [RFC PATCH v3 5/7] virtio

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]

Date: Thu, 20 Jun 2024 14:01:01 +0200
From: Peter Hilber <peter.hilber@...nsynergy.com>
To: David Woodhouse <dwmw2@...radead.org>, linux-kernel@...r.kernel.org,
 virtualization@...ts.linux.dev, virtio-dev@...ts.oasis-open.org
Cc: "Michael S. Tsirkin" <mst@...hat.com>, Jason Wang <jasowang@...hat.com>,
 Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
 Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org,
 Marc Zyngier <maz@...nel.org>, Mark Rutland <mark.rutland@....com>,
 Daniel Lezcano <daniel.lezcano@...aro.org>,
 Thomas Gleixner <tglx@...utronix.de>, linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC PATCH v3 5/7] virtio_rtc: Add PTP clocks

On 15.06.24 10:01, David Woodhouse wrote:
> On Mon, 2023-12-18 at 08:38 +0100, Peter Hilber wrote:
>>
>> +       ret = viortc_hw_xtstamp_params(&hw_counter, &cs_id);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ktime_get_snapshot(&history_begin);
>> +       if (history_begin.cs_id != cs_id)
>> +               return -EOPNOTSUPP;
> 
> I think you have to call ktime_get_snapshot() anyway to get a snapshot
> from before your crosststamp? But I still don't much like the fact that
> you need to use it to work out which cs_id is being used.

The actual cs_id check is in get_device_system_crosststamp(), where it was
added recently [1]. So this additional check is just verifying that the
history_begin is usable.

> 
> Shouldn't get_device_system_crosststamp() pass that to its get_time_fn
> as a hint?

This is unneeded in this case, since get_device_system_crosststamp() does
the check already (but the driver is free to pass it through the
get_time_fn parameter ctx).

> 
> On x86, you are likely to find that history_begin.cs_id is the KVM
> clock, so this will return -EOPNOTSUPP and userspace will have to fall
> back to PTP_SYS_OFFSET. I note the KVM PTP clock actually *converts* a
> TSC-based crosststamp to kvmclock µs for itself, so that it can report 
> *cs_id = CSID_X86_KVM_CLK. Not sure how I feel about that though. I'm
> inclined to suggest that it shouldn't, as anyone who wants accurate
> timekeeping shouldn't be using the KVM clock anyway.
> 
> But we should at least be relatively consistent about it.

ATM, the driver does indeed not have TSC support (for cross-timestamping)
enabled at all, so would always use fallback. If *not* using the KVM clock,
I think TSC can just be enabled by adding architecture-specific code
similar to virtio_rtc_arm.c.

I am not familiar with the KVM clock, but maybe it would be sufficient to
allow CSID_X86_KVM_CLK as well?

Thanks for the comments,

Peter

[1] https://git.kernel.org/torvalds/c/4b7f521229ef