virtio-rng only returns zeros with CONFIG_HW_RANDOM=m

virtio-rng only returns zeros with CONFIG_HW_RANDOM=m

[Aurelien Jarno]

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

Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m

[Rusty Russell]

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));
 }
 

Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m

[Rusty Russell]

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) {

Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m

[Ben Hutchings]

[-- 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 --]

Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m

[Aurelien Jarno]

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

Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m

[Jens Axboe]

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

Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m

[Aurelien Jarno]

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

Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m

[Rusty Russell]

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.

Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m

[Rusty Russell]

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) {

Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m

[Herbert Xu]

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

Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m

[Rusty Russell]

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));
 }
 

Re: virtio-rng only returns zeros with CONFIG_HW_RANDOM=m

[Jens Axboe]

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