hw_random: support for delayed init randomness requests

RFC PATCH In review Amit Shah 3 participants 9 messages Jul 14, 2014 – Jul 18, 2014

Subsystems (3)

virtio core Hardware random number generator core virtio rng driver

lore

Share & tools

lore

Amit Shah

author

2014-07-14 04:35 UTC #0 lore raw

cover hw_random: support for delayed init randomness requests

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

Signature · 1 line

1.9.3

Amit Shah

author

2014-07-14 04:35 UTC #1 lore raw

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

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

CC Jason Cooper <[email protected]>

CC Herbert Xu <[email protected]>

Signed-off-by Amit Shah <[email protected]>

---
 drivers/char/hw_random/core.c | 20 +++++++++++++++++++-
 include/linux/hw_random.h     |  8 ++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

drivers/char/hw_random/core.c +19 -1

index c4419ea..2a765fd 100644
@@ -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)) {

include/linux/hw_random.h +8 -0

index b4b0eef..8f7370d 100644
@@ -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;
 };

Signature · 1 line

1.9.3

Jason Cooper

2014-07-14 12:37 UTC #2 lore raw

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

Quoted 32 lines · 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 <[email protected]>
CC Jason Cooper <[email protected]>
CC Herbert Xu <[email protected]>
Signed-off-by Amit Shah <[email protected]>
---
 drivers/char/hw_random/core.c | 20 +++++++++++++++++++-
 include/linux/hw_random.h     |  8 ++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)
drivers/char/hw_random/core.c +1 -1
index c4419ea..2a765fd 100644
@@ -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);

drivers/char/hw_random/core.c static void add_early_randomness(struct hwrng *rng) · line 89

Quoted 10 lines

drivers/char/hw_random/core.c +8 -0

 }
 
+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.

drivers/char/hw_random/core.c static void add_early_randomness(struct hwrng *rng) · line 96

Quoted 7 lines

drivers/char/hw_random/core.c +7 -0

+{
+	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.

Amit Shah

author

2014-07-14 12:42 UTC #3 lore raw

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

Quoted 35 lines · Jason Cooper

Quoted 34 lines · 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 <[email protected]>
CC Jason Cooper <[email protected]>
CC Herbert Xu <[email protected]>
Signed-off-by Amit Shah <[email protected]>
---
 drivers/char/hw_random/core.c | 20 +++++++++++++++++++-
 include/linux/hw_random.h     |  8 ++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)
drivers/char/hw_random/core.c +1 -1
index c4419ea..2a765fd 100644
@@ -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);
 }
 

Quoted 10 lines

drivers/char/hw_random/core.c static void add_early_randomness(struct hwrng *rng) · line 89
Quoted 8 lines
drivers/char/hw_random/core.c +8 -0
+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.

Quoted 12 lines

drivers/char/hw_random/core.c static void add_early_randomness(struct hwrng *rng) · line 96
Quoted 7 lines
drivers/char/hw_random/core.c +7 -0
+{
+	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:

Quoted 4 lines

	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

Amit Shah

author

2014-07-18 08:56 UTC #4 lore raw

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

Quoted 47 lines · Amit Shah

Quoted 35 lines · Jason Cooper
Quoted 34 lines · 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 <[email protected]>
CC Jason Cooper <[email protected]>
CC Herbert Xu <[email protected]>
Signed-off-by Amit Shah <[email protected]>
---
 drivers/char/hw_random/core.c | 20 +++++++++++++++++++-
 include/linux/hw_random.h     |  8 ++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)
drivers/char/hw_random/core.c +1 -1
index c4419ea..2a765fd 100644
@@ -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);
 }
 
Quoted 10 lines
drivers/char/hw_random/core.c static void add_early_randomness(struct hwrng *rng) · line 89
Quoted 8 lines
drivers/char/hw_random/core.c +8 -0
+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.

Quoted 17 lines

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.
Quoted 13 lines
drivers/char/hw_random/core.c static void add_early_randomness(struct hwrng *rng) · line 96
Quoted 7 lines
drivers/char/hw_random/core.c +7 -0
+{
+	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.

Quoted 8 lines

Quoted 2 lines
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

Herbert Xu

maintainer

2014-07-18 09:14 UTC #5 lore raw

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

Quoted 8 lines · Amit Shah

Quoted 6 lines
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,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Amit Shah

author

2014-07-18 09:27 UTC #6 lore raw

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

Quoted 8 lines · Herbert Xu

Quoted 7 lines · Amit Shah
Quoted 6 lines
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?

Quoted 4 lines

Quoted 1 line
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

Amit Shah

author

2014-07-14 04:35 UTC #7 lore raw

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

In reply to 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 <[email protected]>

CC Jason Cooper <[email protected]>

CC Herbert Xu <[email protected]>

Signed-off-by Amit Shah <[email protected]>

---
 drivers/char/hw_random/virtio-rng.c | 1 +
 1 file changed, 1 insertion(+)

drivers/char/hw_random/virtio-rng.c +1 -0

index d8ffebd..f53f1e8 100644
@@ -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;

Signature · 1 line

1.9.3

Amit Shah

author

2014-07-14 04:35 UTC #8 lore raw

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

In reply to 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 <[email protected]>

CC Jason Cooper <[email protected]>

CC Herbert Xu <[email protected]>

Signed-off-by Amit Shah <[email protected]>

---
 drivers/char/hw_random/core.c       |  6 ------
 drivers/char/hw_random/virtio-rng.c | 10 ----------
 2 files changed, 16 deletions(-)

drivers/char/hw_random/core.c +0 -6

index 2a765fd..148d9be 100644
@@ -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);

drivers/char/hw_random/virtio-rng.c +0 ~~-10~~

index f53f1e8..f7d1573 100644
@@ -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;
 }

Signature · 1 line

1.9.3