Message ID | 1406837476-13776-1-git-send-email-dborkman@redhat.com |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
From: Daniel Borkmann <dborkman@redhat.com> Date: Thu, 31 Jul 2014 22:11:16 +0200 > - for (j = 0; j < sizeof(u32); j++) { > - p[i + j] = random; > - random >>= BITS_PER_BYTE; > - } > + while (bytes > sizeof(u32)) { > + put_unaligned(prandom_u32_state(state), (u32 *) ptr); > + ptr += sizeof(u32); > + bytes -= sizeof(u32); > } > - if (i < bytes) { > - u32 random = prandom_u32_state(state); > > - for (; i < bytes; i++) { > - p[i] = random; > - random >>= BITS_PER_BYTE; > - } > + if (bytes > 0) { > + u32 rem = prandom_u32_state(state); > + do { > + *ptr++ = (u8) rem; > + bytes--; > + rem >>= BITS_PER_BYTE; > + } while (bytes > 0); > } This conversion to put_unaligned() is not an equivalent depending upon the endianness of the cpu. And this means the random value gets distributed differently into full words than it will into trailing bytes. Let's just not mess around with this, ok? Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[ Cc'ing Akinobu, 6582c665d6b882 ("prandom: introduce prandom_bytes() and prandom_bytes_state()") ] On 08/01/2014 07:28 AM, David Miller wrote: > From: Daniel Borkmann <dborkman@redhat.com> > Date: Thu, 31 Jul 2014 22:11:16 +0200 > >> - for (j = 0; j < sizeof(u32); j++) { >> - p[i + j] = random; >> - random >>= BITS_PER_BYTE; >> - } >> + while (bytes > sizeof(u32)) { [ Should have been >= here, missed that earlier. ] >> + put_unaligned(prandom_u32_state(state), (u32 *) ptr); >> + ptr += sizeof(u32); >> + bytes -= sizeof(u32); >> } >> - if (i < bytes) { >> - u32 random = prandom_u32_state(state); >> >> - for (; i < bytes; i++) { >> - p[i] = random; >> - random >>= BITS_PER_BYTE; >> - } >> + if (bytes > 0) { >> + u32 rem = prandom_u32_state(state); >> + do { >> + *ptr++ = (u8) rem; >> + bytes--; >> + rem >>= BITS_PER_BYTE; >> + } while (bytes > 0); >> } > > This conversion to put_unaligned() is not an equivalent depending > upon the endianness of the cpu. > > And this means the random value gets distributed differently into > full words than it will into trailing bytes. > > Let's just not mess around with this, ok? Sorry for the late answer, Dave. So for the case of callers of prandom_bytes() which internally invoke prandom_bytes_state(), it doesn't matter at all since, well, they expect the array to be filled randomly and don't have any control of the internal state anyway (that's also why we have periodic reseeding there, etc), so they really don't care. Now for the direct callers of prandom_bytes_state(), which are solely located in drivers/mtd/tests/{oobtest.c, pagetest.c, subpagetest.c}: These tests basically fill a test write-vector through prandom_bytes_state() with an a-priori defined seed each time and write that to a MTD device. Later on, they set up a read-vector and read back that blocks from the device. So in the verification phase, the write-vector is being re-setup [ so same seed and prandom_bytes_state() called ], and then memcmp()'ed against the read-vector to check if the data is the same. Akinobu and I also tested this patch with the above fix included, and it runs through the 3 relevant test cases _without_ any errors on the nandsim device (simulator for MTD devs): # modprobe nandsim first_id_byte=0x20 second_id_byte=0xac \ third_id_byte=0x00 fourth_id_byte=0x15 # modprobe mtd_oobtest dev=0 # modprobe mtd_pagetest dev=0 # modprobe mtd_subpagetest dev=0 [ ... 'finished with 0 errors' each in klog ] We also don't have any users depending directly on a particular result of the PRNG (except the PRNG self-test itself), and that's just fine as it e.g. allowed us easily to do things like upgrading from taus88 to taus113. Thanks, Daniel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Daniel Borkmann <dborkman@redhat.com> Date: Mon, 04 Aug 2014 11:48:33 +0200 > Akinobu and I also tested this patch with the above fix included, > and it runs through the 3 relevant test cases _without_ any errors > on the nandsim device (simulator for MTD devs): On one endianness only perhaps? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/04/2014 08:41 PM, David Miller wrote: > From: Daniel Borkmann <dborkman@redhat.com> > Date: Mon, 04 Aug 2014 11:48:33 +0200 > >> Akinobu and I also tested this patch with the above fix included, >> and it runs through the 3 relevant test cases _without_ any errors >> on the nandsim device (simulator for MTD devs): > > On one endianness only perhaps? On x86_64, ppc64, ARM (i.MX28, i.MX53 and i.MX6). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/05/2014 05:16 PM, Daniel Borkmann wrote: > On 08/04/2014 08:41 PM, David Miller wrote: >> From: Daniel Borkmann <dborkman@redhat.com> >> Date: Mon, 04 Aug 2014 11:48:33 +0200 >> >>> Akinobu and I also tested this patch with the above fix included, >>> and it runs through the 3 relevant test cases _without_ any errors >>> on the nandsim device (simulator for MTD devs): >> >> On one endianness only perhaps? > > On x86_64, ppc64, ARM (i.MX28, i.MX53 and i.MX6). Not really urgent, so if there are no further objections, I'd respin it with the code fix included, proper Tested-by tags from MTD people, and updated commit message when net-next reopens again. Thanks, Daniel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Daniel Borkmann <dborkman@redhat.com> Date: Wed, 06 Aug 2014 12:12:33 +0200 > On 08/05/2014 05:16 PM, Daniel Borkmann wrote: >> On 08/04/2014 08:41 PM, David Miller wrote: >>> From: Daniel Borkmann <dborkman@redhat.com> >>> Date: Mon, 04 Aug 2014 11:48:33 +0200 >>> >>>> Akinobu and I also tested this patch with the above fix included, >>>> and it runs through the 3 relevant test cases _without_ any errors >>>> on the nandsim device (simulator for MTD devs): >>> >>> On one endianness only perhaps? >> >> On x86_64, ppc64, ARM (i.MX28, i.MX53 and i.MX6). > > Not really urgent, so if there are no further objections, > I'd respin it with the code fix included, proper Tested-by > tags from MTD people, and updated commit message when > net-next reopens again. Ok, please do. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/random.h b/include/linux/random.h index 57fbbff..b05856e 100644 --- a/include/linux/random.h +++ b/include/linux/random.h @@ -26,7 +26,7 @@ unsigned int get_random_int(void); unsigned long randomize_range(unsigned long start, unsigned long end, unsigned long len); u32 prandom_u32(void); -void prandom_bytes(void *buf, int nbytes); +void prandom_bytes(void *buf, size_t nbytes); void prandom_seed(u32 seed); void prandom_reseed_late(void); @@ -35,7 +35,7 @@ struct rnd_state { }; u32 prandom_u32_state(struct rnd_state *state); -void prandom_bytes_state(struct rnd_state *state, void *buf, int nbytes); +void prandom_bytes_state(struct rnd_state *state, void *buf, size_t nbytes); /** * prandom_u32_max - returns a pseudo-random number in interval [0, ep_ro) diff --git a/lib/random32.c b/lib/random32.c index c9b6bf3..ce29432 100644 --- a/lib/random32.c +++ b/lib/random32.c @@ -37,6 +37,7 @@ #include <linux/jiffies.h> #include <linux/random.h> #include <linux/sched.h> +#include <asm/unaligned.h> #ifdef CONFIG_RANDOM32_SELFTEST static void __init prandom_state_selftest(void); @@ -96,27 +97,23 @@ EXPORT_SYMBOL(prandom_u32); * This is used for pseudo-randomness with no outside seeding. * For more random results, use prandom_bytes(). */ -void prandom_bytes_state(struct rnd_state *state, void *buf, int bytes) +void prandom_bytes_state(struct rnd_state *state, void *buf, size_t bytes) { - unsigned char *p = buf; - int i; - - for (i = 0; i < round_down(bytes, sizeof(u32)); i += sizeof(u32)) { - u32 random = prandom_u32_state(state); - int j; + u8 *ptr = buf; - for (j = 0; j < sizeof(u32); j++) { - p[i + j] = random; - random >>= BITS_PER_BYTE; - } + while (bytes > sizeof(u32)) { + put_unaligned(prandom_u32_state(state), (u32 *) ptr); + ptr += sizeof(u32); + bytes -= sizeof(u32); } - if (i < bytes) { - u32 random = prandom_u32_state(state); - for (; i < bytes; i++) { - p[i] = random; - random >>= BITS_PER_BYTE; - } + if (bytes > 0) { + u32 rem = prandom_u32_state(state); + do { + *ptr++ = (u8) rem; + bytes--; + rem >>= BITS_PER_BYTE; + } while (bytes > 0); } } EXPORT_SYMBOL(prandom_bytes_state); @@ -126,7 +123,7 @@ EXPORT_SYMBOL(prandom_bytes_state); * @buf: where to copy the pseudo-random bytes to * @bytes: the requested number of bytes */ -void prandom_bytes(void *buf, int bytes) +void prandom_bytes(void *buf, size_t bytes) { struct rnd_state *state = &get_cpu_var(net_rand_state); @@ -137,7 +134,7 @@ EXPORT_SYMBOL(prandom_bytes); static void prandom_warmup(struct rnd_state *state) { - /* Calling RNG ten times to satify recurrence condition */ + /* Calling RNG ten times to satisfy recurrence condition */ prandom_u32_state(state); prandom_u32_state(state); prandom_u32_state(state); @@ -152,7 +149,7 @@ static void prandom_warmup(struct rnd_state *state) static u32 __extract_hwseed(void) { - u32 val = 0; + unsigned int val = 0; (void)(arch_get_random_seed_int(&val) || arch_get_random_int(&val)); @@ -228,7 +225,7 @@ static void __prandom_timer(unsigned long dontcare) prandom_seed(entropy); /* reseed every ~60 seconds, in [40 .. 80) interval with slack */ - expires = 40 + (prandom_u32() % 40); + expires = 40 + prandom_u32_max(40); seed_timer.expires = jiffies + msecs_to_jiffies(expires * MSEC_PER_SEC); add_timer(&seed_timer);
This patch addresses a couple of minor items, mostly addressing prandom_bytes(): 1) prandom_bytes{,_state}() should use size_t for length arguments, 2) We can use put_unaligned() when filling the array instead of open coding it [ perhaps some archs will further benefit from their own arch specific implementation when GCC cannot make up for it ], 3) Fix a typo, 4) Better use unsigned int as type for getting the arch seed, 5) Make use of prandom_u32_max() for timer slack. Signed-off-by: Daniel Borkmann <dborkman@redhat.com> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org> --- include/linux/random.h | 4 ++-- lib/random32.c | 39 ++++++++++++++++++--------------------- 2 files changed, 20 insertions(+), 23 deletions(-)