Re: [PATCH v10 3/4] random: introduce generic vDSO getrandom() implementation

[Jason A. Donenfeld]

Hi Thomas,

On Tue, Nov 29, 2022 at 11:42:16PM +0100, Thomas Gleixner wrote:
> Jason!
> 
> On Tue, Nov 29 2022 at 22:06, Jason A. Donenfeld wrote:
> > +/**
> > + * struct vdso_rng_data - vdso RNG state information
> > + * @generation:	a counter representing the number of RNG reseeds

FYI, ever other struct in this file uses lower case and no punctuation,
so I'll follow that for this one.

> A counter
> 
> > + * @is_ready:	whether the RNG is initialized
> 
> Signals whether ...

Ack.

> > + */
> > +struct vdso_rng_data {
> > +	unsigned long	generation;
> > +	bool		is_ready;
> > +};
> > +
> > +
> > +#define MEMCPY_AND_ZERO_SRC(type, dst, src, len) do { \
> > +	while (len >= sizeof(type)) { \
> > +		__put_unaligned_t(type, __get_unaligned_t(type, src), dst); \
> > +		__put_unaligned_t(type, 0, src); \
> > +		dst += sizeof(type); \
> > +		src += sizeof(type); \
> > +		len -= sizeof(type); \
> > +	} \
> > +} while (0)
> 
> I'd appreciate it if you go back to the code I suggested to you and
> compare and contrast it in terms of readability.

Ahh, you like to align your \. Okay, I'll do that. I also added a do {
... } while (0) wrapper around it, but I think it makes sense to keep
that so that there aren't stray semicolons.

> > +/**
> > + * __cvdso_getrandom_data - generic vDSO implementation of getrandom() syscall
> > + * @rng_info:		describes state of kernel RNG, memory shared with kernel
> > + * @buffer:		destination buffer to fill with random bytes
> > + * @len:		size of @buffer in bytes
> > + * @flags:		zero or more GRND_* flags
> > + * @opaque_state:	a pointer to an opaque state area
> 
> NIT. Please start the explanations with an uppercase letter

Okay. Will do everywhere in this patchset except for in vdso/datapage.h.

 
> This one is odd:
> 
> > +	len = ret;
> 
> @ret is not modified after the initialization at the top of the
> function:
> 
> > +	ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = MAX_RW_COUNT */, len);
> 
> so I really had to go up a page and figure out what the story is.

If the generation changes, and it's tried again, the whole random buffer
is filled again, so that has to be reset. I'll leave a comment.

> > +
> > +	/* Since the batch was just refilled, set the position back to 0 to indicate a full batch. */
> > +	state->pos = 0;
> > +	goto more_batch;
> 
> Aside of the nitpicks above, thank you very much for making this
> comprehensible.

Thanks for nudging me in the right direction.

> 
> The comments are well done and appreciated and I'm pretty sure that this
> part:
> 
> > +	in_use = READ_ONCE(state->in_use);
> > +	if (unlikely(in_use))
> > +		goto fallback_syscall;
> > +	WRITE_ONCE(state->in_use, true);
> 
> was very much induced by writing those comments :)

Well, not exactly, unfortunately. Adhemerval -- the glibc maintainer
working on the libc side of this -- and I have been discussing signal
handling craziness and lots of different schemes over the last week+,
and this rather simple thing is the result of those conversations.

Jason