Received-SPF: pass (google.com: domain of linux-crypto+bounces-4941-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.117.160 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.160; helo=mail.nvidia.com; pr=C
Message-ID: <13483c92-cac5-4a3a-891f-22eb006c533b@nvidia.com>
Date: Fri, 14 Jun 2024 18:53:09 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v17 5/5] x86: vdso: Wire up getrandom() vDSO
 implementation
To: "Jason A. Donenfeld" <Jason@zx2c4.com>, <linux-kernel@vger.kernel.org>,
	<patches@lists.linux.dev>, <tglx@linutronix.de>
CC: <linux-crypto@vger.kernel.org>, <linux-api@vger.kernel.org>,
	<x86@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Adhemerval Zanella Netto" <adhemerval.zanella@linaro.org>, Carlos O'Donell
	<carlos@redhat.com>, Florian Weimer <fweimer@redhat.com>, Arnd Bergmann
	<arnd@arndb.de>, Jann Horn <jannh@google.com>, Christian Brauner
	<brauner@kernel.org>, David Hildenbrand <dhildenb@redhat.com>, Samuel Neves
	<sneves@dei.uc.pt>, <linux-kselftest@vger.kernel.org>
References: <20240614190646.2081057-1-Jason@zx2c4.com>
 <20240614190646.2081057-6-Jason@zx2c4.com>
Content-Language: en-US
From: John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <20240614190646.2081057-6-Jason@zx2c4.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Jun 2024 01:53:23.7226
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 9255a5e8-c346-4d7e-db56-08dc8cddf0f4
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.160];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource:
	CO1PEPF000042AA.namprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB7501

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