um: Rewrite host RNG driver.
Subsystems (2)
PATCH 1/1 um: Rewrite host RNG driver.
The old driver had a bug that would cause it to outright stop working if the host's /dev/random were to block. Instead of trying to track down the cause of said bug, rewriting it from scratch turned out to be a much better option as it came with a few benefits:
- The new driver properly registers itself as an hardware RNG.
- The code is simpler and therefore easier to maintain.
- It serves as a minimal example of writing a hardware RNG driver.
I also edited the Kconfig symbol to bring it up to more modern standards.
--- arch/um/drivers/Makefile | 3 +- arch/um/drivers/random.c | 192 ++++++++------------------------- drivers/char/hw_random/Kconfig | 21 ++-- 3 files changed, 59 insertions(+), 157 deletions(-)
arch/um/drivers/Makefile +2 -1
@@ -17,6 +17,7 @@ hostaudio-objs := hostaudio_kern.o ubd-objs := ubd_kern.o ubd_user.o port-objs := port_kern.o port_user.o harddog-objs := harddog_kern.o harddog_user.o +uml-rng-objs := random.o LDFLAGS_pcap.o := -r $(shell $(CC) $(KBUILD_CFLAGS) -print-file-name=libpcap.a)
@@ -60,7 +61,7 @@ obj-$(CONFIG_TTY_CHAN) += tty.o obj-$(CONFIG_XTERM_CHAN) += xterm.o xterm_kern.o obj-$(CONFIG_UML_WATCHDOG) += harddog.o obj-$(CONFIG_BLK_DEV_COW_COMMON) += cow_user.o -obj-$(CONFIG_UML_RANDOM) += random.o +obj-$(CONFIG_UML_RANDOM) += uml-rng.o # pcap_user.o must be added explicitly. USER_OBJS := fd.o null.o pty.o tty.o xterm.o slip_common.o pcap_user.o vde_user.o vector_user.o
arch/um/drivers/random.c +46 -146
221 lines not on this page · raw message
drivers/char/hw_random/Kconfig +11 -10
@@ -440,22 +440,23 @@ config HW_RANDOM_OPTEE If unsure, say Y. -endif # HW_RANDOM - config UML_RANDOM + tristate "UML Host Random Number Generator Support" depends on UML - tristate "Hardware random number generator" + default HW_RANDOM help This option enables UML's "hardware" random number generator. It attaches itself to the host's /dev/random, supplying as much entropy as the host has, rather than the small amount the UML gets from its - own drivers. It registers itself as a standard hardware random number - generator, major 10, minor 183, and the canonical device name is - /dev/hwrng. - The way to make use of this is to install the rng-tools package - (check your distro, or download from - http://sourceforge.net/projects/gkernel/). rngd periodically reads - /dev/hwrng and injects the entropy into /dev/random. + own drivers. + + To compile this driver as a moudle, choose M here: the module + will be called uml-rng + + If unsure, say Y. + +endif # HW_RANDOM + config HW_RANDOM_KEYSTONE depends on ARCH_KEYSTONE
Signature · 11 lines
2.23.0 Sorry about the diff being a bit messy. If desired, I can manually fix the diff. Also, please let me know if my mail client decided to mangle my patch. _______________________________________________ linux-um mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-um
Quoted 6 lines · Alexander Neville
The old driver had a bug that would cause it to outright stop working if the host's
/dev/randomwere to block. Instead of trying to track down the cause of said bug, rewriting it from scratch turned out to be a much better option as it came with a few benefits:
Quoted 8 lines
- The new driver properly registers itself as an hardware RNG.
- The code is simpler and therefore easier to maintain.
- It serves as a minimal example of writing a hardware RNG driver.
I also edited the
Kconfigsymbol to bring it up to more modern standards.
So, you removed -EAGAIN handling, made everything synchronous, and changed the interface.
I'm not sure if this really a much better option.
Rewriting the driver in a modern manner is a good thing, but throwing the old one way with a little hand weaving just because of a unspecified issue is a little harsh.
Can you at lest provide more infos what problem you're facing with the old driver?
Signature · 7 lines
Thanks, //richard _______________________________________________ linux-um mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-um
Quoted 3 lines · Richard Weinberger
So, you removed
-EAGAINhandling, made everything synchronous, and changed the interface.t
I'm not sure if this really a much better option.
I should have been more clear here that I'm using the interfaces provided by drivers/char/hw_random/core.c for consistency with the other hardware RNG drivers and to avoid reimplementing stuff that's already there.
It might be a bit hard to see in the diff, but I pass the file descriptor to os_set_fd_async() to prevent it from blocking.
For the -EAGAIN handling, I'm passing it onto the caller. Since you mentioned it, It would be better to handle it in the driver itself so I'll update the patch to address that.
Quoted 5 lines
Rewriting the driver in a modern manner is a good thing, but throwing the old one way with a little hand weaving just because of a unspecified issue is a little harsh.
Can you at lest provide more infos what problem you're facing with the old driver?
Most of it boiled down to it silently breaking if /dev/random on the host were to block for any reason, and there was the userspace tool requirement to properly make use of it. With that said, the interface was also inconsistent with the other hardware RNG drivers which would require a rewrite to address anyway.
Richard Weinberger
maintainer----- Ursprüngliche Mail -----
Quoted 9 lines
Von: "Dark" <[email protected]>
An: "Richard Weinberger" <[email protected]>, "linux-kernel" <[email protected]>
CC: "richard" <[email protected]>, "anton ivanov" <[email protected]>, "linux-um" <[email protected]>
Gesendet: Donnerstag, 29. August 2019 16:36:28
Betreff: Re: [PATCH] um: Rewrite host RNG driver.On Thu, 29 Aug 2019 15:26:24 +0200, Richard Weinberger <[email protected]> wrote:
Quoted 8 lines
Quoted 3 lines
So, you removed
-EAGAINhandling, made everything synchronous, and changed the interface.t
I'm not sure if this really a much better option.I should have been more clear here that I'm using the interfaces provided by
drivers/char/hw_random/core.cfor consistency with the other hardware RNG drivers and to avoid reimplementing stuff that's already there.
I got this, and this is a good thing!
Quoted 2 lines
It might be a bit hard to see in the diff, but I pass the file descriptor to
os_set_fd_async()to prevent it from blocking.
Well, it does not block but passing -EAGAIN directly back is not nice. Or does the hw_random framework handle this?
Quoted 10 lines
For the
-EAGAINhandling, I'm passing it onto the caller. Since you mentioned it, It would be better to handle it in the driver itself so I'll update the patch to address that.Quoted 5 lines
Rewriting the driver in a modern manner is a good thing, but throwing the old one way with a little hand weaving just because of a unspecified issue is a little harsh.
Can you at lest provide more infos what problem you're facing with the old driver?
Quoted 5 lines
Most of it boiled down to it silently breaking if
/dev/randomon the host were to block for any reason, and there was the userspace tool requirement to properly make use of it. With that said, the interface was also inconsistent with the other hardware RNG drivers which would require a rewrite to address anyway.
Maybe our -EAGAIN handling is buggy.
That said I'm all for changing the driver to use the right framework but please make sure that we don't drop useful stuff like -EAGAIN handling.
Thanks,
//richard
Quoted 2 lines
Well, it does not block but passing
-EAGAINdirectly back is not nice. Or does thehw_randomframework handle this?
The framework is passing -EAGAIN to userspace which isn't very nice at all. Luckily, handling it is pretty trival so I went ahead and made an updated patch to address this. (I'm unsure of how to submit an update to my patch so I'll need a bit of guidence on this.)
Quoted 3 lines
Maybe our
-EAGAINhandling is buggy.
That said I'm all for changing the driver to use the right framework but please make sure that we don't drop useful stuff like-EAGAINhandling.
Most of the old code was pulled from the framework anyway so it's very unlikely that anything else would be dropped here.
Quoted 49 lines · Richard Weinberger
----- Ursprüngliche Mail -----
Quoted 9 lines
Von: "Dark" <[email protected]>
An: "Richard Weinberger" <[email protected]>, "linux-kernel" <[email protected]>
CC: "richard" <[email protected]>, "anton ivanov" <[email protected]>, "linux-um" <[email protected]>
Gesendet: Donnerstag, 29. August 2019 16:36:28
Betreff: Re: [PATCH] um: Rewrite host RNG driver.On Thu, 29 Aug 2019 15:26:24 +0200, Richard Weinberger <[email protected]> wrote:
Quoted 8 lines
Quoted 3 lines
So, you removed
-EAGAINhandling, made everything synchronous, and changed the interface.t
I'm not sure if this really a much better option.I should have been more clear here that I'm using the interfaces provided by
drivers/char/hw_random/core.cfor consistency with the other hardware RNG drivers and to avoid reimplementing stuff that's already there.I got this, and this is a good thing!
Quoted 2 lines
It might be a bit hard to see in the diff, but I pass the file descriptor to
os_set_fd_async()to prevent it from blocking.Well, it does not block but passing
-EAGAINdirectly back is not nice. Or does thehw_randomframework handle this?Quoted 10 lines
For the
-EAGAINhandling, I'm passing it onto the caller. Since you mentioned it, It would be better to handle it in the driver itself so I'll update the patch to address that.Quoted 5 lines
Rewriting the driver in a modern manner is a good thing, but throwing the old one way with a little hand weaving just because of a unspecified issue is a little harsh.
Can you at lest provide more infos what problem you're facing with the old driver?Quoted 5 lines
Most of it boiled down to it silently breaking if
/dev/randomon the host were to block for any reason, and there was the userspace tool requirement to properly make use of it. With that said, the interface was also inconsistent with the other hardware RNG drivers which would require a rewrite to address anyway.Maybe our
-EAGAINhandling is buggy.
That said I'm all for changing the driver to use the right framework but please make sure that we don't drop useful stuff like-EAGAINhandling.Thanks,
//richard
Johannes Berg
maintainerQuoted 2 lines · Dark
(I'm unsure of how to submit an update
to my patch so I'll need a bit of guidence on this.)
Resend the patch, but use --subject-prefix 'PATCH v2' and ideally note somewhere in the patch (possibly after a "---" marker after Signed-off- by) the changes between the versions, like
---
v2:
* fix -EAGAIN handling
[...]
Hmm, but looks like you didn't actually use git send-email directly, so I guess just put "[PATCH v2]" manually.
johannes
_______________________________________________
linux-um mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-um
Johannes Berg
maintainerQuoted 5 lines · Alexander Neville
The old driver had a bug that would cause it to outright stop working if the host's
/dev/randomwere to block. Instead of trying to track down the cause of said bug, rewriting it from scratch turned out to be a much better option as it came with a few benefits:
Quoted 8 lines
- The new driver properly registers itself as an hardware RNG.
- The code is simpler and therefore easier to maintain.
- It serves as a minimal example of writing a hardware RNG driver.
I also edited the
Kconfigsymbol to bring it up to more modern standards.
I realize that it's still easier to use, but theoretically you could just use the virtio implementation, and use that driver with a suitable small vhost device implementation. :-)
johannes