From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eu-smtp-delivery-151.mimecast.com (eu-smtp-delivery-151.mimecast.com [185.58.85.151]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CFDF5BA51 for ; Wed, 30 Nov 2022 22:39:43 +0000 (UTC) Received: from AcuMS.aculab.com (156.67.243.121 [156.67.243.121]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384) id uk-mta-50-6YXDDUj9OhOl-PdOYMK1rg-1; Wed, 30 Nov 2022 22:39:39 +0000 X-MC-Unique: 6YXDDUj9OhOl-PdOYMK1rg-1 Received: from AcuMS.Aculab.com (10.202.163.6) by AcuMS.aculab.com (10.202.163.6) with Microsoft SMTP Server (TLS) id 15.0.1497.42; Wed, 30 Nov 2022 22:39:38 +0000 Received: from AcuMS.Aculab.com ([::1]) by AcuMS.aculab.com ([::1]) with mapi id 15.00.1497.044; Wed, 30 Nov 2022 22:39:38 +0000 From: David Laight To: 'Thomas Gleixner' , "Jason A. Donenfeld" , "linux-kernel@vger.kernel.org" , "patches@lists.linux.dev" CC: "Jason A. Donenfeld" , "linux-crypto@vger.kernel.org" , "linux-api@vger.kernel.org" , "x86@kernel.org" , "Greg Kroah-Hartman" , Adhemerval Zanella Netto , Carlos O'Donell , "Florian Weimer" , Arnd Bergmann , Christian Brauner Subject: RE: [PATCH v10 1/4] random: add vgetrandom_alloc() syscall Thread-Topic: [PATCH v10 1/4] random: add vgetrandom_alloc() syscall Thread-Index: AQHZBD5MKfvjUBqLyk+xAMt8s9EHVq5YDxQQ Date: Wed, 30 Nov 2022 22:39:38 +0000 Message-ID: <310b91f650424d338e56794b8861a088@AcuMS.aculab.com> References: <20221129210639.42233-1-Jason@zx2c4.com> <20221129210639.42233-2-Jason@zx2c4.com> <87cz95v2q2.ffs@tglx> In-Reply-To: <87cz95v2q2.ffs@tglx> Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.202.205.107] Precedence: bulk X-Mailing-List: patches@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: aculab.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable From: Thomas Gleixner > Sent: 29 November 2022 22:02 >=20 > Jason! >=20 > On Tue, Nov 29 2022 at 22:06, Jason A. Donenfeld wrote: > > + > > +/******************************************************************** > > + * > > + * vDSO support helpers. > > + * > > + * The actual vDSO function is defined over in lib/vdso/getrandom.c, > > + * but this section contains the kernel-mode helpers to support that. > > + * > > + ********************************************************************/ > > + > > +#ifdef CONFIG_VGETRANDOM_ALLOC_SYSCALL > > +/** > > + * vgetrandom_alloc - allocate opaque states for use with vDSO getrand= om(). > > + * > > + * @num: on input, a pointer to a suggested hint of how many states to > > + * allocate, and on output the number of states actually allocated. > > + * > > + * @size_per_each: the size of each state allocated, so that the calle= r can > > + * split up the returned allocation into individual states. > > + * > > + * @flags: currently always zero. >=20 > NIT! >=20 > I personally prefer and ask for it in stuff I maintain: >=20 > * @num:=09=09On input, a pointer to a suggested hint of how many states = to > *=09=09=09allocate, and on output the number of states actually allocate= d. > * > * @size_per_each: =09The size of each state allocated, so that the calle= r can > * =09=09=09split up the returned allocation into individual states. > * > * @flags: =09=09Currently always zero. >=20 > But your turf :) >=20 > > + * > > + * The getrandom() vDSO function in userspace requires an opaque state= , which > > + * this function allocates by mapping a certain number of special page= s into > > + * the calling process. It takes a hint as to the number of opaque sta= tes > > + * desired, and provides the caller with the number of opaque states a= ctually > > + * allocated, the size of each one in bytes, and the address of the fi= rst > > + * state. >=20 > make W=3D1 rightfully complains about: >=20 > > + >=20 > drivers/char/random.c:182: warning: bad line: >=20 > > + * Returns a pointer to the first state in the allocation. >=20 > I have serious doubts that this statement is correct. >=20 > Think about this comment and documentation as a boiler plate for the > mandatory man page for a new syscall (hint...) >=20 > > + * > > + */ >=20 > and W=3D1 also complains rightfully here: >=20 > > +SYSCALL_DEFINE3(vgetrandom_alloc, unsigned int __user *, num, > > +=09=09unsigned int __user *, size_per_each, unsigned int, flags) >=20 > drivers/char/random.c:188: warning: expecting prototype for vgetrandom_al= loc(). Prototype was for > sys_vgetrandom_alloc() instead >=20 > > +{ > > diff --git a/include/vdso/getrandom.h b/include/vdso/getrandom.h > > new file mode 100644 > > index 000000000000..5f04c8bf4bd4 > > --- /dev/null > > +++ b/include/vdso/getrandom.h > > @@ -0,0 +1,24 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (C) 2022 Jason A. Donenfeld . All Rights= Reserved. > > + */ > > + > > +#ifndef _VDSO_GETRANDOM_H > > +#define _VDSO_GETRANDOM_H > > + > > +#include > > + > > +struct vgetrandom_state { > > +=09union { > > +=09=09struct { > > +=09=09=09u8 batch[CHACHA_BLOCK_SIZE * 3 / 2]; > > +=09=09=09u32 key[CHACHA_KEY_SIZE / sizeof(u32)]; > > +=09=09}; > > +=09=09u8 batch_key[CHACHA_BLOCK_SIZE * 2]; > > +=09}; > > +=09unsigned long generation; > > +=09u8 pos; > > +=09bool in_use; > > +}; >=20 > Again, please make this properly tabular: >=20 > struct vgetrandom_state { > =09union { > =09=09struct { > =09=09=09u8=09batch[CHACHA_BLOCK_SIZE * 3 / 2]; > =09=09=09u32=09key[CHACHA_KEY_SIZE / sizeof(u32)]; > =09=09}; > =09=09u8=09batch_key[CHACHA_BLOCK_SIZE * 2]; > =09}; > =09unsigned long=09generation; > =09u8=09=09pos; > =09bool=09=09in_use; > }; >=20 > Plus some kernel doc which explains what this is about. That structure looks horrid - especially for something shared between entities. The 'unsigned long' should be either u32 or u64. There is 'hidden' padding at the end. If 'pos' is an index into something (longer name would be better) then there is no reason to squeeze the value into 1 byte - it doesn't save anything and might make things bigger. (I think Jason might have blocked my emails, he doesn't like critisicism/feedback.) =09David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1= PT, UK Registration No: 1397386 (Wales)