Virtualization - Jul 2014 - [PATCH 0/2] hwrng: don't fetch data before device init

Hi,

When booting a recent kernel under KVM with the virtio-rng device
enabled, the boot process was stalling.  Bisect pointed to a commit
made during the 3.15 window to fetch randomness from newly-registered
devices in the hwrng core.  The details are in the patches.

I considered a couple of approaches, but basing on the init() function
being registered, as is done in patch 1 here, seems like the best idea,
since quite a few drivers need to initialize their devices before data
is fetched off them.

Please review and apply if appropriate,

Amit Shah (2):
  hwrng: don't fetch rng from sources without init
  virtio: rng: introduce an init fn for hwrng core

 drivers/char/hw_random/core.c       |  8 +++++---
 drivers/char/hw_random/virtio-rng.c | 11 +++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

-- 
1.9.3

Commit d9e7972619334 "hwrng: add randomness to system from rng
sources"
added a call to rng_get_data() from the hwrng_register() function.
However, some rng devices need initialization before data can be read
from them.

Also, the virtio-rng device does not behave properly when this call is
made in its probe() routine - the virtio core sets the DRIVER_OK status
bit only on a successful probe, which means the host ignores all
communication from the guest, and the guest insmod or boot process just
sits there doing nothing.

This commit makes the call to rng_get_data() depend on no init fn
pointer being registered by the device.  If an init function is
registered, this call isn't made.

CC: Kees Cook <keescook at chromium.org>
CC: Jason Cooper <jason at lakedaemon.net>
CC: Herbert Xu <herbert at gondor.apana.org.au>
CC: <stable at vger.kernel.org> # For 3.15 only
Signed-off-by: Amit Shah <amit.shah at redhat.com>
---
 drivers/char/hw_random/core.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 334601c..3f3941d 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)
 	INIT_LIST_HEAD(&rng->list);
 	list_add_tail(&rng->list, &rng_list);
 
-	bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
-	if (bytes_read > 0)
-		add_device_randomness(bytes, bytes_read);
+	if (!rng->init) {
+		bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
+		if (bytes_read > 0)
+			add_device_randomness(bytes, bytes_read);
+	}
 out_unlock:
 	mutex_unlock(&rng_mutex);
 out:
-- 
1.9.3

The hwrng core asks for random data in the hwrng_register() call itself
from commit d9e7972619.  This doesn't play well with virtio -- the
DRIVER_OK bit is only set by virtio core on a successful probe, and
we're not yet out of our probe routine when this call is made.  This
causes the host to not acknowledge any requests we put in the virtqueue,
and the insmod or kernel boot process just waits for data to arrive from
the host, which never happens.

The previous commit makes the hwrng core check for on an init function
in the hwrng struct that's registered.  If such a fn exists, the request
for random data isn't made, and the stall when loading the module
doesn't happen.

CC: Kees Cook <keescook at chromium.org>
CC: Jason Cooper <jason at lakedaemon.net>
CC: Herbert Xu <herbert at gondor.apana.org.au>
CC: <stable at vger.kernel.org> # For 3.15 only
Signed-off-by: Amit Shah <amit.shah at redhat.com>
---
 drivers/char/hw_random/virtio-rng.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index f3e7150..c8471ef 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -93,6 +93,16 @@ static void virtio_cleanup(struct hwrng *rng)
 		wait_for_completion(&vi->have_data);
 }
 
