* virtio-rng only returns zeros with CONFIG_HW_RANDOM=m
@ 2013-02-24 23:11 Aurelien Jarno
2013-02-27 0:13 ` Rusty Russell
2013-02-27 1:26 ` Rusty Russell
0 siblings, 2 replies; 12+ messages in thread
From: Aurelien Jarno @ 2013-02-24 23:11 UTC (permalink / raw)
To: kvm; +Cc: Ian Molton, Matt Mackall, Rusty Russell, Herbert Xu
Hi,
I have noticed that virtio-rng only returns zero for kernels >= 2.6.33
built with CONFIG_HW_RANDOM=m. This is a bit much too predictable for a
random generator ;-).
The reason for that is virtio expects guest real addresses, while
rng_core.ko (ie when built as a module) is passing a vmalloced buffer
to the virtio-rng read function, declared as such:
static u8 rng_buffer[SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES]
__cacheline_aligned;
This is basically the same issue than the following one:
https://lists.linux-foundation.org/pipermail/virtualization/2008-May/010946.html
but introduced in a more subtle way in this commit:
commit bb347d98079a547e80bd4722dee1de61e4dca0e8
Author: Ian Molton <ian.molton@collabora.co.uk>
Date: Tue Dec 1 15:26:33 2009 +0800
hwrng: virtio-rng - Convert to new API
This patch converts virtio-rng to the new hw_rng API.
In the process it fixes a previously untriggered buffering bug where the
buffer is not drained correctly if it has a non-multiple-of-4 length.
Performance has improved under qemu-kvm testing also.
Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>
Acked-by: Matt Mackall <mpm@selenic.com>
Acked-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
I basically see three possible way of fixing that:
- prevent rng_core to be built as a module;
- use an intermediary kmalloced buffer in virtio-rng passed to virtio
functions, followed by a memcpy to get the data in the rng core
buffer;
- use a kmalloc buffer in rng_core instead of vmalloc one.
What would be best way to fix that? Did I miss another way?
Thanks,
Aurelien
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m 2013-02-24 23:11 virtio-rng only returns zeros with CONFIG_HW_RANDOM=m Aurelien Jarno @ 2013-02-27 0:13 ` Rusty Russell 2013-02-27 3:40 ` Ben Hutchings ` (2 more replies) 2013-02-27 1:26 ` Rusty Russell 1 sibling, 3 replies; 12+ messages in thread From: Rusty Russell @ 2013-02-27 0:13 UTC (permalink / raw) To: Aurelien Jarno, kvm Cc: Ian Molton, Matt Mackall, Herbert Xu, Ben Hutchings, Jens Axboe Aurelien Jarno <aurelien@aurel32.net> writes: > Hi, > > I have noticed that virtio-rng only returns zero for kernels >= 2.6.33 > built with CONFIG_HW_RANDOM=m. This is a bit much too predictable for a > random generator ;-). Wow. Fortunately, all of SLES, RHEL, Ubuntu or Fedora set CONFIG_HW_RANDOM=y. What do they know that we don't? Oops, looks like Debian testing: config-3.2.0-4-amd64:CONFIG_HW_RANDOM=m > The reason for that is virtio expects guest real addresses, while > rng_core.ko (ie when built as a module) is passing a vmalloced buffer > to the virtio-rng read function, declared as such: > > static u8 rng_buffer[SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES] > __cacheline_aligned; Yuck... It would be nice if this has oopsed. Jens, what about this patch? Cheers, Rusty. Subject: scatterlist: sg_set_buf() argument must be in linear mapping. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h index 4bd6c06..9365375 100644 --- a/include/linux/scatterlist.h +++ b/include/linux/scatterlist.h @@ -111,6 +111,9 @@ static inline struct page *sg_page(struct scatterlist *sg) static inline void sg_set_buf(struct scatterlist *sg, const void *buf, unsigned int buflen) { +#ifdef CONFIG_DEBUG_SG + BUG_ON(!virt_addr_valid(buf)); +#endif sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf)); } ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m 2013-02-27 0:13 ` Rusty Russell @ 2013-02-27 3:40 ` Ben Hutchings 2013-02-27 10:33 ` Aurelien Jarno 2013-02-27 13:22 ` Jens Axboe 2 siblings, 0 replies; 12+ messages in thread From: Ben Hutchings @ 2013-02-27 3:40 UTC (permalink / raw) To: Rusty Russell Cc: Aurelien Jarno, kvm, Ian Molton, Matt Mackall, Herbert Xu, Jens Axboe [-- Attachment #1: Type: text/plain, Size: 750 bytes --] On Wed, 2013-02-27 at 10:43 +1030, Rusty Russell wrote: > Aurelien Jarno <aurelien@aurel32.net> writes: > > Hi, > > > > I have noticed that virtio-rng only returns zero for kernels >= 2.6.33 > > built with CONFIG_HW_RANDOM=m. This is a bit much too predictable for a > > random generator ;-). > > Wow. Fortunately, all of SLES, RHEL, Ubuntu or Fedora set > CONFIG_HW_RANDOM=y. What do they know that we don't? > > Oops, looks like Debian testing: config-3.2.0-4-amd64:CONFIG_HW_RANDOM=m [...] *Groan*. I'll change the configuration for now, but trust this will be fixed in virtio-rng later. Thanks for letting me know. Ben. -- Ben Hutchings Never attribute to conspiracy what can adequately be explained by stupidity. [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m 2013-02-27 0:13 ` Rusty Russell 2013-02-27 3:40 ` Ben Hutchings @ 2013-02-27 10:33 ` Aurelien Jarno 2013-02-27 13:22 ` Jens Axboe 2 siblings, 0 replies; 12+ messages in thread From: Aurelien Jarno @ 2013-02-27 10:33 UTC (permalink / raw) To: Rusty Russell Cc: kvm, Ian Molton, Matt Mackall, Herbert Xu, Ben Hutchings, Jens Axboe On Wed, Feb 27, 2013 at 10:43:37AM +1030, Rusty Russell wrote: > Aurelien Jarno <aurelien@aurel32.net> writes: > > Hi, > > > > I have noticed that virtio-rng only returns zero for kernels >= 2.6.33 > > built with CONFIG_HW_RANDOM=m. This is a bit much too predictable for a > > random generator ;-). > > Wow. Fortunately, all of SLES, RHEL, Ubuntu or Fedora set > CONFIG_HW_RANDOM=y. What do they know that we don't? > > Oops, looks like Debian testing: config-3.2.0-4-amd64:CONFIG_HW_RANDOM=m > > > The reason for that is virtio expects guest real addresses, while > > rng_core.ko (ie when built as a module) is passing a vmalloced buffer > > to the virtio-rng read function, declared as such: > > > > static u8 rng_buffer[SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES] > > __cacheline_aligned; > > Yuck... It would be nice if this has oopsed. Jens, what about this patch? > > Cheers, > Rusty. > > Subject: scatterlist: sg_set_buf() argument must be in linear mapping. > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h > index 4bd6c06..9365375 100644 > --- a/include/linux/scatterlist.h > +++ b/include/linux/scatterlist.h > @@ -111,6 +111,9 @@ static inline struct page *sg_page(struct scatterlist *sg) > static inline void sg_set_buf(struct scatterlist *sg, const void *buf, > unsigned int buflen) > { > +#ifdef CONFIG_DEBUG_SG > + BUG_ON(!virt_addr_valid(buf)); > +#endif > sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf)); > } > I confirm this patch catches the issue. Thanks. Tested-by: Aurelien Jarno <aurelien@aurel32.net> -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurelien@aurel32.net http://www.aurel32.net ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m 2013-02-27 0:13 ` Rusty Russell 2013-02-27 3:40 ` Ben Hutchings 2013-02-27 10:33 ` Aurelien Jarno @ 2013-02-27 13:22 ` Jens Axboe 2013-02-28 3:00 ` Rusty Russell 2013-05-30 1:07 ` Rusty Russell 2 siblings, 2 replies; 12+ messages in thread From: Jens Axboe @ 2013-02-27 13:22 UTC (permalink / raw) To: Rusty Russell Cc: Aurelien Jarno, kvm, Ian Molton, Matt Mackall, Herbert Xu, Ben Hutchings On Wed, Feb 27 2013, Rusty Russell wrote: > Aurelien Jarno <aurelien@aurel32.net> writes: > > Hi, > > > > I have noticed that virtio-rng only returns zero for kernels >= 2.6.33 > > built with CONFIG_HW_RANDOM=m. This is a bit much too predictable for a > > random generator ;-). > > Wow. Fortunately, all of SLES, RHEL, Ubuntu or Fedora set > CONFIG_HW_RANDOM=y. What do they know that we don't? > > Oops, looks like Debian testing: config-3.2.0-4-amd64:CONFIG_HW_RANDOM=m > > > The reason for that is virtio expects guest real addresses, while > > rng_core.ko (ie when built as a module) is passing a vmalloced buffer > > to the virtio-rng read function, declared as such: > > > > static u8 rng_buffer[SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES] > > __cacheline_aligned; > > Yuck... It would be nice if this has oopsed. Jens, what about this patch? > > Cheers, > Rusty. > > Subject: scatterlist: sg_set_buf() argument must be in linear mapping. > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h > index 4bd6c06..9365375 100644 > --- a/include/linux/scatterlist.h > +++ b/include/linux/scatterlist.h > @@ -111,6 +111,9 @@ static inline struct page *sg_page(struct scatterlist *sg) > static inline void sg_set_buf(struct scatterlist *sg, const void *buf, > unsigned int buflen) > { > +#ifdef CONFIG_DEBUG_SG > + BUG_ON(!virt_addr_valid(buf)); > +#endif > sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf)); > } Looks good to me, in lieu of being able to return an error. Want me to queue it up? -- Jens Axboe ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m 2013-02-27 13:22 ` Jens Axboe @ 2013-02-28 3:00 ` Rusty Russell 2013-05-30 1:07 ` Rusty Russell 1 sibling, 0 replies; 12+ messages in thread From: Rusty Russell @ 2013-02-28 3:00 UTC (permalink / raw) To: Jens Axboe Cc: Aurelien Jarno, kvm, Ian Molton, Matt Mackall, Herbert Xu, Ben Hutchings Jens Axboe <axboe@kernel.dk> writes: > On Wed, Feb 27 2013, Rusty Russell wrote: >> Subject: scatterlist: sg_set_buf() argument must be in linear mapping. >> >> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> >> >> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h >> index 4bd6c06..9365375 100644 >> --- a/include/linux/scatterlist.h >> +++ b/include/linux/scatterlist.h >> @@ -111,6 +111,9 @@ static inline struct page *sg_page(struct scatterlist *sg) >> static inline void sg_set_buf(struct scatterlist *sg, const void *buf, >> unsigned int buflen) >> { >> +#ifdef CONFIG_DEBUG_SG >> + BUG_ON(!virt_addr_valid(buf)); >> +#endif >> sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf)); >> } > > Looks good to me, in lieu of being able to return an error. Want me to > queue it up? Please... it'll catch me the next time I make the same mistake :) (Though the static-definitions-in-modules-on-most-archs is a pretty nasty corner case). Thanks, Rusty. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m 2013-02-27 13:22 ` Jens Axboe 2013-02-28 3:00 ` Rusty Russell @ 2013-05-30 1:07 ` Rusty Russell 2013-05-30 7:18 ` Jens Axboe 1 sibling, 1 reply; 12+ messages in thread From: Rusty Russell @ 2013-05-30 1:07 UTC (permalink / raw) To: Jens Axboe Cc: Aurelien Jarno, kvm, Ian Molton, Matt Mackall, Herbert Xu, Ben Hutchings Jens Axboe <axboe@kernel.dk> writes: > On Wed, Feb 27 2013, Rusty Russell wrote: >> Aurelien Jarno <aurelien@aurel32.net> writes: >> > Hi, >> > >> > I have noticed that virtio-rng only returns zero for kernels >= 2.6.33 >> > built with CONFIG_HW_RANDOM=m. This is a bit much too predictable for a >> > random generator ;-). >> >> Wow. Fortunately, all of SLES, RHEL, Ubuntu or Fedora set >> CONFIG_HW_RANDOM=y. What do they know that we don't? >> >> Oops, looks like Debian testing: config-3.2.0-4-amd64:CONFIG_HW_RANDOM=m >> >> > The reason for that is virtio expects guest real addresses, while >> > rng_core.ko (ie when built as a module) is passing a vmalloced buffer >> > to the virtio-rng read function, declared as such: >> > >> > static u8 rng_buffer[SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES] >> > __cacheline_aligned; >> >> Yuck... It would be nice if this has oopsed. Jens, what about this patch? >> >> Cheers, >> Rusty. >> >> Subject: scatterlist: sg_set_buf() argument must be in linear mapping. >> >> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> >> >> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h >> index 4bd6c06..9365375 100644 >> --- a/include/linux/scatterlist.h >> +++ b/include/linux/scatterlist.h >> @@ -111,6 +111,9 @@ static inline struct page *sg_page(struct scatterlist *sg) >> static inline void sg_set_buf(struct scatterlist *sg, const void *buf, >> unsigned int buflen) >> { >> +#ifdef CONFIG_DEBUG_SG >> + BUG_ON(!virt_addr_valid(buf)); >> +#endif >> sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf)); >> } > > Looks good to me, in lieu of being able to return an error. Want me to > queue it up? > > -- > Jens Axboe Ping? I haven't seen this go into Linus' tree... Subject: scatterlist: sg_set_buf() argument must be in linear mapping. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h index 4bd6c06..9365375 100644 --- a/include/linux/scatterlist.h +++ b/include/linux/scatterlist.h @@ -111,6 +111,9 @@ static inline struct page *sg_page(struct scatterlist *sg) static inline void sg_set_buf(struct scatterlist *sg, const void *buf, unsigned int buflen) { +#ifdef CONFIG_DEBUG_SG + BUG_ON(!virt_addr_valid(buf)); +#endif sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf)); } ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m 2013-05-30 1:07 ` Rusty Russell @ 2013-05-30 7:18 ` Jens Axboe 0 siblings, 0 replies; 12+ messages in thread From: Jens Axboe @ 2013-05-30 7:18 UTC (permalink / raw) To: Rusty Russell Cc: Aurelien Jarno, kvm, Ian Molton, Matt Mackall, Herbert Xu, Ben Hutchings On Thu, May 30 2013, Rusty Russell wrote: > Jens Axboe <axboe@kernel.dk> writes: > > On Wed, Feb 27 2013, Rusty Russell wrote: > >> Aurelien Jarno <aurelien@aurel32.net> writes: > >> > Hi, > >> > > >> > I have noticed that virtio-rng only returns zero for kernels >= 2.6.33 > >> > built with CONFIG_HW_RANDOM=m. This is a bit much too predictable for a > >> > random generator ;-). > >> > >> Wow. Fortunately, all of SLES, RHEL, Ubuntu or Fedora set > >> CONFIG_HW_RANDOM=y. What do they know that we don't? > >> > >> Oops, looks like Debian testing: config-3.2.0-4-amd64:CONFIG_HW_RANDOM=m > >> > >> > The reason for that is virtio expects guest real addresses, while > >> > rng_core.ko (ie when built as a module) is passing a vmalloced buffer > >> > to the virtio-rng read function, declared as such: > >> > > >> > static u8 rng_buffer[SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES] > >> > __cacheline_aligned; > >> > >> Yuck... It would be nice if this has oopsed. Jens, what about this patch? > >> > >> Cheers, > >> Rusty. > >> > >> Subject: scatterlist: sg_set_buf() argument must be in linear mapping. > >> > >> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > >> > >> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h > >> index 4bd6c06..9365375 100644 > >> --- a/include/linux/scatterlist.h > >> +++ b/include/linux/scatterlist.h > >> @@ -111,6 +111,9 @@ static inline struct page *sg_page(struct scatterlist *sg) > >> static inline void sg_set_buf(struct scatterlist *sg, const void *buf, > >> unsigned int buflen) > >> { > >> +#ifdef CONFIG_DEBUG_SG > >> + BUG_ON(!virt_addr_valid(buf)); > >> +#endif > >> sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf)); > >> } > > > > Looks good to me, in lieu of being able to return an error. Want me to > > queue it up? > > > > -- > > Jens Axboe > > Ping? I haven't seen this go into Linus' tree... I forget if I didn't get a reply, or whether it just got lost! Queued up now, at least. -- Jens Axboe ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m 2013-02-24 23:11 virtio-rng only returns zeros with CONFIG_HW_RANDOM=m Aurelien Jarno 2013-02-27 0:13 ` Rusty Russell @ 2013-02-27 1:26 ` Rusty Russell 2013-02-27 16:36 ` Aurelien Jarno 2013-03-10 8:26 ` Herbert Xu 1 sibling, 2 replies; 12+ messages in thread From: Rusty Russell @ 2013-02-27 1:26 UTC (permalink / raw) To: Aurelien Jarno, kvm; +Cc: Ian Molton, Matt Mackall, Herbert Xu Aurelien Jarno <aurelien@aurel32.net> writes: > Hi, > > I have noticed that virtio-rng only returns zero for kernels >= 2.6.33 > built with CONFIG_HW_RANDOM=m. This is a bit much too predictable for a > random generator ;-). > > The reason for that is virtio expects guest real addresses, while > rng_core.ko (ie when built as a module) is passing a vmalloced buffer > to the virtio-rng read function, declared as such: > > static u8 rng_buffer[SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES] > __cacheline_aligned; > > This is basically the same issue than the following one: > > https://lists.linux-foundation.org/pipermail/virtualization/2008-May/010946.html > > but introduced in a more subtle way in this commit: > > commit bb347d98079a547e80bd4722dee1de61e4dca0e8 > Author: Ian Molton <ian.molton@collabora.co.uk> > Date: Tue Dec 1 15:26:33 2009 +0800 OK, I looked at doing a kmalloc and copy in virtio_rng, but it's very inelegant (we don't know what size of buffer to allocate). No driver other than virtio_rng cares about this issue, but it's still far easier to fix in the core. How's this? Works here... Subject: hw_random: make buffer usable in scatterlist. virtio_rng feeds the randomness buffer handed by the core directly into the scatterlist, since commit bb347d98079a547e80bd4722dee1de61e4dca0e8. However, if CONFIG_HW_RANDOM=m, the static buffer isn't a linear address (at least on most archs). We could fix this in virtio_rng, but it's actually far easier to just do it in the core as virtio_rng would have to allocate a buffer every time (it doesn't know how much the core will want to read). Reported-by: Aurelien Jarno <aurelien@aurel32.net> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c index 1bafb40..69ae597 100644 --- a/drivers/char/hw_random/core.c +++ b/drivers/char/hw_random/core.c @@ -40,6 +40,7 @@ #include <linux/init.h> #include <linux/miscdevice.h> #include <linux/delay.h> +#include <linux/slab.h> #include <asm/uaccess.h> @@ -52,8 +53,12 @@ static struct hwrng *current_rng; static LIST_HEAD(rng_list); static DEFINE_MUTEX(rng_mutex); static int data_avail; -static u8 rng_buffer[SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES] - __cacheline_aligned; +static u8 *rng_buffer; + +static size_t rng_buffer_size(void) +{ + return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES; +} static inline int hwrng_init(struct hwrng *rng) { @@ -116,7 +121,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, if (!data_avail) { bytes_read = rng_get_data(current_rng, rng_buffer, - sizeof(rng_buffer), + rng_buffer_size(), !(filp->f_flags & O_NONBLOCK)); if (bytes_read < 0) { err = bytes_read; @@ -307,6 +312,14 @@ int hwrng_register(struct hwrng *rng) mutex_lock(&rng_mutex); + /* kmalloc makes this safe for virt_to_page() in virtio_rng.c */ + err = -ENOMEM; + if (!rng_buffer) { + rng_buffer = kmalloc(rng_buffer_size(), GFP_KERNEL); + if (!rng_buffer) + goto out_unlock; + } + /* Must not register two RNGs with the same name. */ err = -EEXIST; list_for_each_entry(tmp, &rng_list, list) { ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m 2013-02-27 1:26 ` Rusty Russell @ 2013-02-27 16:36 ` Aurelien Jarno 2013-02-28 3:04 ` Rusty Russell 2013-03-10 8:26 ` Herbert Xu 1 sibling, 1 reply; 12+ messages in thread From: Aurelien Jarno @ 2013-02-27 16:36 UTC (permalink / raw) To: Rusty Russell; +Cc: kvm, Ian Molton, Matt Mackall, Herbert Xu On Wed, Feb 27, 2013 at 11:56:55AM +1030, Rusty Russell wrote: > Aurelien Jarno <aurelien@aurel32.net> writes: > > Hi, > > > > I have noticed that virtio-rng only returns zero for kernels >= 2.6.33 > > built with CONFIG_HW_RANDOM=m. This is a bit much too predictable for a > > random generator ;-). > > > > The reason for that is virtio expects guest real addresses, while > > rng_core.ko (ie when built as a module) is passing a vmalloced buffer > > to the virtio-rng read function, declared as such: > > > > static u8 rng_buffer[SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES] > > __cacheline_aligned; > > > > This is basically the same issue than the following one: > > > > https://lists.linux-foundation.org/pipermail/virtualization/2008-May/010946.html > > > > but introduced in a more subtle way in this commit: > > > > commit bb347d98079a547e80bd4722dee1de61e4dca0e8 > > Author: Ian Molton <ian.molton@collabora.co.uk> > > Date: Tue Dec 1 15:26:33 2009 +0800 > > OK, I looked at doing a kmalloc and copy in virtio_rng, but it's very > inelegant (we don't know what size of buffer to allocate). On the other hand, the rng API allows to return less bytes than requested, so it's possible to have a fixed buffer size of for example 64 or 128 bytes. But I agree it's better to do that in rng core. > No driver other than virtio_rng cares about this issue, but it's still > far easier to fix in the core. > > How's this? Works here... > > Subject: hw_random: make buffer usable in scatterlist. > > virtio_rng feeds the randomness buffer handed by the core directly > into the scatterlist, since commit bb347d98079a547e80bd4722dee1de61e4dca0e8. > > However, if CONFIG_HW_RANDOM=m, the static buffer isn't a linear address > (at least on most archs). We could fix this in virtio_rng, but it's actually > far easier to just do it in the core as virtio_rng would have to allocate > a buffer every time (it doesn't know how much the core will want to read). > > Reported-by: Aurelien Jarno <aurelien@aurel32.net> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c > index 1bafb40..69ae597 100644 > --- a/drivers/char/hw_random/core.c > +++ b/drivers/char/hw_random/core.c > @@ -40,6 +40,7 @@ > #include <linux/init.h> > #include <linux/miscdevice.h> > #include <linux/delay.h> > +#include <linux/slab.h> > #include <asm/uaccess.h> > > > @@ -52,8 +53,12 @@ static struct hwrng *current_rng; > static LIST_HEAD(rng_list); > static DEFINE_MUTEX(rng_mutex); > static int data_avail; > -static u8 rng_buffer[SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES] > - __cacheline_aligned; > +static u8 *rng_buffer; > + > +static size_t rng_buffer_size(void) > +{ > + return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES; > +} > > static inline int hwrng_init(struct hwrng *rng) > { > @@ -116,7 +121,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, > > if (!data_avail) { > bytes_read = rng_get_data(current_rng, rng_buffer, > - sizeof(rng_buffer), > + rng_buffer_size(), > !(filp->f_flags & O_NONBLOCK)); > if (bytes_read < 0) { > err = bytes_read; > @@ -307,6 +312,14 @@ int hwrng_register(struct hwrng *rng) > > mutex_lock(&rng_mutex); > > + /* kmalloc makes this safe for virt_to_page() in virtio_rng.c */ > + err = -ENOMEM; > + if (!rng_buffer) { > + rng_buffer = kmalloc(rng_buffer_size(), GFP_KERNEL); > + if (!rng_buffer) > + goto out_unlock; > + } > + > /* Must not register two RNGs with the same name. */ > err = -EEXIST; > list_for_each_entry(tmp, &rng_list, list) { > It works fine for me. Thanks for the patch. Tested-by: Aurelien Jarno <aurelien@aurel32.net> -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurelien@aurel32.net http://www.aurel32.net ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m 2013-02-27 16:36 ` Aurelien Jarno @ 2013-02-28 3:04 ` Rusty Russell 0 siblings, 0 replies; 12+ messages in thread From: Rusty Russell @ 2013-02-28 3:04 UTC (permalink / raw) To: Aurelien Jarno; +Cc: kvm, ian, Matt Mackall, Herbert Xu Aurelien Jarno <aurelien@aurel32.net> writes: > On Wed, Feb 27, 2013 at 11:56:55AM +1030, Rusty Russell wrote: >> Aurelien Jarno <aurelien@aurel32.net> writes: >> > Hi, >> > >> > I have noticed that virtio-rng only returns zero for kernels >= 2.6.33 >> > built with CONFIG_HW_RANDOM=m. This is a bit much too predictable for a >> > random generator ;-). >> > >> > The reason for that is virtio expects guest real addresses, while >> > rng_core.ko (ie when built as a module) is passing a vmalloced buffer >> > to the virtio-rng read function, declared as such: >> > >> > static u8 rng_buffer[SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES] >> > __cacheline_aligned; >> > >> > This is basically the same issue than the following one: >> > >> > https://lists.linux-foundation.org/pipermail/virtualization/2008-May/010946.html >> > >> > but introduced in a more subtle way in this commit: >> > >> > commit bb347d98079a547e80bd4722dee1de61e4dca0e8 >> > Author: Ian Molton <ian.molton@collabora.co.uk> >> > Date: Tue Dec 1 15:26:33 2009 +0800 >> >> OK, I looked at doing a kmalloc and copy in virtio_rng, but it's very >> inelegant (we don't know what size of buffer to allocate). > > On the other hand, the rng API allows to return less bytes than > requested, so it's possible to have a fixed buffer size of for example > 64 or 128 bytes. But I agree it's better to do that in rng core. That's true, too. I'd really like Ian's feedback, since he was the one who made the change, but the previous email address bounced. Trying again... hw_random: make buffer usable in scatterlist. virtio_rng feeds the randomness buffer handed by the core directly into the scatterlist, since commit bb347d98079a547e80bd4722dee1de61e4dca0e8. However, if CONFIG_HW_RANDOM=m, the static buffer isn't a linear address (at least on most archs). We could fix this in virtio_rng, but it's actually far easier to just do it in the core as virtio_rng would have to allocate a buffer every time (it doesn't know how much the core will want to read). Reported-by: Aurelien Jarno <aurelien@aurel32.net> Tested-by: Aurelien Jarno <aurelien@aurel32.net> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c index 1bafb40..69ae597 100644 --- a/drivers/char/hw_random/core.c +++ b/drivers/char/hw_random/core.c @@ -40,6 +40,7 @@ #include <linux/init.h> #include <linux/miscdevice.h> #include <linux/delay.h> +#include <linux/slab.h> #include <asm/uaccess.h> @@ -52,8 +53,12 @@ static struct hwrng *current_rng; static LIST_HEAD(rng_list); static DEFINE_MUTEX(rng_mutex); static int data_avail; -static u8 rng_buffer[SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES] - __cacheline_aligned; +static u8 *rng_buffer; + +static size_t rng_buffer_size(void) +{ + return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES; +} static inline int hwrng_init(struct hwrng *rng) { @@ -116,7 +121,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, if (!data_avail) { bytes_read = rng_get_data(current_rng, rng_buffer, - sizeof(rng_buffer), + rng_buffer_size(), !(filp->f_flags & O_NONBLOCK)); if (bytes_read < 0) { err = bytes_read; @@ -307,6 +312,14 @@ int hwrng_register(struct hwrng *rng) mutex_lock(&rng_mutex); + /* kmalloc makes this safe for virt_to_page() in virtio_rng.c */ + err = -ENOMEM; + if (!rng_buffer) { + rng_buffer = kmalloc(rng_buffer_size(), GFP_KERNEL); + if (!rng_buffer) + goto out_unlock; + } + /* Must not register two RNGs with the same name. */ err = -EEXIST; list_for_each_entry(tmp, &rng_list, list) { ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m 2013-02-27 1:26 ` Rusty Russell 2013-02-27 16:36 ` Aurelien Jarno @ 2013-03-10 8:26 ` Herbert Xu 1 sibling, 0 replies; 12+ messages in thread From: Herbert Xu @ 2013-03-10 8:26 UTC (permalink / raw) To: Rusty Russell; +Cc: Aurelien Jarno, kvm, Ian Molton, Matt Mackall On Wed, Feb 27, 2013 at 11:56:55AM +1030, Rusty Russell wrote: > > OK, I looked at doing a kmalloc and copy in virtio_rng, but it's very > inelegant (we don't know what size of buffer to allocate). As the hwrng API allows you to return any number of bytes, you can just go back to the old virtio-rng code that did the 64-byte buffer and return a maximum of 64 bytes each time. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-05-30 7:19 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-02-24 23:11 virtio-rng only returns zeros with CONFIG_HW_RANDOM=m Aurelien Jarno 2013-02-27 0:13 ` Rusty Russell 2013-02-27 3:40 ` Ben Hutchings 2013-02-27 10:33 ` Aurelien Jarno 2013-02-27 13:22 ` Jens Axboe 2013-02-28 3:00 ` Rusty Russell 2013-05-30 1:07 ` Rusty Russell 2013-05-30 7:18 ` Jens Axboe 2013-02-27 1:26 ` Rusty Russell 2013-02-27 16:36 ` Aurelien Jarno 2013-02-28 3:04 ` Rusty Russell 2013-03-10 8:26 ` Herbert Xu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.