| Message ID | 20190828204609.02a7ff70@TheDarkness |
|---|---|
| State | Rejected |
| Headers | show |
| Series | um: Rewrite host RNG driver. | expand |
On Thu, Aug 29, 2019 at 3:45 AM Alexander Neville <dark@volatile.bz> 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?
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 <richard.weinberger@gmail.com> 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" <dark@volatile.bz> > An: "Richard Weinberger" <richard.weinberger@gmail.com>, "linux-kernel" <linux-kernel@vger.kernel.org> > CC: "richard" <richard@nod.at>, "anton ivanov" <anton.ivanov@cambridgegreys.com>, "linux-um" > <linux-um@lists.infradead.org> > 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 > <richard.weinberger@gmail.com> 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 <richard@nod.at> wrote: > ----- Ursprüngliche Mail ----- > > Von: "Dark" <dark@volatile.bz> > > An: "Richard Weinberger" <richard.weinberger@gmail.com>, "linux-kernel" <linux-kernel@vger.kernel.org> > > CC: "richard" <richard@nod.at>, "anton ivanov" <anton.ivanov@cambridgegreys.com>, "linux-um" > > <linux-um@lists.infradead.org> > > 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 > > <richard.weinberger@gmail.com> 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
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 <dark@volatile.bz> * - * 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 <dark@volatile.bz>"); +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
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 <dark@volatile.bz> --- arch/um/drivers/Makefile | 3 +- arch/um/drivers/random.c | 192 ++++++++------------------------- drivers/char/hw_random/Kconfig | 21 ++-- 3 files changed, 59 insertions(+), 157 deletions(-)