+int virtio_init(struct hwrng *rng)
+{
+	/*
+	 * Empty function to ensure hwrandom core does not ask us for
+	 * randomness during hwrng_register phase.  This prevents us
+	 * from exiting to host without VIRTIO_CONFIG_S_DRIVER_OK set.
+	 */
+	return 0;
+}
+
 static int probe_common(struct virtio_device *vdev)
 {
 	int err, index;
@@ -111,6 +121,7 @@ static int probe_common(struct virtio_device *vdev)
 	init_completion(&vi->have_data);
 
 	vi->hwrng = (struct hwrng) {
+		.init = virtio_init,
 		.read = virtio_read,
 		.cleanup = virtio_cleanup,
 		.priv = (unsigned long)vi,
-- 
1.9.3

On Wed, Jul 02, 2014 at 03:58:15PM +0530, Amit Shah
wrote:
> Commit d9e7972619334 "hwrng: add randomness to system from rng
sources"
> added a call to rng_get_data() from the hwrng_register() function.
> However, some rng devices need initialization before data can be read
> from them.
> 
> Also, the virtio-rng device does not behave properly when this call is
> made in its probe() routine - the virtio core sets the DRIVER_OK status
> bit only on a successful probe, which means the host ignores all
> communication from the guest, and the guest insmod or boot process just
> sits there doing nothing.
> 
> This commit makes the call to rng_get_data() depend on no init fn
> pointer being registered by the device.  If an init function is
> registered, this call isn't made.
> 
> CC: Kees Cook <keescook at chromium.org>
> CC: Jason Cooper <jason at lakedaemon.net>
> CC: Herbert Xu <herbert at gondor.apana.org.au>
> CC: <stable at vger.kernel.org> # For 3.15 only
# v3.15+ should be fine here.
> Signed-off-by: Amit Shah <amit.shah at redhat.com>
> ---
>  drivers/char/hw_random/core.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 334601c..3f3941d 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)
>  	INIT_LIST_HEAD(&rng->list);
>  	list_add_tail(&rng->list, &rng_list);
>  
> -	bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> -	if (bytes_read > 0)
> -		add_device_randomness(bytes, bytes_read);
> +	if (!rng->init) {
> +		bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> +		if (bytes_read > 0)
> +			add_device_randomness(bytes, bytes_read);
> +	}
afaict, this is redundant at initialization time.  current_rng shouldn't
be set yet, so hwrng_init(rng) will get called at line 333.  Or, am I
missing something?

thx,

Jason.

Commit d9e7972619334 "hwrng: add randomness to system from rng
sources"
added a call to rng_get_data() from the hwrng_register() function.
However, some rng devices need initialization before data can be read
from them.

Also, the virtio-rng device does not behave properly when this call is
made in its probe() routine - the virtio core sets the DRIVER_OK status
bit only on a successful probe, which means the host ignores all
communication from the guest, and the guest insmod or boot process just
sits there doing nothing.

[ jac: Modify the API to allow drivers to disable reading at probe, new
patch, copied Amit's commit message. ]

CC: Kees Cook <keescook at chromium.org>
CC: Jason Cooper <jason at lakedaemon.net>
CC: Herbert Xu <herbert at gondor.apana.org.au>
CC: <stable at vger.kernel.org> # v3.15+
Signed-off-by: Amit Shah <amit.shah at redhat.com>
Signed-off-by: Jason Cooper <jason at lakedaemon.net>
---
 drivers/char/hw_random/core.c | 8 +++++---
 include/linux/hw_random.h     | 4 ++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 334601cc81cf..b7b6c48ca682 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)
 	INIT_LIST_HEAD(&rng->list);
 	list_add_tail(&rng->list, &rng_list);
 
-	bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
-	if (bytes_read > 0)
-		add_device_randomness(bytes, bytes_read);
+	if (!(rng->flags & HWRNG_NO_READ_AT_PROBE)) {
+		bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
+		if (bytes_read > 0)
+			add_device_randomness(bytes, bytes_read);
+	}
 out_unlock:
 	mutex_unlock(&rng_mutex);
 out:
diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index b4b0eef5fddf..5e358c7f3aae 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -15,6 +15,8 @@
 #include <linux/types.h>
 #include <linux/list.h>
 
+#define HWRNG_NO_READ_AT_PROBE	BIT(0)
+
 /**
  * struct hwrng - Hardware Random Number Generator driver
  * @name:		Unique RNG name.
@@ -28,6 +30,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 properties
  * @priv:		Private data, for use by the RNG driver.
  */
 struct hwrng {
@@ -37,6 +40,7 @@ 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 long flags;
 	unsigned long priv;
 
 	/* internal. */
-- 
2.0.0

The hwrng core asks for random data in the hwrng_register() call itself
from commit d9e7972619.  This doesn't play well with virtio -- the
DRIVER_OK bit is only set by virtio core on a successful probe, and
we're not yet out of our probe routine when this call is made.  This
causes the host to not acknowledge any requests we put in the virtqueue,
and the insmod or kernel boot process just waits for data to arrive from
the host, which never happens.

The previous commit makes the hwrng core check for the
HWRNG_NO_READ_AT_PROBE flag before reading from the device  If the flag
is set, the request for random data isn't made, and the stall when
loading the module doesn't happen.

[jac: reworked to use a flag instead of an init function.  Reused most
of Amit's commit message. ]

CC: Kees Cook <keescook at chromium.org>
CC: Jason Cooper <jason at lakedaemon.net>
CC: Herbert Xu <herbert at gondor.apana.org.au>
CC: <stable at vger.kernel.org> # v3.15+
Signed-off-by: Amit Shah <amit.shah at redhat.com>
Signed-off-by: Jason Cooper <jason at lakedaemon.net>
---
 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 f3e71501de54..f83433bb1fae 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -115,6 +115,7 @@ static int probe_common(struct virtio_device *vdev)
 		.cleanup = virtio_cleanup,
 		.priv = (unsigned long)vi,
 		.name = vi->name,
+		.flags = HWRNG_NO_READ_AT_PROBE,
 	};
 	vdev->priv = vi;
 
-- 
2.0.0

Hi Jason,

On (Wed) 02 Jul 2014 [13:00:19], Jason Cooper wrote:
> Commit d9e7972619334 "hwrng: add randomness to system from rng
sources"
> added a call to rng_get_data() from the hwrng_register() function.
> However, some rng devices need initialization before data can be read
> from them.
> 
> Also, the virtio-rng device does not behave properly when this call is
> made in its probe() routine - the virtio core sets the DRIVER_OK status
> bit only on a successful probe, which means the host ignores all
> communication from the guest, and the guest insmod or boot process just
> sits there doing nothing.
> 
> [ jac: Modify the API to allow drivers to disable reading at probe, new
> patch, copied Amit's commit message. ]
> 
> CC: Kees Cook <keescook at chromium.org>
> CC: Jason Cooper <jason at lakedaemon.net>
> CC: Herbert Xu <herbert at gondor.apana.org.au>
> CC: <stable at vger.kernel.org> # v3.15+
> Signed-off-by: Amit Shah <amit.shah at redhat.com>
> Signed-off-by: Jason Cooper <jason at lakedaemon.net>
> ---
>  drivers/char/hw_random/core.c | 8 +++++---
>  include/linux/hw_random.h     | 4 ++++
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 334601cc81cf..b7b6c48ca682 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)
>  	INIT_LIST_HEAD(&rng->list);
>  	list_add_tail(&rng->list, &rng_list);
>  
> -	bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> -	if (bytes_read > 0)
> -		add_device_randomness(bytes, bytes_read);
> +	if (!(rng->flags & HWRNG_NO_READ_AT_PROBE)) {
> +		bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> +		if (bytes_read > 0)
> +			add_device_randomness(bytes, bytes_read);
> +	}
But this has the inverse problem: if there are two hwrngs in the
system, one will be initialized and probed.  The other will not be
initialized, but still be probed.

My version was more conservative while this one keeps the bug from the
current kernels.

		Amit