[RFC PATCH 0/3] hw_random: support for delayed init randomness requests

[RFC PATCH 0/3] hw_random: support for delayed init randomness requests

[Amit Shah]

Hello,

This series introduces a way to allow devices to contribute to initial
system randomness after a certain delay.  Specifically, the virtio-rng
device can contribute initial randomness only after a successful
probe().

A delayed workqueue item is queued in the system queue to fetch this
randomness if the device indicates it's capable of contributing only
after a delay, via the new HWRNG_DELAY_READ_AT_INIT flag.

This series is intended for the next merge window.  I've marked it as
RFC because the last revert doesn't yet have a linux.git sha1.  I'll
re-post when the previous series gets committed.

Please review,

Amit Shah (3):
  hw_random: allow RNG devices to give early randomness after a delay
  virtio: rng: only accept delayed early randomness requests
  Revert "virtio: rng: ensure reads happen after successful probe"

 drivers/char/hw_random/core.c       | 26 +++++++++++++++++++-------
 drivers/char/hw_random/virtio-rng.c | 11 +----------
 include/linux/hw_random.h           |  8 ++++++++
 3 files changed, 28 insertions(+), 17 deletions(-)

-- 
1.9.3

[RFC PATCH 1/3] hw_random: allow RNG devices to give early randomness after a delay

[Amit Shah]

Some RNG devices may not be ready to give early randomness at probe()
time, and hence lose out on the opportunity to contribute to system
randomness at boot- or device hotplug- time.

This commit schedules a delayed work item for such devices, and fetches
early randomness after a delay.  Currently the delay is 500ms, which is
enough for the lone device that needs such treatment: virtio-rng.

CC: Kees Cook <keescook@chromium.org>
CC: Jason Cooper <jason@lakedaemon.net>
CC: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/hw_random/core.c | 20 +++++++++++++++++++-
 include/linux/hw_random.h     |  8 ++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index c4419ea..2a765fd 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -63,7 +63,7 @@ static size_t rng_buffer_size(void)
 	return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES;
 }
 
-static void add_early_randomness(struct hwrng *rng)
+static void get_early_randomness(struct hwrng *rng)
 {
 	unsigned char bytes[16];
 	int bytes_read;
@@ -79,6 +79,21 @@ static void add_early_randomness(struct hwrng *rng)
 		add_device_randomness(bytes, bytes_read);
 }
 
