| Message ID | 1469477255-26824-1-git-send-email-keithp@keithp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
On Mon, Jul 25, 2016 at 01:07:35PM -0700, Keith Packard wrote: > Instead of having only one hwrng feeding /dev/random at a time, maintain > a list of devices and cycle between them when filling the entropy pool. > > Signed-off-by: Keith Packard <keithp@keithp.com> So you're cycling RNGs even for user-space reads? That could be problematic because not all hardware RNGs carry the maximum amount of entropy. It would be rather annoying to be cycling between RNGs of different qualities. Perhaps only cycle for the kernel hwrngd? Thanks,
Hi Keith, Herbert, On Tue, Aug 09, 2016 at 05:50:58PM +0800, Herbert Xu wrote: > On Mon, Jul 25, 2016 at 01:07:35PM -0700, Keith Packard wrote: > > Instead of having only one hwrng feeding /dev/random at a time, maintain > > a list of devices and cycle between them when filling the entropy pool. > > > > Signed-off-by: Keith Packard <keithp@keithp.com> > > So you're cycling RNGs even for user-space reads? That could be > problematic because not all hardware RNGs carry the maximum amount > of entropy. It would be rather annoying to be cycling between > RNGs of different qualities. > > Perhaps only cycle for the kernel hwrngd? Perhaps a /dev/hwrng[0-9] per rng? That would lend itself nicely to a sysfs interface for per device quality, rate, and enabled attributes. e.g. /sys/class/hw_random/hwrng0/{device/,quality,rate,enabled} /dev/hwrng could pull from the one with the highest quality, or user specified for backwards compatibility. thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jason Cooper <jason@lakedaemon.net> writes: > Perhaps a /dev/hwrng[0-9] per rng? That would lend itself nicely to a > sysfs interface for per device quality, rate, and enabled attributes. > e.g. /sys/class/hw_random/hwrng0/{device/,quality,rate,enabled} I was interested in the data being provided for /dev/random; that seems like the most important interface to me. But, exposing all of the devices using consistent names does seem like a useful idea at some level. > /dev/hwrng could pull from the one with the highest quality, or user > specified for backwards compatibility. I like the notion of using all of them in turn; if one of them turns out to be broken, you're still stirring in data from the others. After all, the quality metric is provided by the device, we aren't doing any analysis on the data to determine it independently.
Hi Keith, On Tue, Aug 09, 2016 at 10:58:05AM -0700, Keith Packard wrote: > Jason Cooper <jason@lakedaemon.net> writes: > > Perhaps a /dev/hwrng[0-9] per rng? That would lend itself nicely to a > > sysfs interface for per device quality, rate, and enabled attributes. > > e.g. /sys/class/hw_random/hwrng0/{device/,quality,rate,enabled} > > I was interested in the data being provided for /dev/random; that seems > like the most important interface to me. Me too, agreed. > But, exposing all of the devices using consistent names does seem like > a useful idea at some level. On another thread, regarding the ath9k-rng (actually just the adc registers), Henrique asked about per-source knobs. My suggestion follows from that. > > /dev/hwrng could pull from the one with the highest quality, or user > > specified for backwards compatibility. > > I like the notion of using all of them in turn; if one of them turns out > to be broken, you're still stirring in data from the others. After all, > the quality metric is provided by the device, we aren't doing any > analysis on the data to determine it independently. Sure, but /dev/hwrng is a user interface. Typically to rngd, but not necessarily. We need to make sure it's behavior is consistent with existing expectations. We shouldn't attach first-probed to /dev/hwrng, because that may not be what the user is expecting. If I bought a raw entropy source, and knew nothing of the proposed multi-source interfaces, I'd expect the USB dongle to be attached to /dev/hwrng. Despite the fact that my pcie wifi card was probed first and has adc registers providing an entropy source. I'm not sure how we ensure that. Perhaps an 'environmental' flag in the hw_random source attributes? Or a 'not-designed-to-be-an-rng' flag? :) Maybe those would be /dev/envrng[0-9]... thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jason Cooper <jason@lakedaemon.net> writes: > On another thread, regarding the ath9k-rng (actually just the adc > registers), Henrique asked about per-source knobs. My suggestion > follows from that. I'd do that with the source-specific driver instead of attempting to route controls through hwrng. Anything else seems like 'ioctl' to me. > Sure, but /dev/hwrng is a user interface. Typically to rngd, but not > necessarily. We need to make sure it's behavior is consistent with > existing expectations. Hrm. Maybe /dev/hwrng should use a different policy than how we feed /dev/random -- we could use the existing behaviour for /dev/hwrng, but use a round-robin for /dev/random. That way, the latest device would always end up in /dev/hwrng (unless configured otherwise), and we'd still use all of the available sources to help stir the kernel entropy pool. > We shouldn't attach first-probed to /dev/hwrng, because that may not be > what the user is expecting. If I bought a raw entropy source, and knew > nothing of the proposed multi-source interfaces, I'd expect the USB > dongle to be attached to /dev/hwrng. Despite the fact that my pcie wifi > card was probed first and has adc registers providing an entropy source. That seems like a fragile interface as it depends on discovery order, but it is what we have currently. The chaoskey driver also exposes it's own device; that provides a simple way to ensure that the application is getting bits from the desired entropy source. > I'm not sure how we ensure that. Perhaps an 'environmental' flag in the > hw_random source attributes? Or a 'not-designed-to-be-an-rng' flag? :) > Maybe those would be /dev/envrng[0-9]... Or some set of query ioctls on /dev/hwrng[0-9]+ that would provide information about the capabilities of the underlying device. There are lots of things we could do, I guess the question I have is how much of this would applications actually use effectively? You're probably right that /dev/hwrng should point at a single source and not change though; otherwise figuring out what the quality of the bits you're getting isn't possible.x
On Tue, 09 Aug 2016, Jason Cooper wrote: > Perhaps a /dev/hwrng[0-9] per rng? That would lend itself nicely to a > sysfs interface for per device quality, rate, and enabled attributes. > e.g. /sys/class/hw_random/hwrng0/{device/,quality,rate,enabled} IMHO, this is mightly annoying to use from inside a rngd-like utility in a race-free, safe way. It looks to me that ioctl() would be a much better interface for everything but the "enabled" functionality (which should be reported to the rngd-like utility as open() on the real device failing with, e.g., ENXIO, when that source is disabled).
Henrique de Moraes Holschuh <hmh@hmh.eng.br> writes: > IMHO, this is mightly annoying to use from inside a rngd-like utility in > a race-free, safe way. It looks to me that ioctl() would be a much > better interface for everything but the "enabled" functionality (which > should be reported to the rngd-like utility as open() on the real device > failing with, e.g., ENXIO, when that source is disabled). What information does an rngd-like program actually want? All I can think that it would need is the stream of random data. I guess some estimate of the entropy available would be nice, but surely it would want to verify that in any case.
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c index 9203f2d..97fd4a7 100644 --- a/drivers/char/hw_random/core.c +++ b/drivers/char/hw_random/core.c @@ -51,10 +51,10 @@ #define RNG_MISCDEV_MINOR 183 /* official */ -static struct hwrng *current_rng; +static LIST_HEAD(active_rng); static struct task_struct *hwrng_fill; static LIST_HEAD(rng_list); -/* Protects rng_list and current_rng */ +/* Protects rng_list and active_rng */ static DEFINE_MUTEX(rng_mutex); /* Protects rng read functions, data_avail, rng_buffer and rng_fillbuf */ static DEFINE_MUTEX(reading_mutex); @@ -70,7 +70,6 @@ module_param(default_quality, ushort, 0644); MODULE_PARM_DESC(default_quality, "default entropy content of hwrng per mill"); -static void drop_current_rng(void); static int hwrng_init(struct hwrng *rng); static void start_khwrngd(void); @@ -104,31 +103,65 @@ static inline void cleanup_rng(struct kref *kref) complete(&rng->cleanup_done); } -static int set_current_rng(struct hwrng *rng) +static unsigned short rng_quality(struct hwrng *rng) +{ + unsigned short quality; + + quality = current_quality; + if (!quality) { + quality = rng->quality; + if (!quality) + quality = default_quality; + } + if (quality > 1024) + quality = 1024; + return quality; +} + +static int add_active_rng(struct hwrng *rng) { int err; BUG_ON(!mutex_is_locked(&rng_mutex)); + if (rng->is_active) + return 0; + err = hwrng_init(rng); if (err) return err; - drop_current_rng(); - current_rng = rng; + if (rng_quality(rng) != 0) { + rng->is_active = true; + list_add(&rng->active, &active_rng); + if (!hwrng_fill) + start_khwrngd(); + } return 0; } -static void drop_current_rng(void) +static int drop_active_rng(struct hwrng *rng) { BUG_ON(!mutex_is_locked(&rng_mutex)); - if (!current_rng) - return; + if (!rng->is_active) + return 0; + + list_del(&rng->active); + rng->is_active = false; /* decrease last reference for triggering the cleanup */ - kref_put(¤t_rng->ref, cleanup_rng); - current_rng = NULL; + kref_put(&rng->ref, cleanup_rng); + + if (list_empty(&active_rng) && hwrng_fill) + kthread_stop(hwrng_fill); + + return 0; +} + +static struct hwrng *first_active_rng(void) +{ + return list_first_entry_or_null(&active_rng, struct hwrng, active); } /* Returns ERR_PTR(), NULL or refcounted hwrng */ @@ -139,18 +172,32 @@ static struct hwrng *get_current_rng(void) if (mutex_lock_interruptible(&rng_mutex)) return ERR_PTR(-ERESTARTSYS); - rng = current_rng; + rng = first_active_rng(); if (rng) kref_get(&rng->ref); mutex_unlock(&rng_mutex); + return rng; } +static int next_rng(void) +{ + if (mutex_lock_interruptible(&rng_mutex)) + return -ERESTARTSYS; + + if (!list_empty(&active_rng) && !list_is_singular(&active_rng)) + list_rotate_left(&active_rng); + + mutex_unlock(&rng_mutex); + + return 0; +} + static void put_rng(struct hwrng *rng) { /* - * Hold rng_mutex here so we serialize in case they set_current_rng + * Hold rng_mutex here so we serialize in case they add_active_rng * on rng again immediately. */ mutex_lock(&rng_mutex); @@ -178,15 +225,6 @@ static int hwrng_init(struct hwrng *rng) skip_init: add_early_randomness(rng); - current_quality = rng->quality ? : default_quality; - if (current_quality > 1024) - current_quality = 1024; - - if (current_quality == 0 && hwrng_fill) - kthread_stop(hwrng_fill); - if (current_quality > 0 && !hwrng_fill) - start_khwrngd(); - return 0; } @@ -246,6 +284,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, bytes_read = rng_get_data(rng, rng_buffer, rng_buffer_size(), !(filp->f_flags & O_NONBLOCK)); + next_rng(); if (bytes_read < 0) { err = bytes_read; goto out_unlock_reading; @@ -320,20 +359,30 @@ static ssize_t hwrng_attr_current_store(struct device *dev, const char *buf, size_t len) { int err; + int add = 1; struct hwrng *rng; err = mutex_lock_interruptible(&rng_mutex); if (err) return -ERESTARTSYS; + if (*buf == '-') { + add = 0; + buf++; + } else if (*buf == '+') { + add = 1; + buf++; + } err = -ENODEV; list_for_each_entry(rng, &rng_list, list) { if (sysfs_streq(rng->name, buf)) { - err = 0; - if (rng != current_rng) - err = set_current_rng(rng); + if (add) + err = add_active_rng(rng); + else + err = drop_active_rng(rng); break; } } + mutex_unlock(&rng_mutex); return err ? : len; @@ -343,17 +392,26 @@ static ssize_t hwrng_attr_current_show(struct device *dev, struct device_attribute *attr, char *buf) { - ssize_t ret; + int err; struct hwrng *rng; - rng = get_current_rng(); - if (IS_ERR(rng)) - return PTR_ERR(rng); - - ret = snprintf(buf, PAGE_SIZE, "%s\n", rng ? rng->name : "none"); - put_rng(rng); + err = mutex_lock_interruptible(&rng_mutex); + if (err) + return -ERESTARTSYS; + buf[0] = '\0'; + if (list_empty(&active_rng)) { + strlcat(buf, "none", PAGE_SIZE); + } else { + list_for_each_entry(rng, &active_rng, active) { + if (rng != first_active_rng()) + strlcat(buf, " ", PAGE_SIZE); + strlcat(buf, rng->name, PAGE_SIZE); + } + } + strlcat(buf, "\n", PAGE_SIZE); + mutex_unlock(&rng_mutex); - return ret; + return strlen(buf); } static ssize_t hwrng_attr_available_show(struct device *dev, @@ -412,6 +470,7 @@ static int hwrng_fillfn(void *unused) rng = get_current_rng(); if (IS_ERR(rng) || !rng) break; + next_rng(); mutex_lock(&reading_mutex); rc = rng_get_data(rng, rng_fillbuf, rng_buffer_size(), 1); @@ -442,7 +501,7 @@ static void start_khwrngd(void) int hwrng_register(struct hwrng *rng) { int err = -EINVAL; - struct hwrng *old_rng, *tmp; + struct hwrng *tmp; if (rng->name == NULL || (rng->data_read == NULL && rng->read == NULL)) @@ -472,19 +531,18 @@ int hwrng_register(struct hwrng *rng) goto out_unlock; } + rng->is_active = false; + init_completion(&rng->cleanup_done); complete(&rng->cleanup_done); - old_rng = current_rng; - err = 0; - if (!old_rng) { - err = set_current_rng(rng); - if (err) - goto out_unlock; - } + err = add_active_rng(rng); + if (err) + goto out_unlock; + list_add_tail(&rng->list, &rng_list); - if (old_rng && !rng->init) { + if (!rng->init) { /* * Use a new device's input to add some randomness to * the system. If this rng device isn't going to be @@ -507,16 +565,7 @@ void hwrng_unregister(struct hwrng *rng) mutex_lock(&rng_mutex); list_del(&rng->list); - if (current_rng == rng) { - drop_current_rng(); - if (!list_empty(&rng_list)) { - struct hwrng *tail; - - tail = list_entry(rng_list.prev, struct hwrng, list); - - set_current_rng(tail); - } - } + drop_active_rng(rng); if (list_empty(&rng_list)) { mutex_unlock(&rng_mutex); @@ -579,7 +628,7 @@ static int __init hwrng_modinit(void) static void __exit hwrng_modexit(void) { mutex_lock(&rng_mutex); - BUG_ON(current_rng); + WARN_ON(!list_empty(&active_rng)); kfree(rng_buffer); kfree(rng_fillbuf); mutex_unlock(&rng_mutex); diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h index 4f7d8f4..c655563 100644 --- a/include/linux/hw_random.h +++ b/include/linux/hw_random.h @@ -45,7 +45,9 @@ struct hwrng { unsigned short quality; /* internal. */ + bool is_active; struct list_head list; + struct list_head active; struct kref ref; struct completion cleanup_done; };
Instead of having only one hwrng feeding /dev/random at a time, maintain a list of devices and cycle between them when filling the entropy pool. Signed-off-by: Keith Packard <keithp@keithp.com> --- drivers/char/hw_random/core.c | 155 +++++++++++++++++++++++++++--------------- include/linux/hw_random.h | 2 + 2 files changed, 104 insertions(+), 53 deletions(-)