Re: [Devel] [PATCH hci-8.1 4/4] 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: lockless frozen_offsets fast path lacks acquire/release ordering
--

commit 0c3eee5060109c37a2e00993e54d077407012d3c
Author: Eva Kurchatova <eva.kurchatova@virtuozzo.com>

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

This moves the freezing of a time namespace's offsets out of the vDSO
page setup code and into timens_commit(), so that kernels without a
generic vDSO also freeze the offsets on the first task join. The vDSO
page is now filled from the same place, where one exists.

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

[ ... ]

> +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]
Is the lockless fast path in timens_freeze_offsets() ordered against the
vvar page setup on weakly ordered CPUs?

The first joiner runs timens_vdso_setup_vvar_page() and then publishes the
flag with a plain store:

    timens_vdso_setup_vvar_page(ns);
    ns->frozen_offsets = true;

A second task joining the same namespace (via timens_on_fork() or
commit_nsset()) reads the flag without taking the mutex:

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

Neither side has release/acquire semantics.  On arm64, powerpc or riscv,
could the second task observe frozen_offsets == true before the stores done
in timens_vdso_setup_vvar_page() (seq, clock_mode and the offsets set up by
timens_setup_vdso_clock_data()) are visible to it?

If so, that task would go on to read an un-namespaced or zeroed vvar page,
and its clocks would be wrong.  This would be wrong output rather than
memory corruption.  The window is very small and incidental
synchronisation, such as passing the namespace reference between tasks,
probably hides it in practice, so this may be speculative.

Would it make sense to use smp_store_release(&ns->frozen_offsets, true) for
the publication and smp_load_acquire(&ns->frozen_offsets) on the fast path?

> 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);
>  }

[Severity: Low]
These are the plain stores to the shared vvar page that must be visible to
any task that sees frozen_offsets set through the lockless fast path in
timens_freeze_offsets(), as described above.

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

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