Re: [PATCH v17 5/5] x86: vdso: Wire up getrandom() vDSO implementation

John Hubbard
2024-06-15T01:53:27Z

On 6/14/24 12:06 PM, Jason A. Donenfeld wrote:
> Hook up the generic vDSO implementation to the x86 vDSO data page. Since
> the existing vDSO infrastructure is heavily based on the timekeeping
> functionality, which works over arrays of bases, a new macro is
> introduced for vvars that are not arrays.
>
> The vDSO function requires a ChaCha20 implementation that does not write
> to the stack, yet can still do an entire ChaCha20 permutation, so
> provide this using SSE2, since this is userland code that must work on
> all x86-64 processors. There's a simple test for this code as well.
>
> Reviewed-by: Samuel Neves <sneves@dei.uc.pt> # for vgetrandom-chacha.S
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> arch/x86/Kconfig | 1 +
> arch/x86/entry/vdso/Makefile | 3 +-
> arch/x86/entry/vdso/vdso.lds.S | 2 +
> arch/x86/entry/vdso/vgetrandom-chacha.S | 178 ++++++++++++++++++
> arch/x86/entry/vdso/vgetrandom.c | 17 ++
> arch/x86/include/asm/vdso/getrandom.h | 55 ++++++
> arch/x86/include/asm/vdso/vsyscall.h | 2 +
> arch/x86/include/asm/vvar.h | 16 ++
> tools/testing/selftests/vDSO/.gitignore | 1 +
> tools/testing/selftests/vDSO/Makefile | 13 ++
> .../testing/selftests/vDSO/vdso_test_chacha.c | 43 +++++

Hi Jason,

This is a large patch, so it might be helpful to split out the selftests
into their own patch. In fact, my comments here are only about those.

I'm adding linux-kselftest to Cc for visibility, and I've also Cc'd you
on a related selftests/vDSO series I just now posted [1].

In fact, I think it might work well if you insert patches 2/3 and 3/3
from that series, and build on top of those for the
selftests/vDSO/Makefile. See below for details.

...

> diff --git a/tools/testing/selftests/vDSO/Makefile b/tools/testing/selftests/vDSO/Makefile
> index a33b4d200a32..8b87ebea1630 100644
> --- a/tools/testing/selftests/vDSO/Makefile
> +++ b/tools/testing/selftests/vDSO/Makefile
> @@ -3,6 +3,7 @@ include ../lib.mk
>
> uname_M := $(shell uname -m 2>/dev/null || echo not)
> ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/x86/ -e s/x86_64/x86/)
> +SODIUM := $(shell pkg-config --libs libsodium 2>/dev/null)
>
> TEST_GEN_PROGS := $(OUTPUT)/vdso_test_gettimeofday $(OUTPUT)/vdso_test_getcpu
> TEST_GEN_PROGS += $(OUTPUT)/vdso_test_abi
> @@ -12,9 +13,15 @@ TEST_GEN_PROGS += $(OUTPUT)/vdso_standalone_test_x86
> endif
> TEST_GEN_PROGS += $(OUTPUT)/vdso_test_correctness
> TEST_GEN_PROGS += $(OUTPUT)/vdso_test_getrandom
> +ifeq ($(uname_M),x86_64)
> +ifneq ($(SODIUM),)
> +TEST_GEN_PROGS += $(OUTPUT)/vdso_test_chacha

Unfortunately, this is "pre-existing wrong". :) That is, it is following
a pre-existing pattern that is broken: the $(OUTPUT) is not supposed to
be part of TEST_GEN_PROGS. Fixing it requires other changes, though, as
I've done in [2].

[1] https://lore.kernel.org/all/20240614233105.265009-1-jhubbard@nvidia.com/
[2] https://lore.kernel.org/all/20240614233105.265009-3-jhubbard@nvidia.com/
[3] https://lore.kernel.org/all/20240614233105.265009-4-jhubbard@nvidia.com/

> +endif
> +endif
>
> CFLAGS := -std=gnu99
> CFLAGS_vdso_standalone_test_x86 := -nostdlib -fno-asynchronous-unwind-tables -fno-stack-protector
> +CFLAGS_vdso_test_chacha := $(SODIUM) -idirafter $(top_srcdir)/include -idirafter $(top_srcdir)/arch/$(ARCH)/include -idirafter include -D__ASSEMBLY__ -DBULID_VDSO -DCONFIG_FUNCTION_ALIGNMENT=0 -Wa,--noexecstack

Line breaks via "\" are allowed in Makefiles. Might need two or three here.

> LDFLAGS_vdso_test_correctness := -ldl
> ifeq ($(CONFIG_X86_32),y)
> LDLIBS += -lgcc_s
> @@ -35,3 +42,9 @@ $(OUTPUT)/vdso_test_correctness: vdso_test_correctness.c
> -o $@ \
> $(LDFLAGS_vdso_test_correctness)
> $(OUTPUT)/vdso_test_getrandom: parse_vdso.c
> +$(OUTPUT)/vdso_test_chacha: CFLAGS += $(CFLAGS_vdso_test_chacha)

This also follows an unfortunate pattern, which I've also fixed just today
in a patch [3]. Please just add to CFLAGS directly, rather than creating
these name-mangled intermediate variables. See [3] for how that looks.

> +$(OUTPUT)/vdso_test_chacha: $(top_srcdir)/arch/$(ARCH)/entry/vdso/vgetrandom-chacha.S
> +$(OUTPUT)/vdso_test_chacha: include/asm/rwonce.h
> +include/asm/rwonce.h:
> + mkdir -p include/asm
> + touch $@

thanks,
--
John Hubbard
NVIDIA