Re: [Devel] [PATCH hci-8.1 v2 4/6] timens: freeze the offsets on the first join without the vDSO too

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] timens: frozen_offsets published without release/acquire after vvar page setup
--

commit 594d8fa1aae41e5f90c67ea3a3e7ad325b6b1173
Author: Eva Kurchatova <eva.kurchatova@virtuozzo.com>

timens: freeze the offsets on the first join without the vDSO too

This patch moves the freezing of time namespace offsets out of the vDSO
page setup code and into timens_commit(). Kernels without a generic vDSO
(UML) now freeze the offsets on the first join as well. The vDSO page fill
and the vvar zap are now called from timens_commit() through
timens_vdso_setup_vvar_page() and timens_vdso_join().

Link: https://virtuozzo.atlassian.net/browse/VSTOR-148112

> diff --git a/kernel/time/namespace.c b/kernel/time/namespace.c
> --- a/kernel/time/namespace.c
> +++ b/kernel/time/namespace.c

[ ... ]

> +/*
> + * The offsets are read without a lock once a task runs in the namespace,
> + * so the first task to join freezes them, and sets up the vDSO page from
> + * them where there is one.
> + */
> +static void timens_freeze_offsets(struct time_namespace *ns)
> +{
> +	if (ns == &init_time_ns)
> +		return;
> +
> +	/* Fast-path, taken by every task in namespace except the first. */
> +	if (likely(ns->frozen_offsets))
> +		return;
> +
> +	guard(mutex)(&timens_offset_lock);
> +	if (ns->frozen_offsets)
> +		return;
> +
> +	timens_vdso_setup_vvar_page(ns);
> +	ns->frozen_offsets = true;
> +}

[Severity: Low]
This is speculative, and I could not construct a concrete failing
interleaving. Is frozen_offsets published safely here?

The first joiner stores the vvar page contents in
timens_vdso_setup_vvar_page() and then does a plain store:

    ns->frozen_offsets = true;

A second task in another thread group calling timens_commit() takes the
unlocked fast path:

    if (likely(ns->frozen_offsets))
        return;

The mutex only orders lock holders against each other, so this load is not
ordered against the vvar page stores (seq, clock_mode, offsets) made by the
first joiner.

The comment on timens_vdso_setup_vvar_page() says setup happens "before any
task reads", but no release/acquire pairing enforces that. On a weakly
ordered CPU, could the second joiner see frozen_offsets == true and then
have its userspace read a stale or zeroed vvar page?

timens_vdso_join() takes mmap_read_lock only after the flag load, and
find_timens_vvar_page() does not re-check the flag, so neither seems to
supply acquire semantics for the earlier load. In practice the window looks
very narrow, and x86 and UML are strongly ordered.

The flag is also read and written with plain accesses, which KCSAN could
report as a data race.

Would smp_store_release(&ns->frozen_offsets, true) in the slow path and
smp_load_acquire(&ns->frozen_offsets) in the fast path, with READ_ONCE() for
the check under the mutex, be appropriate?

[ ... ]

> diff --git a/kernel/time/namespace_vdso.c b/kernel/time/namespace_vdso.c
> --- a/kernel/time/namespace_vdso.c
> +++ b/kernel/time/namespace_vdso.c

[ ... ]

> +/* Called once per namespace, under timens_offset_lock, before any task reads */
> +void timens_vdso_setup_vvar_page(struct time_namespace *ns)
>  {
> +	struct vdso_time_data *vdata = page_address(ns->vvar_page);
> +	struct vdso_clock *vc = vdata->clock_data;
>  	unsigned int i;
>  
>  	for (i = 0; i < CS_BASES; i++)
>  		timens_setup_vdso_clock_data(&vc[i], ns);
>  }

These are the vvar page stores that need to be visible before the flag is
observed by a fast-path joiner in timens_freeze_offsets().

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261019000649.1371025-1-eva.kurchatova@virtuozzo.com?part=4

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel