hwrng: don't fetch data before device init
cover 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(-)
Signature · 1 line
1.9.3
PATCH 1/2 hwrng: don't fetch rng from sources without init
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.
--- drivers/char/hw_random/core.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/char/hw_random/core.c +5 -3
@@ -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:
Signature · 1 line
1.9.3
Re: [PATCH 1/2] hwrng: don't fetch rng from sources without init
Quoted 11 lines · Amit Shah
Commit
d9e7972619334"hwrng: add randomness to system from rng sources" added a call torng_get_data()from thehwrng_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 theDRIVER_OKstatus 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.
Quoted 8 lines
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 <[email protected]>CC Jason Cooper <[email protected]>CC Herbert Xu <[email protected]>CC <[email protected]> # For 3.15 only
# v3.15+ should be fine here.
Quoted 11 lines
Signed-off-by Amit Shah <[email protected]>--- drivers/char/hw_random/core.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)drivers/char/hw_random/core.c +0
-0@@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)INIT_LIST_HEAD (&rng->list);
drivers/char/hw_random/core.c int hwrng_register(struct hwrng *rng) · line 354
Quoted 10 lines
drivers/char/hw_random/core.c +5
-3list_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.
Re: [PATCH 1/2] hwrng: don't fetch rng from sources without init
Quoted 36 lines · Jason Cooper
Quoted 11 lines · Amit Shah
Commit
d9e7972619334"hwrng: add randomness to system from rng sources" added a call torng_get_data()from thehwrng_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 theDRIVER_OKstatus 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.Quoted 8 lines
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 <[email protected]>CC Jason Cooper <[email protected]>CC Herbert Xu <[email protected]>CC <[email protected]> # For 3.15 only# v3.15+ should be fine here.
Quoted 13 lines
Signed-off-by Amit Shah <[email protected]>--- drivers/char/hw_random/core.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)drivers/char/hw_random/core.c +0
-0@@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)INIT_LIST_HEAD (&rng->list);list_add_tail (&rng->list, &rng_list);
Quoted 12 lines
drivers/char/hw_random/core.c int hwrng_register(struct hwrng *rng) · line 354
Quoted 8 lines
drivers/char/hw_random/core.c +5
-3- 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_rngshouldn't be set yet, sohwrng_init(rng)will get called at line 333. Or, am I missing something?
You're right -- the device will have been initialized if it's the first hwrng device to be registered. And in that case, this device won't contribute to the initial pool (and that's the only case).
Can someone think of a better way to handle this for that case, then?
Amit
Re: [PATCH 1/2] hwrng: don't fetch rng from sources without init
Quoted 50 lines · Amit Shah
Quoted 36 lines · Jason Cooper
Quoted 11 lines · Amit Shah
Commit
d9e7972619334"hwrng: add randomness to system from rng sources" added a call torng_get_data()from thehwrng_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 theDRIVER_OKstatus 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.Quoted 8 lines
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 <[email protected]>CC Jason Cooper <[email protected]>CC Herbert Xu <[email protected]>CC <[email protected]> # For 3.15 only# v3.15+ should be fine here.
Quoted 13 lines
Signed-off-by Amit Shah <[email protected]>--- drivers/char/hw_random/core.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)drivers/char/hw_random/core.c +0
-0@@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)INIT_LIST_HEAD (&rng->list);list_add_tail (&rng->list, &rng_list);Quoted 12 lines
drivers/char/hw_random/core.c int hwrng_register(struct hwrng *rng) · line 354
Quoted 8 lines
drivers/char/hw_random/core.c +5
-3- 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_rngshouldn't be set yet, sohwrng_init(rng)will get called at line 333. Or, am I missing something?
Quoted 5 lines
You're right -- the device will have been initialized if it's the first hwrng device to be registered. And in that case, this device won't contribute to the initial pool (and that's the only case).
Can someone think of a better way to handle this for that case, then?
I'm cooking up something...
thx,
Jason.
Re: [PATCH 1/2] hwrng: don't fetch rng from sources without init
In reply to Amit ShahQuoted 50 lines · Amit Shah
Quoted 36 lines · Jason Cooper
Quoted 11 lines · Amit Shah
Commit
d9e7972619334"hwrng: add randomness to system from rng sources" added a call torng_get_data()from thehwrng_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 theDRIVER_OKstatus 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.Quoted 8 lines
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 <[email protected]>CC Jason Cooper <[email protected]>CC Herbert Xu <[email protected]>CC <[email protected]> # For 3.15 only# v3.15+ should be fine here.
Quoted 13 lines
Signed-off-by Amit Shah <[email protected]>--- drivers/char/hw_random/core.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)drivers/char/hw_random/core.c +0
-0@@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)INIT_LIST_HEAD (&rng->list);list_add_tail (&rng->list, &rng_list);Quoted 12 lines
drivers/char/hw_random/core.c int hwrng_register(struct hwrng *rng) · line 354
Quoted 8 lines
drivers/char/hw_random/core.c +5
-3- 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_rngshouldn't be set yet, sohwrng_init(rng)will get called at line 333. Or, am I missing something?
Quoted 5 lines
You're right -- the device will have been initialized if it's the first hwrng device to be registered. And in that case, this device won't contribute to the initial pool (and that's the only case).
Can someone think of a better way to handle this for that case, then?
I'm cooking up something...
thx,
Jason.
Re: [PATCH 1/2] hwrng: don't fetch rng from sources without init
In reply to Amit ShahQuoted 11 lines · Amit Shah
Commit
d9e7972619334"hwrng: add randomness to system from rng sources" added a call torng_get_data()from thehwrng_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 theDRIVER_OKstatus 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.
Quoted 8 lines
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 <[email protected]>CC Jason Cooper <[email protected]>CC Herbert Xu <[email protected]>CC <[email protected]> # For 3.15 only
# v3.15+ should be fine here.
Quoted 11 lines
Signed-off-by Amit Shah <[email protected]>--- drivers/char/hw_random/core.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)drivers/char/hw_random/core.c +0
-0@@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)INIT_LIST_HEAD (&rng->list);
drivers/char/hw_random/core.c int hwrng_register(struct hwrng *rng) · line 354
Quoted 10 lines
drivers/char/hw_random/core.c +5
-3list_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.
PATCH 2/2 virtio: rng: introduce an init fn for hwrng core
In reply to Amit ShahThe 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.
--- drivers/char/hw_random/virtio-rng.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
drivers/char/hw_random/virtio-rng.c +11 -0
@@ -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,
Signature · 1 line
1.9.3
PATCH v2 1/2 hwrng: Allow drivers to disable reading during probe
In reply to Amit ShahCommit 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. ]
--- drivers/char/hw_random/core.c | 8 +++++--- include/linux/hw_random.h | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-)
drivers/char/hw_random/core.c +5 -3
@@ -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:
include/linux/hw_random.h +4 -0
@@ -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. */
Signature · 1 line
2.0.0
PATCH v2 2/2 hwrng: virtio: disable reading during probe
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. ]
--- drivers/char/hw_random/virtio-rng.c | 1 + 1 file changed, 1 insertion(+)
drivers/char/hw_random/virtio-rng.c +1 -0
@@ -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;
Signature · 1 line
2.0.0
PATCH v2 2/2 hwrng: virtio: disable reading during probe
In reply to Jason CooperThe 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. ]
--- drivers/char/hw_random/virtio-rng.c | 1 + 1 file changed, 1 insertion(+)
drivers/char/hw_random/virtio-rng.c +1 -0
@@ -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;
Signature · 1 line
2.0.0
Re: [PATCH 1/2 v2] hwrng: Allow drivers to disable reading during probe
In reply to Jason CooperHi Jason,
Quoted 31 lines · Jason Cooper
Commit
d9e7972619334"hwrng: add randomness to system from rng sources" added a call torng_get_data()from thehwrng_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 theDRIVER_OKstatus 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 <[email protected]>CC Jason Cooper <[email protected]>CC Herbert Xu <[email protected]>CC <[email protected]> # v3.15+Signed-off-by Amit Shah <[email protected]>Signed-off-by Jason Cooper <[email protected]>--- drivers/char/hw_random/core.c | 8 +++++--- include/linux/hw_random.h | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-)drivers/char/hw_random/core.c +0
-0@@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)INIT_LIST_HEAD (&rng->list);
drivers/char/hw_random/core.c int hwrng_register(struct hwrng *rng) · line 354
Quoted 10 lines
drivers/char/hw_random/core.c +5
-3list_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
Re: [PATCH 1/2 v2] hwrng: Allow drivers to disable reading during probe
Quoted 36 lines · Amit Shah
Hi Jason,
Quoted 33 lines · Jason Cooper
Commit
d9e7972619334"hwrng: add randomness to system from rng sources" added a call torng_get_data()from thehwrng_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 theDRIVER_OKstatus 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 <[email protected]>CC Jason Cooper <[email protected]>CC Herbert Xu <[email protected]>CC <[email protected]> # v3.15+Signed-off-by Amit Shah <[email protected]>Signed-off-by Jason Cooper <[email protected]>--- drivers/char/hw_random/core.c | 8 +++++--- include/linux/hw_random.h | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-)drivers/char/hw_random/core.c +0
-0@@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)INIT_LIST_HEAD (&rng->list);list_add_tail (&rng->list, &rng_list);
Quoted 12 lines
drivers/char/hw_random/core.c int hwrng_register(struct hwrng *rng) · line 354
Quoted 8 lines
drivers/char/hw_random/core.c +5
-3- 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.
That's a problem outside the scope of this patch. You're basically saying the ->init() should be called unconditionally for each hwrng. If that's what driver authors assumed, that's not what is happening if there is more than one driver in the system.
I think you should be changing the code a few lines up to make sure hwrng_init() is called once for each driver.
Quoted 2 lines
My version was more conservative while this one keeps the bug from the current kernels.
Huh? What do you mean by "keeps the bug from the current kernels." ?
Besides, you're second patch isn't actually doing any ->init to get the hwrng ready for reading... If you had a real ->init function, and it was always called, would rng_get_data() work at probe time for your driver?
confused,
Jason.
Re: [PATCH 1/2 v2] hwrng: Allow drivers to disable reading during probe
Quoted 50 lines · Jason Cooper
Quoted 36 lines · Amit Shah
Hi Jason,
Quoted 33 lines · Jason Cooper
Commit
d9e7972619334"hwrng: add randomness to system from rng sources" added a call torng_get_data()from thehwrng_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 theDRIVER_OKstatus 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 <[email protected]>CC Jason Cooper <[email protected]>CC Herbert Xu <[email protected]>CC <[email protected]> # v3.15+Signed-off-by Amit Shah <[email protected]>Signed-off-by Jason Cooper <[email protected]>--- drivers/char/hw_random/core.c | 8 +++++--- include/linux/hw_random.h | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-)drivers/char/hw_random/core.c +0
-0@@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)INIT_LIST_HEAD (&rng->list);list_add_tail (&rng->list, &rng_list);Quoted 12 lines
drivers/char/hw_random/core.c int hwrng_register(struct hwrng *rng) · line 354
Quoted 8 lines
drivers/char/hw_random/core.c +5
-3- 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.
Quoted 4 lines
That's a problem outside the scope of this patch. You're basically saying the ->init() should be called unconditionally for each hwrng. If that's what driver authors assumed, that's not what is happening if there is more than one driver in the system.
My intent with the patch was to get randomness added when a hwrng was available. Perhaps instead of skipping this call, it can be done either during probe (for devices that support it or lack an init function), or done after initialization?
Quoted 2 lines
I think you should be changing the code a few lines up to make sure
hwrng_init()is called once for each driver.
Is that really how it should work? I'd be for the "always-init" approach since it gains us access to the entropy, but I have to play devil's advocate: would one expect "probe" to just probe, not initialize? That seems more like a function of enabling the device.
-Kees
Signature · 2 lines
Kees Cook Chrome OS Security
Re: [PATCH 1/2 v2] hwrng: Allow drivers to disable reading during probe
Quoted 56 lines · Kees Cook
Quoted 50 lines · Jason Cooper
Quoted 36 lines · Amit Shah
Hi Jason,
Quoted 33 lines · Jason Cooper
Commit
d9e7972619334"hwrng: add randomness to system from rng sources" added a call torng_get_data()from thehwrng_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 theDRIVER_OKstatus 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 <[email protected]>CC Jason Cooper <[email protected]>CC Herbert Xu <[email protected]>CC <[email protected]> # v3.15+Signed-off-by Amit Shah <[email protected]>Signed-off-by Jason Cooper <[email protected]>--- drivers/char/hw_random/core.c | 8 +++++--- include/linux/hw_random.h | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-)drivers/char/hw_random/core.c +0
-0@@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)INIT_LIST_HEAD (&rng->list);list_add_tail (&rng->list, &rng_list);Quoted 12 lines
drivers/char/hw_random/core.c int hwrng_register(struct hwrng *rng) · line 354
Quoted 8 lines
drivers/char/hw_random/core.c +5
-3- 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.
Quoted 4 lines
That's a problem outside the scope of this patch. You're basically saying the ->init() should be called unconditionally for each hwrng. If that's what driver authors assumed, that's not what is happening if there is more than one driver in the system.
Quoted 4 lines
My intent with the patch was to get randomness added when a hwrng was available. Perhaps instead of skipping this call, it can be done either during probe (for devices that support it or lack an init function), or done after initialization?
I'll also look at just going ahead with ->init() in the hwrng_register function itself, that's in line with the spirit of your patch.
However, that may lead to unwelcome problems. The other approach is to call rng_get_data() in hwrng_init().
Quoted 7 lines
Quoted 2 lines
I think you should be changing the code a few lines up to make sure
hwrng_init()is called once for each driver.Is that really how it should work? I'd be for the "always-init" approach since it gains us access to the entropy, but I have to play devil's advocate: would one expect "probe" to just probe, not initialize? That seems more like a function of enabling the device.
There are some other considerations: I found this problem while looking at other things: especially that the hwrng core doesn't take a reference on the driver that's the current_rng. That has to be fixed. If we always call ->init(), we'll have to cleanup and possibly take a reference. I'll look at this more closely in a bit.
Amit
Re: [PATCH 1/2 v2] hwrng: Allow drivers to disable reading during probe
In reply to Jason CooperQuoted 50 lines · Jason Cooper
Quoted 36 lines · Amit Shah
Hi Jason,
Quoted 33 lines · Jason Cooper
Commit
d9e7972619334"hwrng: add randomness to system from rng sources" added a call torng_get_data()from thehwrng_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 theDRIVER_OKstatus 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 <[email protected]>CC Jason Cooper <[email protected]>CC Herbert Xu <[email protected]>CC <[email protected]> # v3.15+Signed-off-by Amit Shah <[email protected]>Signed-off-by Jason Cooper <[email protected]>--- drivers/char/hw_random/core.c | 8 +++++--- include/linux/hw_random.h | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-)drivers/char/hw_random/core.c +0
-0@@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)INIT_LIST_HEAD (&rng->list);list_add_tail (&rng->list, &rng_list);Quoted 12 lines
drivers/char/hw_random/core.c int hwrng_register(struct hwrng *rng) · line 354
Quoted 8 lines
drivers/char/hw_random/core.c +5
-3- 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.
Quoted 1 line
That's a problem outside the scope of this patch.
No, not really. What will happen in a 2 hwrng scenario is that the first one that calls hwrng_register() will get its init() routine called, and probed for data properly.
The second device to call hwrng_register() will not get its init routine called, but still will be probed. And that will result in unpredictable behaviour, as the device wouldn't have been initialized for reading of the data. This bug was introduced by the original patch, which was fixed by my series (by ensuring probe is only called if init was called first. But it missed one case where probe will not be called even if init was called, as you pointed out).
Quoted 4 lines
You're basically
saying the ->init() should be called unconditionally for each hwrng. If that's what driver authors assumed, that's not what is happening if there is more than one driver in the system.
Either that, or that rng_get_data() should be called in hwrng_init(), or not call rng_get_data() at all on that device.
Quoted 7 lines
I think you should be changing the code a few lines up to make sure
hwrng_init()is called once for each driver.Quoted 2 lines
My version was more conservative while this one keeps the bug from the current kernels.
Huh? What do you mean by "keeps the bug from the current kernels." ?
Sorry; hope the explanation above is clearer.
Quoted 4 lines
Besides, you're second patch isn't actually doing any ->init to get the hwrng ready for reading... If you had a real ->init function, and it was always called, would
rng_get_data()work at probe time for your driver?
Right; that's the reason I didn't do it that way.
I think a combination of my patches and your flags approach will solve the issues; I'll post a v2 tomorrow.
Amit
Re: [PATCH 1/2 v2] hwrng: Allow drivers to disable reading during probe
In reply to Jason CooperQuoted 50 lines · Jason Cooper
Quoted 36 lines · Amit Shah
Hi Jason,
Quoted 33 lines · Jason Cooper
Commit
d9e7972619334"hwrng: add randomness to system from rng sources" added a call torng_get_data()from thehwrng_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 theDRIVER_OKstatus 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 <[email protected]>CC Jason Cooper <[email protected]>CC Herbert Xu <[email protected]>CC <[email protected]> # v3.15+Signed-off-by Amit Shah <[email protected]>Signed-off-by Jason Cooper <[email protected]>--- drivers/char/hw_random/core.c | 8 +++++--- include/linux/hw_random.h | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-)drivers/char/hw_random/core.c +0
-0@@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)INIT_LIST_HEAD (&rng->list);list_add_tail (&rng->list, &rng_list);Quoted 12 lines
drivers/char/hw_random/core.c int hwrng_register(struct hwrng *rng) · line 354
Quoted 8 lines
drivers/char/hw_random/core.c +5
-3- 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.
Quoted 1 line
That's a problem outside the scope of this patch.
No, not really. What will happen in a 2 hwrng scenario is that the first one that calls hwrng_register() will get its init() routine called, and probed for data properly.
The second device to call hwrng_register() will not get its init routine called, but still will be probed. And that will result in unpredictable behaviour, as the device wouldn't have been initialized for reading of the data. This bug was introduced by the original patch, which was fixed by my series (by ensuring probe is only called if init was called first. But it missed one case where probe will not be called even if init was called, as you pointed out).
Quoted 4 lines
You're basically
saying the ->init() should be called unconditionally for each hwrng. If that's what driver authors assumed, that's not what is happening if there is more than one driver in the system.
Either that, or that rng_get_data() should be called in hwrng_init(), or not call rng_get_data() at all on that device.
Quoted 7 lines
I think you should be changing the code a few lines up to make sure
hwrng_init()is called once for each driver.Quoted 2 lines
My version was more conservative while this one keeps the bug from the current kernels.
Huh? What do you mean by "keeps the bug from the current kernels." ?
Sorry; hope the explanation above is clearer.
Quoted 4 lines
Besides, you're second patch isn't actually doing any ->init to get the hwrng ready for reading... If you had a real ->init function, and it was always called, would
rng_get_data()work at probe time for your driver?
Right; that's the reason I didn't do it that way.
I think a combination of my patches and your flags approach will solve the issues; I'll post a v2 tomorrow.
Amit
Re: [PATCH 1/2 v2] hwrng: Allow drivers to disable reading during probe
In reply to Amit ShahQuoted 36 lines · Amit Shah
Hi Jason,
Quoted 33 lines · Jason Cooper
Commit
d9e7972619334"hwrng: add randomness to system from rng sources" added a call torng_get_data()from thehwrng_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 theDRIVER_OKstatus 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 <[email protected]>CC Jason Cooper <[email protected]>CC Herbert Xu <[email protected]>CC <[email protected]> # v3.15+Signed-off-by Amit Shah <[email protected]>Signed-off-by Jason Cooper <[email protected]>--- drivers/char/hw_random/core.c | 8 +++++--- include/linux/hw_random.h | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-)drivers/char/hw_random/core.c +0
-0@@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)INIT_LIST_HEAD (&rng->list);list_add_tail (&rng->list, &rng_list);
Quoted 12 lines
drivers/char/hw_random/core.c int hwrng_register(struct hwrng *rng) · line 354
Quoted 8 lines
drivers/char/hw_random/core.c +5
-3- 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.
That's a problem outside the scope of this patch. You're basically saying the ->init() should be called unconditionally for each hwrng. If that's what driver authors assumed, that's not what is happening if there is more than one driver in the system.
I think you should be changing the code a few lines up to make sure hwrng_init() is called once for each driver.
Quoted 2 lines
My version was more conservative while this one keeps the bug from the current kernels.
Huh? What do you mean by "keeps the bug from the current kernels." ?
Besides, you're second patch isn't actually doing any ->init to get the hwrng ready for reading... If you had a real ->init function, and it was always called, would rng_get_data() work at probe time for your driver?
confused,
Jason.
Re: [PATCH 1/2 v2] hwrng: Allow drivers to disable reading during probe
In reply to Jason CooperHi Jason,
Quoted 31 lines · Jason Cooper
Commit
d9e7972619334"hwrng: add randomness to system from rng sources" added a call torng_get_data()from thehwrng_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 theDRIVER_OKstatus 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 <[email protected]>CC Jason Cooper <[email protected]>CC Herbert Xu <[email protected]>CC <[email protected]> # v3.15+Signed-off-by Amit Shah <[email protected]>Signed-off-by Jason Cooper <[email protected]>--- drivers/char/hw_random/core.c | 8 +++++--- include/linux/hw_random.h | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-)drivers/char/hw_random/core.c +0
-0@@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)INIT_LIST_HEAD (&rng->list);
drivers/char/hw_random/core.c int hwrng_register(struct hwrng *rng) · line 354
Quoted 10 lines
drivers/char/hw_random/core.c +5
-3list_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
PATCH v2 1/2 hwrng: Allow drivers to disable reading during probe
In reply to Amit ShahCommit 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. ]
--- drivers/char/hw_random/core.c | 8 +++++--- include/linux/hw_random.h | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-)
drivers/char/hw_random/core.c +5 -3
@@ -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:
include/linux/hw_random.h +4 -0
@@ -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. */
Signature · 1 line
2.0.0