+static void sched_init_random(struct work_struct *work)
+{
+	struct hwrng *rng = container_of(work, struct hwrng, dwork.work);
+
+	get_early_randomness(rng);
+}
+
+static void add_early_randomness(struct hwrng *rng)
+{
+	if (!(rng->flags & HWRNG_DELAY_READ_AT_INIT))
+		return get_early_randomness(rng);
+
+	schedule_delayed_work(&rng->dwork, msecs_to_jiffies(500));
+}
+
 static inline int hwrng_init(struct hwrng *rng)
 {
 	if (rng->init) {
@@ -351,6 +366,7 @@ int hwrng_register(struct hwrng *rng)
 			goto out_unlock;
 	}
 
+	INIT_DELAYED_WORK(&rng->dwork, sched_init_random);
 	old_rng = current_rng;
 	if (!old_rng) {
 		err = hwrng_init(rng);
@@ -362,6 +378,7 @@ int hwrng_register(struct hwrng *rng)
 	if (!old_rng) {
 		err = register_miscdev();
 		if (err) {
+			cancel_delayed_work_sync(&rng->dwork);
 			hwrng_cleanup(rng);
 			current_rng = NULL;
 			goto out_unlock;
@@ -395,6 +412,7 @@ void hwrng_unregister(struct hwrng *rng)
 	mutex_lock(&rng_mutex);
 
 	list_del(&rng->list);
+	cancel_delayed_work_sync(&rng->dwork);
 	if (current_rng == rng) {
 		hwrng_cleanup(rng);
 		if (list_empty(&rng_list)) {
diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index b4b0eef..8f7370d 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -14,6 +14,11 @@
 
 #include <linux/types.h>
 #include <linux/list.h>
+#include <linux/workqueue.h>
+
+#define HWRNG_DELAY_READ_AT_INIT BIT(0)	/* Schedule delayed work to fetch
+					 * initial randomness instead of doing
+					 * it at ->init()-time */
 
 /**
  * struct hwrng - Hardware Random Number Generator driver
@@ -28,6 +33,7 @@
  *			Must not be NULL.    *OBSOLETE*
  * @read:		New API. drivers can fill up to max bytes of data
  *			into the buffer. The buffer is aligned for any type.
+ * @flags:		Per-device flags.
  * @priv:		Private data, for use by the RNG driver.
  */
 struct hwrng {
@@ -37,9 +43,11 @@ struct hwrng {
 	int (*data_present)(struct hwrng *rng, int wait);
 	int (*data_read)(struct hwrng *rng, u32 *data);
 	int (*read)(struct hwrng *rng, void *data, size_t max, bool wait);
+	unsigned int flags;
 	unsigned long priv;
 
 	/* internal. */
+	struct delayed_work dwork;
 	struct list_head list;
 };
 
-- 
1.9.3

Re: [RFC PATCH 1/3] hw_random: allow RNG devices to give early randomness after a delay

[Jason Cooper]

On Mon, Jul 14, 2014 at 10:05:19AM +0530, Amit Shah wrote:
> Some RNG devices may not be ready to give early randomness at probe()
> time, and hence lose out on the opportunity to contribute to system
> randomness at boot- or device hotplug- time.
> 
> This commit schedules a delayed work item for such devices, and fetches
> early randomness after a delay.  Currently the delay is 500ms, which is
> enough for the lone device that needs such treatment: virtio-rng.
> 
> CC: Kees Cook <keescook@chromium.org>
> CC: Jason Cooper <jason@lakedaemon.net>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/char/hw_random/core.c | 20 +++++++++++++++++++-
>  include/linux/hw_random.h     |  8 ++++++++
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index c4419ea..2a765fd 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -63,7 +63,7 @@ static size_t rng_buffer_size(void)
>  	return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES;
>  }
>  
> -static void add_early_randomness(struct hwrng *rng)
> +static void get_early_randomness(struct hwrng *rng)
>  {
>  	unsigned char bytes[16];
>  	int bytes_read;
> @@ -79,6 +79,21 @@ static void add_early_randomness(struct hwrng *rng)
>  		add_device_randomness(bytes, bytes_read);
>  }
>  
> +static void sched_init_random(struct work_struct *work)
> +{
> +	struct hwrng *rng = container_of(work, struct hwrng, dwork.work);
> +
> +	get_early_randomness(rng);
> +}
> +
> +static void add_early_randomness(struct hwrng *rng)

The add/get naming seems awkward in the above hunks.

> +{
> +	if (!(rng->flags & HWRNG_DELAY_READ_AT_INIT))
> +		return get_early_randomness(rng);
> +
> +	schedule_delayed_work(&rng->dwork, msecs_to_jiffies(500));
> +}
> +

Perhaps instead of rng->flags and a hardcoded delay, we could have
rng->seed_delay = msecs_to_jiffies(500) in virtio-rng?  Then you can
just call unconditionally:

	schedule_delayed_work(&rng->dwork, rng->seed_delay);

I think that would be a more extensible solution should other drivers
show up with the same issue.

thx,

Jason.

Re: [RFC PATCH 1/3] hw_random: allow RNG devices to give early randomness after a delay

[Amit Shah]

On (Mon) 14 Jul 2014 [08:37:00], Jason Cooper wrote:
> On Mon, Jul 14, 2014 at 10:05:19AM +0530, Amit Shah wrote:
> > Some RNG devices may not be ready to give early randomness at probe()
> > time, and hence lose out on the opportunity to contribute to system
> > randomness at boot- or device hotplug- time.
> > 
> > This commit schedules a delayed work item for such devices, and fetches
> > early randomness after a delay.  Currently the delay is 500ms, which is
> > enough for the lone device that needs such treatment: virtio-rng.
> > 
> > CC: Kees Cook <keescook@chromium.org>
> > CC: Jason Cooper <jason@lakedaemon.net>
> > CC: Herbert Xu <herbert@gondor.apana.org.au>
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  drivers/char/hw_random/core.c | 20 +++++++++++++++++++-
> >  include/linux/hw_random.h     |  8 ++++++++
> >  2 files changed, 27 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > index c4419ea..2a765fd 100644
> > --- a/drivers/char/hw_random/core.c
> > +++ b/drivers/char/hw_random/core.c
> > @@ -63,7 +63,7 @@ static size_t rng_buffer_size(void)
> >  	return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES;
> >  }
> >  
> > -static void add_early_randomness(struct hwrng *rng)
> > +static void get_early_randomness(struct hwrng *rng)
> >  {
> >  	unsigned char bytes[16];
> >  	int bytes_read;
> > @@ -79,6 +79,21 @@ static void add_early_randomness(struct hwrng *rng)
> >  		add_device_randomness(bytes, bytes_read);
> >  }
> >  
> > +static void sched_init_random(struct work_struct *work)
> > +{
> > +	struct hwrng *rng = container_of(work, struct hwrng, dwork.work);
> > +
> > +	get_early_randomness(rng);
> > +}
> > +
> > +static void add_early_randomness(struct hwrng *rng)
> 
> The add/get naming seems awkward in the above hunks.

Yea; I felt that too.  I thought of a do_add_early_randomness()
instead, but that seemed awkward too.  I forgot to mention I was
planning on revisiting this naming for v1.

> > +{
> > +	if (!(rng->flags & HWRNG_DELAY_READ_AT_INIT))
> > +		return get_early_randomness(rng);
> > +
> > +	schedule_delayed_work(&rng->dwork, msecs_to_jiffies(500));
> > +}
> > +
> 
> Perhaps instead of rng->flags and a hardcoded delay, we could have
> rng->seed_delay = msecs_to_jiffies(500) in virtio-rng?  Then you can
> just call unconditionally:
> 
> 	schedule_delayed_work(&rng->dwork, rng->seed_delay);
> 
> I think that would be a more extensible solution should other drivers
> show up with the same issue.

Sounds like a good idea to me.  Though, changes in core.c that
increase the time in hwrng_register() or hwrng_init() may not get
noticed by rng drivers and they may suddenly start failing for no
apparent reason.  Seems like a far stretch, though.  Does anyone else
have an opinion on this?

Thanks,

		Amit

Re: [RFC PATCH 1/3] hw_random: allow RNG devices to give early randomness after a delay

[Amit Shah]

On (Mon) 14 Jul 2014 [18:12:46], Amit Shah wrote:
> On (Mon) 14 Jul 2014 [08:37:00], Jason Cooper wrote:
> > On Mon, Jul 14, 2014 at 10:05:19AM +0530, Amit Shah wrote:
> > > Some RNG devices may not be ready to give early randomness at probe()
> > > time, and hence lose out on the opportunity to contribute to system
> > > randomness at boot- or device hotplug- time.
> > > 
> > > This commit schedules a delayed work item for such devices, and fetches
> > > early randomness after a delay.  Currently the delay is 500ms, which is
> > > enough for the lone device that needs such treatment: virtio-rng.
> > > 
> > > CC: Kees Cook <keescook@chromium.org>
> > > CC: Jason Cooper <jason@lakedaemon.net>
> > > CC: Herbert Xu <herbert@gondor.apana.org.au>
> > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > ---
> > >  drivers/char/hw_random/core.c | 20 +++++++++++++++++++-
> > >  include/linux/hw_random.h     |  8 ++++++++
> > >  2 files changed, 27 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > > index c4419ea..2a765fd 100644
> > > --- a/drivers/char/hw_random/core.c
> > > +++ b/drivers/char/hw_random/core.c
> > > @@ -63,7 +63,7 @@ static size_t rng_buffer_size(void)
> > >  	return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES;
> > >  }
> > >  
> > > -static void add_early_randomness(struct hwrng *rng)
> > > +static void get_early_randomness(struct hwrng *rng)
> > >  {
> > >  	unsigned char bytes[16];
> > >  	int bytes_read;
> > > @@ -79,6 +79,21 @@ static void add_early_randomness(struct hwrng *rng)
> > >  		add_device_randomness(bytes, bytes_read);
> > >  }
> > >  
> > > +static void sched_init_random(struct work_struct *work)
> > > +{
> > > +	struct hwrng *rng = container_of(work, struct hwrng, dwork.work);
> > > +
> > > +	get_early_randomness(rng);
> > > +}
> > > +
> > > +static void add_early_randomness(struct hwrng *rng)
> > 
> > The add/get naming seems awkward in the above hunks.
> 
> Yea; I felt that too.  I thought of a do_add_early_randomness()
> instead, but that seemed awkward too.  I forgot to mention I was
> planning on revisiting this naming for v1.
> 
> > > +{
> > > +	if (!(rng->flags & HWRNG_DELAY_READ_AT_INIT))
> > > +		return get_early_randomness(rng);
> > > +
> > > +	schedule_delayed_work(&rng->dwork, msecs_to_jiffies(500));
> > > +}
> > > +
> > 
> > Perhaps instead of rng->flags and a hardcoded delay, we could have
> > rng->seed_delay = msecs_to_jiffies(500) in virtio-rng?  Then you can
> > just call unconditionally:
> > 
> > 	schedule_delayed_work(&rng->dwork, rng->seed_delay);

BTW I didn't want to make this call unconditional -- i.e. the existing
behaviour of in-line fetching of randomness for all devices but one
should not be affected.

If indeed people are OK with this being done by a delayed work item
for all the drivers, the code can get a bit simpler here.

> > I think that would be a more extensible solution should other drivers
> > show up with the same issue.
> 
> Sounds like a good idea to me.  Though, changes in core.c that
> increase the time in hwrng_register() or hwrng_init() may not get
> noticed by rng drivers and they may suddenly start failing for no
> apparent reason.  Seems like a far stretch, though.  Does anyone else
> have an opinion on this?

Herbert, do you have any preference?

Thanks,
		Amit

Re: [RFC PATCH 1/3] hw_random: allow RNG devices to give early randomness after a delay

[Herbert Xu]

On Fri, Jul 18, 2014 at 02:26:26PM +0530, Amit Shah wrote:
>
> > Sounds like a good idea to me.  Though, changes in core.c that
> > increase the time in hwrng_register() or hwrng_init() may not get
> > noticed by rng drivers and they may suddenly start failing for no
> > apparent reason.  Seems like a far stretch, though.  Does anyone else
> > have an opinion on this?
> 
> Herbert, do you have any preference?

So it's only virtio-rng that's a problem, right? How about if we
abuse the scan hook in virtio and move the hwrng_register there?

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: [RFC PATCH 1/3] hw_random: allow RNG devices to give early randomness after a delay

[Amit Shah]

On (Fri) 18 Jul 2014 [17:14:08], Herbert Xu wrote:
> On Fri, Jul 18, 2014 at 02:26:26PM +0530, Amit Shah wrote:
> >
> > > Sounds like a good idea to me.  Though, changes in core.c that
> > > increase the time in hwrng_register() or hwrng_init() may not get
> > > noticed by rng drivers and they may suddenly start failing for no
> > > apparent reason.  Seems like a far stretch, though.  Does anyone else
> > > have an opinion on this?
> > 
> > Herbert, do you have any preference?
> 
> So it's only virtio-rng that's a problem, right? How about if we
> abuse the scan hook in virtio and move the hwrng_register there?

Oops, I had completely missed the scan hook, and looks like it was
added for exactly the purpose we want here (so we won't even be
abusing it!)

Thanks!

I'll post the patches when the one to revert gets an upstream commit
id.

		Amit

[RFC PATCH 2/3] virtio: rng: only accept delayed early randomness requests

[Amit Shah]

hw_random core can ask for initial randomness after a slight delay after
probe() finishes, and we can contribute to system randomness at that
point.  Tell the hw_random core by setting the HWRNG_DELAY_READ_AT_INIT
flag.

CC: Kees Cook <keescook@chromium.org>
CC: Jason Cooper <jason@lakedaemon.net>
CC: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/hw_random/virtio-rng.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index d8ffebd..f53f1e8 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -123,6 +123,7 @@ static int probe_common(struct virtio_device *vdev)
 		.cleanup = virtio_cleanup,
 		.priv = (unsigned long)vi,
 		.name = vi->name,
+		.flags = HWRNG_DELAY_READ_AT_INIT,
 	};
 	vdev->priv = vi;
 
-- 
1.9.3

[RFC PATCH 3/3] Revert "virtio: rng: ensure reads happen after successful probe"

[Amit Shah]

This reverts commit ceb5d72a2e27e95bc9570ce259c45b35f0e23462.

This commit was added for -stable so systems with virtio-rng don't
freeze at boot-time.

With the addition of the previous commits that delay the request for
initial randomness after probe() is successful, this is no longer
needed.

CC: Kees Cook <keescook@chromium.org>
CC: Jason Cooper <jason@lakedaemon.net>
CC: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/hw_random/core.c       |  6 ------
 drivers/char/hw_random/virtio-rng.c | 10 ----------
 2 files changed, 16 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 2a765fd..148d9be 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -68,12 +68,6 @@ static void get_early_randomness(struct hwrng *rng)
 	unsigned char bytes[16];
 	int bytes_read;
 
-	/*
-	 * Currently only virtio-rng cannot return data during device
-	 * probe, and that's handled in virtio-rng.c itself.  If there
-	 * are more such devices, this call to rng_get_data can be
-	 * made conditional here instead of doing it per-device.
-	 */
 	bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
 	if (bytes_read > 0)
 		add_device_randomness(bytes, bytes_read);
diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index f53f1e8..f7d1573 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -37,8 +37,6 @@ struct virtrng_info {
 	int index;
 };
 
-static bool probe_done;
-
 static void random_recv_done(struct virtqueue *vq)
 {
 	struct virtrng_info *vi = vq->vdev->priv;
@@ -68,13 +66,6 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 	int ret;
 	struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
 
-	/*
-	 * Don't ask host for data till we're setup.  This call can
-	 * happen during hwrng_register(), after commit d9e7972619.
-	 */
-	if (unlikely(!probe_done))
-		return 0;
-
 	if (!vi->busy) {
 		vi->busy = true;
 		init_completion(&vi->have_data);
@@ -146,7 +137,6 @@ static int probe_common(struct virtio_device *vdev)
 		return err;
 	}
 
-	probe_done = true;
 	return 0;
 }
 
-- 
1.9.3