On Mon, Sep 02, 2024 at 12:52:57PM +0000, Adhemerval Zanella wrote:
> +static __always_inline const struct vdso_rng_data *__arch_get_vdso_rng_data(void)
> +{
> + /*
> + * If a task belongs to a time namespace then the real VVAR page is mapped
> + * with the VVAR_TIMENS_PAGE_OFFSET offset.
> + */
This confused me, and I see that it is truncated from the existing
commit in arch/arm64/kerne/vdso.c:
If a task belongs to a time namespace then a namespace specific VVAR is
mapped with the VVAR_DATA_PAGE_OFFSET and the real VVAR page is mapped
with the VVAR_TIMENS_PAGE_OFFSET offset.
... and IIUC the "namespace specific VVAR" page doesn't have the RNG
data, right? It'd be good to spell that out, e.g.
/*
* The RNG data is in the real VVAR data page, but if a task
* belongs to a time namespsace then VVAR_DATA_PAGE_OFFSET
* points to the namespace-specific VVAR page and
* VVAR_TIMENS_PAGE_OFFSET points to the real VVAR page.
*/
It does feel weird that everything else has to work around timer
namespaces rather than that being limited to the timer code, so we'll
probably want to flip that if we add anything else to the VDSO, or have
a separate VVAR_RNG page.
> + if (IS_ENABLED(CONFIG_TIME_NS) && _vdso_data->clock_mode == VDSO_CLOCKMODE_TIMENS)
> + return (void*)&_vdso_rng_data + VVAR_TIMENS_PAGE_OFFSET * PAGE_SIZE;
> + return &_vdso_rng_data;
> +}
[...]
> diff --git a/arch/arm64/kernel/vdso/vgetrandom.c b/arch/arm64/kernel/vdso/vgetrandom.c
> new file mode 100644
> index 000000000000..95682d29c4bf
> --- /dev/null
> +++ b/arch/arm64/kernel/vdso/vgetrandom.c
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +typeof(__cvdso_getrandom) __kernel_getrandom;
> +
> +ssize_t __kernel_getrandom(void *buffer, size_t len, unsigned int flags, void *opaque_state, size_t opaque_len)
> +{
> + asm goto (
> + ALTERNATIVE("b %[fallback]", "nop", ARM64_HAS_FPSIMD) : : : : fallback);
> + return __cvdso_getrandom(buffer, len, flags, opaque_state, opaque_len);
> +
> +fallback:
> + if (unlikely(opaque_len == ~0UL && !buffer && !len && !flags))
> + return -ENOSYS;
> + return getrandom_syscall(buffer, len, flags);
> +}
The asm it pretty painful to read, and AFAICT what you actually want
here is alternative_has_cap_likely(), which we could use were that not
using alt_cb_patch_nops behind the scenes.
I reckon it's worth making the work for the VDSO first (patch below);
that way you can make this much nicer:
ssize_t __kernel_getrandom(void *buffer, size_t len, unsigned int flags,
void *opaque_state, size_t opaque_len)
{
if (alternative_has_cap_likely(ARM64_HAS_FPSIMD)) {
return __cvdso_getrandom(buffer, len, flags,
opaque_state, opaque_len);
}
if (unlikely(opaque_len == ~0UL && !buffer && !len && !flags))
return -ENOSYS;
return getrandom_syscall(buffer, len, flags);
}
... though the conditions for returning -ENOSYS look very odd to me; why
do we care about fast-pathing that specific case rather than forwarding
that to the kernel, and does __cvdso_getrandom() handle that correctly?
Mark
---->8----
