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 <[email protected]>
---
drivers/char/hw_random/core.c | 155 +++++++++++++++++++++++++++---------------
include/linux/hw_random.h | 2 +
2 files changed, 104 insertions(+), 53 deletions(-)
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;
};
--
2.8.1
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 <[email protected]>
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,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]>
>
> 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.
Jason Cooper <[email protected]> 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.
--
-keith
Hi Keith,
On Tue, Aug 09, 2016 at 10:58:05AM -0700, Keith Packard wrote:
> Jason Cooper <[email protected]> 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.
Jason Cooper <[email protected]> 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
--
-keith
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 Holschuh
Henrique de Moraes Holschuh <[email protected]> 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.
--
-keith