[resending with correct recipients; sorry for duplication]
On Mon, Jul 13, 2020 at 3:53 PM Petr Štetiar <yn...@true.cz> wrote:
>
> nwfila...@gmail.com <nwfila...@gmail.com> [2020-07-13 10:08:17]:
>
> > From: Nathaniel Wesley Filardo <nwfila...@gmail.com>
> >
> > This allows us to attach a hwrng, for example.
> >
> > At most one sample will be taken every time we also add entropy via the
> > jitter mechanism. We won't try reading if the stream hasn't indicated
> > its readiness, and we'll go back to waiting if ever the stream produces
> > 0 bytes or an error on read.
> >
> > Signed-off-by: Nathaniel Wesley Filardo <nwfila...@gmail.com>
> > ---
> > urngd.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 70 insertions(+), 4 deletions(-)
> >
> > diff --git a/urngd.c b/urngd.c
> > index 31181f0..d6c48f8 100644
> > --- a/urngd.c
> > +++ b/urngd.c
> > @@ -61,6 +61,7 @@ unsigned int debug;
> >
> > struct urngd {
> > struct uloop_fd rnd_fd;
> > + struct uloop_fd src_fd;
> > struct rand_data *ec;
> > };
> >
> > @@ -88,7 +89,7 @@ static size_t write_entropy(struct urngd *u, struct
> > rand_pool_info *rpi)
> > return ret;
> > }
> >
> > -static size_t gather_entropy(struct urngd *u)
> > +static size_t gather_jitter_entropy(struct urngd *u)
> > {
> > ssize_t ent;
> > size_t ret = 0;
> > @@ -110,12 +111,48 @@ static size_t gather_entropy(struct urngd *u)
> > return ret;
> > }
> >
> > +static size_t gather_src_entropy(struct urngd *u) {
> > + static const size_t src_bytes = 1024;
> > + struct rand_pool_info *rpi = alloca(sizeof(*rpi) + src_bytes);
> > + ssize_t ent;
> > + size_t ret;
> > +
> > + if ((u->src_fd.fd < 0) || (u->src_fd.registered)) {
> > + /* No source or source still waiting for available bytes */
> > + return 0;
> > + }
> > +
> > + ent = read(u->src_fd.fd, (char *)&rpi->buf[0], src_bytes);
> > + if (ent > 0) {
> > + /* Read some bytes from the source; stir those in, too */
> > + rpi->buf_size = ent;
> > + rpi->entropy_count = 8 * ent;
> > + ret = write_entropy(u, rpi);
> > + } else {
> > + /* No luck this time around */
> > + ret = 0;
> > +
> > + /* Go back to waiting for the source to be ready */
> > + uloop_fd_add(&u->src_fd, ULOOP_READ);
>
> in init you check the return value, but now don't care? :-)
Yes; the intent of checking in init was just to log a warning. Having done so,
there's no point in checking again.
> Anyway, I think,
> that using uloop for this readiness purpose is overkill, wouldn't just read()
> suffice here? Too much code with no added value.
Yes, calling read() would be semantically equivalent, but calling read() is way,
way more expensive than a function and an alloca() that you ask me to avoid
below. I think it's not that much code to avoid the expense of a system call
for devices that may only occasionally be able to feed entropy to the system.
> Too much comments as well, I think, that it's pretty obvious what it is doing.
> If not, then please move that code into separate properly (self-documenting)
> named function.
OK.
> I would rather exchange those comments with handling of ent < 0 case, probably
> for the start DEBUG() out errors other then EAGAIN/EINTR etc.
Sorry, I don't think I understand that.
> > + }
> > +
> > + memset_secure(&rpi->buf, 0, ent);
> > +
> > + return ret;
> > +}
> > +
> > static void low_entropy_cb(struct uloop_fd *ufd, unsigned int events)
> > {
> > struct urngd *u = container_of(ufd, struct urngd, rnd_fd);
> >
> > DEBUG(2, DEV_RANDOM " signals low entropy\n");
> > - gather_entropy(u);
> > + gather_jitter_entropy(u);
> > + gather_src_entropy(u);
>
> I would rather do:
>
> static inline bool entropy_device_available(struct urngd *u)
> {
> return u->fd >= 0;
> }
>
> if (entropy_device_available(u))
> gather_device_entropy(u);
OK.
> in order to avoid wasting cycles with alloca() call and reuse
> `entropy_device_available` in other places to make code more readable.
That's fine, but the cost of a function call and alloca() is tens of cycles,
since the alloca is just a subtraction of the stack pointer which a good
optimizing compiler can fuse into the allocation of local variables,
while a read() is more likely to be thousands.
> That being said I find that `source` name ambiguous and would prefer `device`
> instead as it's actually RNG device behind that FD (perhaps `file` would
> work as well)
>
> Naming is hard :)
Of "device" and "file", I'd prefer "file" since "everything's a file on UNIX"
and "-d" is already taken (thus "-f").
> > +}
> > +
> > +static void src_ready_cb(struct uloop_fd *ufd, unsigned int events)
> > +{
> > + uloop_fd_delete(ufd);
> > }
> >
> > static void urngd_done(struct urngd *u)
> > @@ -129,6 +166,11 @@ static void urngd_done(struct urngd *u)
> > close(u->rnd_fd.fd);
> > u->rnd_fd.fd = 0;
> > }
> > +
> > + if (u->src_fd.fd >= 0) {
> > + close(u->src_fd.fd);
> > + u->src_fd.fd = -1;
> > + }
> > }
> >
> > static bool urngd_init(struct urngd *u)
> > @@ -154,6 +196,18 @@ static bool urngd_init(struct urngd *u)
> >
> > uloop_fd_add(&u->rnd_fd, ULOOP_WRITE);
> >
> > + if (u->src_fd.fd >= 0) {
> > + int ret;
> > +
> > + u->src_fd.cb = src_ready_cb;
> > + ret = uloop_fd_add(&u->src_fd, ULOOP_READ);
> > + if (ret == -1 && errno == EPERM) {
> > + LOG("Source (-f) does not support polling;"
> > + " assuming that's OK.");
> > + u->src_fd.registered = false;
> > + }
> > + }
> > +
> > return true;
> > }
> >
> > @@ -164,6 +218,7 @@ static int usage(const char *prog)
> > #ifdef URNGD_DEBUG
> > " -d <level> Enable debug messages\n"
> > #endif
> > + " -f <file> Source entropy from <file>\n"
> > " -S Print messages to stdout\n"
> > "\n", prog);
> > return 1;
> > @@ -182,13 +237,24 @@ int main(int argc, char **argv)
> > }
> > #endif
> >
> > - while ((ch = getopt(argc, argv, "d:S")) != -1) {
> > + urngd_service.src_fd.fd = -1;
> > +
> > + while ((ch = getopt(argc, argv, "d:f:S")) != -1) {
> > switch (ch) {
> > #ifdef URNGD_DEBUG
> > case 'd':
> > debug = atoi(optarg);
> > break;
> > #endif
> > + case 'f':
> > + urngd_service.src_fd.fd =
> > + open(optarg, O_RDONLY | O_NONBLOCK);
> > + if (urngd_service.src_fd.fd < 0) {
> > + ERROR("%s open failed: %s\n",
> > + optarg, strerror(errno));
> > + return -1;
>
> Jitter entropy is probably still usable, still better then no entropy, right?
OK, I'll make it log a warning and not be fatal.
> That block is not oneliner so move that into separate function.
OK.
> > + }
> > + break;
> > case 'S':
> > ulog_channels = ULOG_STDIO;
> > break;
> > @@ -207,7 +273,7 @@ int main(int argc, char **argv)
> >
> > LOG("v%s started.\n", URNGD_VERSION);
> >
> > - gather_entropy(&urngd_service);
> > + gather_jitter_entropy(&urngd_service);
>
> Why just jitter source? I would like to get entropy from device as well if
> available.
jitter is guaranteed to be available, while the device hasn't been asked.
I don't think it much matters: if we're low on entropy we'll ask the device
almost immediately in the event loop anyway.
But if you'd prefer, I can call gather_file_entropy() at startup and use
that to arm the uloop_fd.
Cheers,
--nwf;
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
