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.
Signed-off-by: Alexander Neville <[email protected]>
---
arch/um/drivers/Makefile | 3 +-
arch/um/drivers/random.c | 192 ++++++++-------------------------
drivers/char/hw_random/Kconfig | 21 ++--
3 files changed, 59 insertions(+), 157 deletions(-)
diff --git a/arch/um/drivers/Makefile b/arch/um/drivers/Makefile
index 693319839f69..29b0364f267d 100644
--- a/arch/um/drivers/Makefile
+++ b/arch/um/drivers/Makefile
@@ -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
diff --git a/arch/um/drivers/random.c b/arch/um/drivers/random.c
index 1d5d3057e6f1..7a3099277ebd 100644
--- a/arch/um/drivers/random.c
+++ b/arch/um/drivers/random.c
@@ -1,175 +1,75 @@
-/* Copyright (C) 2005 - 2008 Jeff Dike <jdike@{linux.intel,addtoit}.com> */
-
-/* Much of this ripped from drivers/char/hw_random.c, see there for other
- * copyright.
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * UML Host RNG Driver
+ *
+ * (c) Copright 2019 Alexander Neville <[email protected]>
*
- * This software may be used and distributed according to the terms
- * of the GNU General Public License, incorporated herein by reference.
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
*/
-#include <linux/sched/signal.h>
+
+#include <linux/kernel.h>
+#include <linux/types.h>
#include <linux/module.h>
-#include <linux/fs.h>
-#include <linux/interrupt.h>
-#include <linux/miscdevice.h>
-#include <linux/delay.h>
-#include <linux/uaccess.h>
-#include <init.h>
-#include <irq_kern.h>
+#include <linux/hw_random.h>
+#include <linux/fcntl.h>
#include <os.h>
-/*
- * core module and version information
- */
-#define RNG_VERSION "1.0.0"
-#define RNG_MODULE_NAME "hw_random"
-
-#define RNG_MISCDEV_MINOR 183 /* official */
-
-/* Changed at init time, in the non-modular case, and at module load
- * time, in the module case. Presumably, the module subsystem
- * protects against a module being loaded twice at the same time.
- */
-static int random_fd = -1;
-static DECLARE_WAIT_QUEUE_HEAD(host_read_wait);
-
-static int rng_dev_open (struct inode *inode, struct file *filp)
+static int uml_rng_read(struct hwrng *rng, void *data, size_t bufsize,
+ bool wait)
{
- /* enforce read-only access to this chrdev */
- if ((filp->f_mode & FMODE_READ) == 0)
- return -EINVAL;
- if ((filp->f_mode & FMODE_WRITE) != 0)
- return -EINVAL;
-
- return 0;
+ return os_read_file(rng->priv, data, bufsize);
}
-static atomic_t host_sleep_count = ATOMIC_INIT(0);
-
-static ssize_t rng_dev_read (struct file *filp, char __user *buf, size_t size,
- loff_t *offp)
+static int uml_rng_init(struct hwrng *rng)
{
- u32 data;
- int n, ret = 0, have_data;
-
- while (size) {
- n = os_read_file(random_fd, &data, sizeof(data));
- if (n > 0) {
- have_data = n;
- while (have_data && size) {
- if (put_user((u8) data, buf++)) {
- ret = ret ? : -EFAULT;
- break;
- }
- size--;
- ret++;
- have_data--;
- data >>= 8;
- }
- }
- else if (n == -EAGAIN) {
- DECLARE_WAITQUEUE(wait, current);
-
- if (filp->f_flags & O_NONBLOCK)
- return ret ? : -EAGAIN;
-
- atomic_inc(&host_sleep_count);
- add_sigio_fd(random_fd);
-
- add_wait_queue(&host_read_wait, &wait);
- set_current_state(TASK_INTERRUPTIBLE);
-
- schedule();
- remove_wait_queue(&host_read_wait, &wait);
-
- if (atomic_dec_and_test(&host_sleep_count)) {
- ignore_sigio_fd(random_fd);
- deactivate_fd(random_fd, RANDOM_IRQ);
- }
- }
- else
- return n;
+ int fd = os_open_file("/dev/random", of_read(OPENFLAGS()), O_NONBLOCK);
- if (signal_pending (current))
- return ret ? : -ERESTARTSYS;
+ if (fd < 0) {
+ pr_debug("uml-rng: failed to open /dev/random");
+ return fd;
}
- return ret;
-}
-static const struct file_operations rng_chrdev_ops = {
- .owner = THIS_MODULE,
- .open = rng_dev_open,
- .read = rng_dev_read,
- .llseek = noop_llseek,
-};
+ int err = os_set_fd_async(fd);
-/* rng_init shouldn't be called more than once at boot time */
-static struct miscdevice rng_miscdev = {
- RNG_MISCDEV_MINOR,
- RNG_MODULE_NAME,
- &rng_chrdev_ops,
-};
+ if (err < 0) {
+ os_close_file(fd);
+ return err;
+ }
-static irqreturn_t random_interrupt(int irq, void *data)
-{
- wake_up(&host_read_wait);
+ rng->priv = fd;
- return IRQ_HANDLED;
+ return 0;
}
-/*
- * rng_init - initialize RNG module
- */
-static int __init rng_init (void)
+static void uml_rng_cleanup(struct hwrng *rng)
{
- int err;
-
- err = os_open_file("/dev/random", of_read(OPENFLAGS()), 0);
- if (err < 0)
- goto out;
-
- random_fd = err;
-
- err = um_request_irq(RANDOM_IRQ, random_fd, IRQ_READ, random_interrupt,
- 0, "random", NULL);
- if (err)
- goto err_out_cleanup_hw;
-
- sigio_broken(random_fd, 1);
-
- err = misc_register (&rng_miscdev);
- if (err) {
- printk (KERN_ERR RNG_MODULE_NAME ": misc device register "
- "failed\n");
- goto err_out_cleanup_hw;
- }
-out:
- return err;
-
-err_out_cleanup_hw:
- os_close_file(random_fd);
- random_fd = -1;
- goto out;
+ os_close_file(rng->priv);
}
-/*
- * rng_cleanup - shutdown RNG module
- */
-static void cleanup(void)
+static struct hwrng uml_rng_ops = {
+ .name = "uml-rng",
+ .init = uml_rng_init,
+ .cleanup = uml_rng_cleanup,
+ .read = uml_rng_read,
+ .quality = 1024
+};
+
+static int __init uml_rng_mod_init(void)
{
- free_irq_by_fd(random_fd);
- os_close_file(random_fd);
+ return hwrng_register(¨_rng_ops);
}
-static void __exit rng_cleanup(void)
+static void __exit uml_rng_mod_exit(void)
{
- os_close_file(random_fd);
- misc_deregister (&rng_miscdev);
+ hwrng_unregister(¨_rng_ops);
}
-module_init (rng_init);
-module_exit (rng_cleanup);
-__uml_exitcall(cleanup);
+module_init(uml_rng_mod_init);
+module_exit(uml_rng_mod_exit);
-MODULE_DESCRIPTION("UML Host Random Number Generator (RNG) driver");
+MODULE_AUTHOR("Alexander Neville <[email protected]>");
+MODULE_DESCRIPTION("UML Host RNG Driver");
MODULE_LICENSE("GPL");
diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 59f25286befe..762acbdd52ce 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -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
--
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.
On Thu, Aug 29, 2019 at 3:45 AM Alexander Neville <[email protected]> wrote:
>
> 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.
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?
--
Thanks,
//richard
On Wed, 2019-08-28 at 21:44 -0400, Alexander Neville wrote:
> 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.
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
On Thu, 29 Aug 2019 15:26:24 +0200, Richard Weinberger <[email protected]> wrote:
> So, you removed -EAGAIN handling, 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.
> 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.
----- Ursprüngliche Mail -----
> 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:
>> So, you removed -EAGAIN handling, 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.
I got this, and this is a good thing!
> 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?
> 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.
>
>> 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.
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
> Well, it does not block but passing -EAGAIN directly back is not nice.
> Or does the hw_random framework 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.)
> 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.
Most of the old code was pulled from the framework anyway so it's very
unlikely that anything else would be dropped here.
On Thu, 29 Aug 2019 17:30:59 +0200 (CEST), Richard Weinberger <[email protected]> wrote:
> ----- Ursprüngliche Mail -----
> > 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:
> >> So, you removed -EAGAIN handling, 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.
>
> I got this, and this is a good thing!
>
> > 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?
>
> > 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.
> >
> >> 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.
>
> 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
On Thu, 2019-08-29 at 13:10 -0400, Dark wrote:
> (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
[...]
Signed-off-by: ...
---
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