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
A counter
> + * @is_ready: whether the RNG is initialized
Signals whether ...
> + */
> +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.
> +
> +static void memcpy_and_zero_src(void *dst, void *src, size_t len)
> +{
> + if (IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {
> + if (IS_ENABLED(CONFIG_64BIT))
> + MEMCPY_AND_ZERO_SRC(u64, dst, src, len);
> + MEMCPY_AND_ZERO_SRC(u32, dst, src, len);
> + MEMCPY_AND_ZERO_SRC(u16, dst, src, len);
> + }
> + MEMCPY_AND_ZERO_SRC(u8, dst, src, len);
> +}
> +
> +/**
> + * __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
> + /*
> + * Set @state->pos to beyond the end of the batch, so that the batch is refilled
> + * using the new key.
> + */
> + state->pos = sizeof(state->batch);
> + }
> +
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.
> +
> + /* 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.
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 :)
Thanks,
tglx
