Messages in this thread | | | From | Thomas Gleixner <> | Subject | Re: [PATCH v10 3/4] random: introduce generic vDSO getrandom() implementation | Date | Tue, 29 Nov 2022 23:42:16 +0100 |
| |
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
| |