linux-kernel - Re: [PATCH] timekeeping: Always initialize use_nsecs when querying time from phc drivers

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]

Message-ID: <eouuhjxmtlnrrsvi45mzyjzrycwjwhzgut3hnlghhbblbdkdeu@5nzama2xthvh>
Date: Sun, 20 Jul 2025 15:51:27 +0200
From: Markus Blöchl <markus@...chl.de>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Markus Blöchl <Markus.Bloechl@...tronik.com>, 
	John Stultz <jstultz@...gle.com>, "Christopher S. Hall" <christopher.s.hall@...el.com>, 
	Lakshmi Sowjanya D <lakshmi.sowjanya.d@...el.com>, Stephen Boyd <sboyd@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] timekeeping: Always initialize use_nsecs when querying
 time from phc drivers

Hi Thomas,

On Fri, Jul 18, 2025 at 10:25:12PM +0200, Thomas Gleixner wrote:
> On Wed, Jul 09 2025 at 10:32, Markus Blöchl wrote:
> > On Tue, Jul 08, 2025 at 12:09:40PM -0700, John Stultz wrote:
> >> On Tue, Jul 8, 2025 at 9:46 AM Markus Blöchl
> >> <markus.bloechl@...tronik.com> wrote:
> >> >
> >> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> >> > index a009c91f7b05..be0da807329f 100644
> >> > --- a/kernel/time/timekeeping.c
> >> > +++ b/kernel/time/timekeeping.c
> >> > @@ -1269,6 +1269,8 @@ int get_device_system_crosststamp(int (*get_time_fn)
> >> >
> >> >         do {
> >> >                 seq = read_seqcount_begin(&tk_core.seq);
> >> > +               system_counterval.use_nsecs = false;
> >> > +
> >>
> >> So if the argument is the local system_counterval structure isn't
> >> being fully initialized by the get_time_fn() functions it is passed
> >> to, it seems like it would be better to do so at the top of
> >> get_device_system_crosststamp(), and not inside the seqloop.
> >
> > Probably, I was just afraid of the case where get_time_fn() would take
> > like *very* different paths during different iterations.
> > But that seems really unlikely, indeed.
> 
> It's impossible. xtstamp->device and the related get_time_fn() are
> immutable during the call.

I don't see it being entirely impossible, just nonsensical.
get_time_fn() tends to read volatile external data and is thus far from
being a pure function.
Some implementations (e.g. ptp_vmclock_get_time_fn()) can take some
weird turns, which I did not want to follow all the way.
I have no clue what could happen e.g. during a VM migration.
That's why resetting the struct before every invocation seemed to be
the safer option to me.
If you are certain that something like that can never happen, then I'm
totally happy.

> 
> >> But having the responsibility to initialize/fill in the structure
> >> being split across the core and the implementation logic (leaving some
> >> of the fields as optional) feels prone to mistakes, so it makes me
> >> wonder if those drivers implementing the get_time_fn() really ought to
> >> fully fill out the structure, and thus the fix would be better done in
> >> those drivers.
> >
> > Yes, they should.
> 
> No, they should not.
> 
> The data structure is instantiated in get_device_system_crosststamp()
> and then handed in un-initialized to get_time_fn(), which is wrong to
> begin with. Why?
> 
> That means if the structure is ever expanded, then you'd have to fix up
> all of the get_time_fn() implementations.

As long as all get_time_fn() implementations adhere to the convention of
always assigning an entire, fully-initialized struct and are always
compiled against the same headers as the kernel, I don't see that
necessity. But yes, those are obviously a lot of "if"s...

> 
> Seriously?
> 
> The obviously correct and future proof thing to do is:
> 
> -	struct system_counterval_t system_counterval;
> +	struct system_counterval_t system_counterval = { };
> 
> Which fixes the problem you discovered once and forever, no?

Works for me, too.
Reroll follows.

> 
> Thanks,
> 
>         tglx

Thanks a lot,

Markus

--