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

[John Hubbard <jhubbard@nvidia.com>]

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