On 2016-02-04 at 19:20 ron minnich <[email protected]> wrote:
> PTAL. I did not yet delete the stuff support TCP, I can do that in a
> CL that will change tcp and random.
> 
> I think I fixed all the issues.

Thanks!

Merged to master at 9a512f392c91..87436bb302ff (from, to]

You can see the entire diff with 'git diff' or at
https://github.com/brho/akaros/compare/9a512f392c91...87436bb302ff

Barret


> 
> On Thu, Feb 4, 2016 at 8:01 AM Barret Rhoden <[email protected]>
> wrote:
> 
> > Hi -
> >
> > Looks nice, minor comments below.
> >
> > On 2016-02-04 at 04:01 ron minnich <[email protected]> wrote:
> > > This is a series starting with import the code, formatting it,
> > > getting it to build, and changing the device.
> > >
> > >
> > > The following changes since commit
> > > 47341e4900dead535ab786d887607f8f8e2ea194:
> > >
> > >   New and easy strace framework. (2016-02-03 18:31:54 -0500)
> > >
> > > are available in the git repository at:
> > >
> > >   [email protected]:rminnich/akaros newrandom
> >
> > > From 3666b7600e0a986e64a7f961674d32aa2fae1cb0 Mon Sep 17 00:00:00
> > > 2001 From: "Ronald G. Minnich" <[email protected]>
> > > Date: Wed, 3 Feb 2016 19:09:02 -0800
> > > Subject: Add a random device; remove old genrandom junk; remove
> > > random
> > from
> > >  #cons
> >
> > > diff --git a/kern/drivers/dev/random.c b/kern/drivers/dev/random.c
> > > new file mode 100644
> > > index 000000000000..230b7a57495d
> > > --- /dev/null
> > > +++ b/kern/drivers/dev/random.c
> > > @@ -0,0 +1,241 @@
> > > +/*
> > > + * copyright here.
> > > + */
> >
> > This needs fixed.  AFAIK, urandomread looks like it's from another
> > project and the other random bits at the end are from the original
> > random.c file.  Though I think we can delete those.
> >
> > > +#include <vfs.h>
> > > +#include <kfs.h>
> > > +#include <slab.h>
> > > +#include <kmalloc.h>
> > > +#include <kref.h>
> > > +#include <string.h>
> > > +#include <stdio.h>
> > > +#include <assert.h>
> > > +#include <error.h>
> > > +#include <cpio.h>
> > > +#include <pmap.h>
> > > +#include <smp.h>
> > > +#include <ip.h>
> > > +#include <random/fortuna.h>
> > > +
> > > +static qlock_t rl;
> > > +static spinlock_t tap_lock;
> > > +static int len;
> > > +
> > > +/* While the rule *used to be* that urandom did not block, this
> > > rng is
> > fast enough
> > > + * that we don't think we will need that distinction. I'm not
> > > sure
> > anything will
> > > + * every really block. But some things may want a select-like
> > > interface
> > anyway. */
> >
> > checkpatch complained about this:
> >
> > WARNING: line over 80 characters
> > #156: FILE: kern/drivers/dev/random.c:24:
> > +/* While the rule *used to be* that urandom did not block, this
> > rng is fast enough
> >
> > WARNING: line over 80 characters
> > #158: FILE: kern/drivers/dev/random.c:26:
> > + * every really block. But some things may want a select-like
> > interface anyway. */
> >
> > ERROR: trailing whitespace
> > #163: FILE: kern/drivers/dev/random.c:31:
> > + * hardware entropy source. $
> >
> >
> > > +struct fdtap_slist data_tap;
> > > +
> > > +/*
> > > + * Add entropy. This is not currently used but we might want to
> > > hook it
> > into a
> > > + * hardware entropy source.
> > > + */
> > > +void random_add(void *xp)
> > > +{
> > > +     ERRSTACK(1);
> > > +
> > > +     if (waserror()) {
> > > +             qunlock(&rl);
> > > +             nexterror();
> > > +     }
> > > +
> > > +     qlock(&rl);
> >
> > For these funcs, do we want to qlock before the waserror?  With the
> > understanding that waserror undoes things done before it (in the
> > simpler cases)?  waserror() can handle either style, so I'm fine
> > either way.
> >
> > > +     fortuna_add_entropy(xp, sizeof(xp));
> > > +     qunlock(&rl);
> > > +
> > > +     poperror();
> > > +}
> > > +
> > > +/*
> > > + *  consume random bytes
> > > + */
> > > +static uint32_t _randomread(void *xp, uint32_t n)
> > > +{
> > > +     ERRSTACK(1);
> > > +
> > > +     if (waserror()) {
> > > +             qunlock(&rl);
> > > +             nexterror();
> > > +     }
> > > +
> > > +     qlock(&rl);
> > > +     fortuna_get_bytes(n, xp);
> > > +     qunlock(&rl);
> > > +
> > > +     poperror();
> > > +
> > > +     return n;
> > > +}
> > > +
> > > +/**
> > > + * Fast random generator
> > > + **/
> > > +uint32_t urandomread(void *xp, uint32_t n)
> > > +{
> > > +     ERRSTACK(1);
> > > +     uint64_t seed[16];
> > > +     uint8_t *e, *p;
> > > +     uint32_t x = 0;
> > > +     uint64_t s0;
> > > +     uint64_t s1;
> > > +
> > > +     if (waserror())
> > > +             nexterror();
> >
> > I think we don't want a waserror() at all here (and thus the
> > poperror below).  If all we do is nexterror, we didn't really do
> > anything.
> >
> > > +
> > > +     // The initial seed is from a good random pool.
> > > +     _randomread(seed, sizeof(seed));
> > > +     p = xp;
> > > +     for (e = p + n; p < e;) {
> > > +             s0 = seed[x];
> > > +             s1 = seed[x = (x + 1) & 15];
> > > +             s1 ^= s1 << 31;
> > > +             s1 ^= s1 >> 11;
> > > +             s0 ^= s0 >> 30;
> > > +             *p++ = (seed[x] = s0 ^ s1) * 1181783497276652981LL;
> > > +     }
> > > +     poperror();
> > > +     return n;
> > > +}
> > > +
> >
> > > +static struct walkqid *randomwalk(struct chan *c, struct chan
> > > *nc, char
> > **name,
> > > +                                                             int
> > > nname) +{
> > > +     return devwalk(c, nc, name, nname, randomdir,
> > ARRAY_SIZE(randomdir), devgen);
> >
> > Checkpatch complaint:
> >
> > WARNING: line over 80 characters
> > #270: FILE: kern/drivers/dev/random.c:138:
> > +       return devwalk(c, nc, name, nname, randomdir,
> > ARRAY_SIZE(randomdir),
> > devgen);
> >
> >
> > > +static long randomread(struct chan *c, void *va, long n, int64_t
> > ignored)
> > > +{
> > > +     switch (c->qid.path) {
> > > +             case Qdir:
> > > +                     return devdirread(c, va, n, randomdir,
> > > +                                       ARRAY_SIZE(randomdir),
> > > devgen);
> > > +             case Qrandom:
> > > +                     return _randomread(va, n);
> > > +             case Qurandom:
> > > +                     return _randomread(va, n);
> >
> > Should Qurandom be calling urandomread instead of randomread?
> >
> > > +             default:
> > > +                     panic("randomread: qid %d is impossible",
> > c->qid.path);
> > > +     }
> > > +     return -1;      /* not reached */
> > > +}
> >
> > > +/* This is something used by the TCP stack.
> > > + * I have no idea of why these numbers were chosen. */
> > > +static uint32_t randn;
> > > +
> > > +static void seedrand(void)
> > > +{
> > > +     ERRSTACK(2);
> > > +     if (!waserror()) {
> > > +             _randomread((void *)&randn, sizeof(randn));
> > > +             poperror();
> > > +     }
> > > +}
> > > +
> > > +int nrand(int n)
> > > +{
> > > +     if (randn == 0)
> > > +             seedrand();
> > > +     randn = randn * 1103515245 + 12345 + read_tsc();
> >
> > This is nasty crap.  I think I hacked in that read_tsc() to give us
> > at least some variation in a busted RNG.
> >
> > I think nrand() is used as a way to get 16 bits of a random
> > number.  We can probably change tcp to use urandom or something.
> > It looks like TCP is using nrand for SEQ numbers, so maybe we just
> > give them a number.
> >
> > > +     return (randn >> 16) % n;
> > > +}
> > > +
> > > +int rand(void)
> > > +{
> > > +     nrand(1);
> > > +     return randn;
> > > +}
> >
> > No one uses this other than a cheap test; I think we can remove all
> > of this crap.
> >
> > > diff --git a/kern/lib/random/fortuna.c b/kern/lib/random/fortuna.c
> > > index 868d59b55618..90c86cff0d04 100644
> > > --- a/kern/lib/random/fortuna.c
> > > +++ b/kern/lib/random/fortuna.c
> > > @@ -31,9 +31,12 @@
> > >
> > >  #include <arch/arch.h>
> > >  #include <time.h>
> > > +
> > > +#include <random/fortuna.h>
> > >  #include <random/rijndael.h>
> > >  #include <random/sha2.h>
> > >
> > > +
> > >  /*
> > >   * Why Fortuna-like: There does not seem to be any definitive
> > > reference
> > >   * on Fortuna in the net.  Instead this implementation is based
> > > on
> >
> > I think this diff should have been squashed into d111786e69fb ("Get
> > the basic random number generator functions to compile").  It's a
> > good git exercise to do it. (I can help).
> >
> > --
> > You received this message because you are subscribed to the Google
> > Groups "Akaros" group.
> > To unsubscribe from this group and stop receiving emails from it,
> > send an email to [email protected].
> > To post to this group, send email to [email protected].
> > For more options, visit https://groups.google.com/d/optout.
> >
> 

-- 
You received this message because you are subscribed to the Google Groups 
"Akaros" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
To post to this group, send email to [email protected].
For more options, visit https://groups.google.com/d/optout.

Reply via email to