linux-kernel - Re: [patch V2 26/26] timekeeping: Provide interface to control auxiliary clocks

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: <20250613083529-510f21e5-1c61-4320-afb0-863666b0f590@linutronix.de>
Date: Fri, 13 Jun 2025 09:18:19 +0200
From: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: LKML <linux-kernel@...r.kernel.org>, netdev@...r.kernel.org, 
	Richard Cochran <richardcochran@...il.com>, Christopher Hall <christopher.s.hall@...el.com>, 
	John Stultz <jstultz@...gle.com>, Frederic Weisbecker <frederic@...nel.org>, 
	Anna-Maria Behnsen <anna-maria@...utronix.de>, Miroslav Lichvar <mlichvar@...hat.com>, 
	Werner Abt <werner.abt@...nberg-usa.com>, David Woodhouse <dwmw2@...radead.org>, 
	Stephen Boyd <sboyd@...nel.org>, Kurt Kanzenbach <kurt@...utronix.de>, 
	Nam Cao <namcao@...utronix.de>, Antoine Tenart <atenart@...nel.org>
Subject: Re: [patch V2 26/26] timekeeping: Provide interface to control
 auxiliary clocks

On Mon, May 19, 2025 at 10:33:46AM +0200, Thomas Gleixner wrote:
> Auxiliary clocks are disabled by default and attempts to access them
> fail.
> 
> Provide an interface to enable/disable them at run-time.
> 
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
>  Documentation/ABI/stable/sysfs-kernel-time-aux-clocks |    5 
>  kernel/time/timekeeping.c                             |  125 ++++++++++++++++++
>  2 files changed, 130 insertions(+)

<snip>

> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -14,6 +14,7 @@
>  #include <linux/sched/loadavg.h>
>  #include <linux/sched/clock.h>
>  #include <linux/syscore_ops.h>
> +#include <linux/sysfs.h>

#include <linux/kobject.h>

>  #include <linux/clocksource.h>
>  #include <linux/jiffies.h>
>  #include <linux/time.h>
> @@ -2900,6 +2901,130 @@ const struct k_clock clock_aux = {
>  	.clock_adj		= aux_clock_adj,
>  };
>  
> +static void aux_clock_enable(unsigned int id)

"clockid_t id".

> +{
> +	struct tk_read_base *tkr_raw = &tk_core.timekeeper.tkr_raw;
> +	struct tk_data *tkd = aux_get_tk_data(id);
> +	struct timekeeper *tks = &tkd->shadow_timekeeper;
> +
> +	/* Prevent the core timekeeper from changing. */
> +	guard(raw_spinlock_irq)(&tk_core.lock);
> +
> +	/*
> +	 * Setup the auxiliary clock assuming that the raw core timekeeper
> +	 * clock frequency conversion is close enough. Userspace has to
> +	 * adjust for the deviation via clock_adjtime(2).
> +	 */
> +	guard(raw_spinlock_nested)(&tkd->lock);
> +
> +	/* Remove leftovers of a previous registration */
> +	memset(tks, 0, sizeof(*tks));
> +	/* Restore the timekeeper id */
> +	tks->id = tkd->timekeeper.id;
> +	/* Setup the timekeeper based on the current system clocksource */
> +	tk_setup_internals(tks, tkr_raw->clock);
> +
> +	/* Mark it valid and set it live */
> +	tks->clock_valid = true;
> +	timekeeping_update_from_shadow(tkd, TK_UPDATE_ALL);
> +}

<snip>

> +static struct kobj_attribute aux_clock_enable_attr = __ATTR_RW(aux_clock_enable);
> +
> +static struct attribute *aux_clock_enable_attrs[] = {
> +	&aux_clock_enable_attr.attr,
> +	NULL,

There should be no comma after sentinel values.

> +};
> +
> +static const struct attribute_group aux_clock_enable_attr_group = {
> +	.attrs = aux_clock_enable_attrs,
> +};
> +
> +static int __init tk_aux_sysfs_init(void)
> +{
> +	struct kobject *auxo, *tko = kobject_create_and_add("time", kernel_kobj);
> +
> +	if (!tko) {
> +		pr_warn("Unable to create /sys/kernel/time/. POSIX AUX clocks disabled.\n");

The other timer code does not log details on failed initcalls.
To me the strings look like wasted memory.
If failure is really intended to be handled gracefully then some cleanup logic
may be necessary, too.

> +		return -ENOMEM;
> +	}
> +
> +	auxo = kobject_create_and_add("aux_clocks", tko);
> +	if (!auxo) {
> +		pr_warn("Unable to create /sys/kernel/time/aux_clocks. POSIX AUX clocks disabled.\n");
> +		kobject_put(tko);
> +		return -ENOMEM;
> +	}
> +
> +	for (int i = TIMEKEEPER_AUX; i <= TIMEKEEPER_AUX_LAST; i++) {

for (int i = 0; i < MAX_AUX_CLOCKS; i++)

This avoids the need for the repeated calculations below and also better
matches the actual semantics.

> +		char id[2] = { [0] = '0' + (i - TIMEKEEPER_AUX), };
> +		struct kobject *clk = kobject_create_and_add(id, auxo);
> +
> +		if (!clk) {
> +			pr_warn("Unable to create /sys/kernel/time/aux_clocks/%d\n",
> +				i - TIMEKEEPER_AUX);
> +			return -ENOMEM;
> +		}
> +
> +		int ret = sysfs_create_group(clk, &aux_clock_enable_attr_group);
> +
> +		if (ret) {
> +			pr_warn("Unable to create /sys/kernel/time/aux_clocks/%d/enable\n",
> +				i - TIMEKEEPER_AUX);


> +			return ret;
> +		}
> +	}
> +	return 0;
> +}
> +late_initcall(tk_aux_sysfs_init);
> +
>  static __init void tk_aux_setup(void)
>  {
>  	For (int i = TIMEKEEPER_AUX; i <= TIMEKEEPER_AUX_LAST; i++)
>