| Message ID | 87sj4ieecg.fsf@rustcorp.com.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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,
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) {