Re: [PATCH 2/5] hw_random: use reference counts on each struct hwrng.
On Thu, Sep 18, 2014 at 12:18:23PM +0930, Rusty Russell wrote:
current_rng holds one reference, and we bump it every time we want
to do a read from it.
This means we only hold the rng_mutex to grab or drop a reference,
so accessing /sys/devices/virtual/misc/hw_random/rng_current doesn't
block on read of /dev/hwrng.
Using a kref is overkill (we're always under the rng_mutex), but
a standard pattern.
This also solves the problem that the hwrng_fillfn thread was
accessing current_rng without a lock, which could change (eg. to NULL)
underneath it.
Hi Rusty,
Signed-off-by: Rusty Russell ru...@rustcorp.com.au
---
drivers/char/hw_random/core.c | 135
--
include/linux/hw_random.h | 2 +
2 files changed, 94 insertions(+), 43 deletions(-)
...
static int rng_dev_open(struct inode *inode, struct file *filp)
{
/* enforce read-only access to this chrdev */
@@ -154,21 +202,22 @@ static ssize_t rng_dev_read(struct file *filp, char
__user *buf,
ssize_t ret = 0;
int err = 0;
int bytes_read, len;
+ struct hwrng *rng;
while (size) {
- if (mutex_lock_interruptible(rng_mutex)) {
- err = -ERESTARTSYS;
+ rng = get_current_rng();
+ if (IS_ERR(rng)) {
+ err = PTR_ERR(rng);
goto out;
}
-
- if (!current_rng) {
+ if (!rng) {
err = -ENODEV;
- goto out_unlock;
+ goto out;
}
mutex_lock(reading_mutex);
if (!data_avail) {
- bytes_read = rng_get_data(current_rng, rng_buffer,
+ bytes_read = rng_get_data(rng, rng_buffer,
rng_buffer_size(),
!(filp-f_flags O_NONBLOCK));
if (bytes_read 0) {
@@ -200,7 +249,6 @@ static ssize_t rng_dev_read(struct file *filp, char
__user *buf,
ret += len;
}
- mutex_unlock(rng_mutex);
mutex_unlock(reading_mutex);
if (need_resched())
@@ -210,15 +258,16 @@ static ssize_t rng_dev_read(struct file *filp, char
__user *buf,
err = -ERESTARTSYS;
We need put_rng() in this error path. Otherwise, unhotplug will hang
in the end of hwrng_unregister()
|/* Just in case rng is reading right now, wait. */
|wait_event(rng_done, atomic_read(rng-ref.refcount) == 0);
Steps to reproduce the hang:
guest) # dd if=/dev/hwrng of=/dev/null
cancel dd process after 10 seconds
guest) # dd if=/dev/hwrng of=/dev/null
hotunplug rng device from qemu monitor
result: device can't be removed (still can find in QEMU monitor)
diff --git a/drivers/char/hw_random/core.c
b/drivers/char/hw_random/core.c
index 96fa067..4e22d70 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -258,6 +258,7 @@ static ssize_t rng_dev_read(struct file *filp,
char __user *buf,
if (signal_pending(current)) {
err = -ERESTARTSYS;
+ put_rng(rng);
goto out;
}
goto out;
}
+
+ put_rng(rng);
}
out:
return ret ? : err;
-out_unlock:
- mutex_unlock(rng_mutex);
- goto out;
+
out_unlock_reading:
mutex_unlock(reading_mutex);
- goto out_unlock;
+ put_rng(rng);
+ goto out;
}
@@ -257,8 +306,8 @@ static ssize_t hwrng_attr_current_store(struct device
*dev,
err = hwrng_init(rng);
if (err)
break;
- hwrng_cleanup(current_rng);
- current_rng = rng;
+ drop_current_rng();
+ set_current_rng(rng);
err = 0;
break;
}
@@ -272,17 +321,15 @@ static ssize_t hwrng_attr_current_show(struct device
*dev,
struct device_attribute *attr,
char *buf)
{
- int err;
ssize_t ret;
- const char *name = none;
+ struct hwrng *rng;
- err = mutex_lock_interruptible(rng_mutex);
- if (err)
- return -ERESTARTSYS;
- if (current_rng)
- name = current_rng-name;
- ret = snprintf(buf, PAGE_SIZE, %s\n, name);
- mutex_unlock(rng_mutex);
+ 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);
return ret;
}
@@ -357,12 +404,16 @@ static int hwrng_fillfn(void *unused)
long rc;
while (!kthread_should_stop()) {
-
[PATCH 2/5] hw_random: use reference counts on each struct hwrng.
current_rng holds one reference, and we bump it every time we want
to do a read from it.
This means we only hold the rng_mutex to grab or drop a reference,
so accessing /sys/devices/virtual/misc/hw_random/rng_current doesn't
block on read of /dev/hwrng.
Using a kref is overkill (we're always under the rng_mutex), but
a standard pattern.
This also solves the problem that the hwrng_fillfn thread was
accessing current_rng without a lock, which could change (eg. to NULL)
underneath it.
Signed-off-by: Rusty Russell ru...@rustcorp.com.au
---
drivers/char/hw_random/core.c | 135 --
include/linux/hw_random.h | 2 +
2 files changed, 94 insertions(+), 43 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index b1b6042ad85c..dc9092a1075d 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -42,6 +42,7 @@
#include linux/delay.h
#include linux/slab.h
#include linux/random.h
+#include linux/err.h
#include asm/uaccess.h
@@ -91,6 +92,59 @@ static void add_early_randomness(struct hwrng *rng)
add_device_randomness(bytes, bytes_read);
}
+static inline void cleanup_rng(struct kref *kref)
+{
+ struct hwrng *rng = container_of(kref, struct hwrng, ref);
+
+ if (rng-cleanup)
+ rng-cleanup(rng);
+}
+
+static void set_current_rng(struct hwrng *rng)
+{
+ BUG_ON(!mutex_is_locked(rng_mutex));
+ kref_get(rng-ref);
+ current_rng = rng;
+}
+
+static void drop_current_rng(void)
+{
+ BUG_ON(!mutex_is_locked(rng_mutex));
+ if (!current_rng)
+ return;
+
+ kref_put(current_rng-ref, cleanup_rng);
+ current_rng = NULL;
+}
+
+/* Returns ERR_PTR(), NULL or refcounted hwrng */
+static struct hwrng *get_current_rng(void)
+{
+ struct hwrng *rng;
+
+ if (mutex_lock_interruptible(rng_mutex))
+ return ERR_PTR(-ERESTARTSYS);
+
+ rng = current_rng;
+ if (rng)
+ kref_get(rng-ref);
+
+ mutex_unlock(rng_mutex);
+ return rng;
+}
+
+static void put_rng(struct hwrng *rng)
+{
+ /*
+* Hold rng_mutex here so we serialize in case they set_current_rng
+* on rng again immediately.
+*/
+ mutex_lock(rng_mutex);
+ if (rng)
+ kref_put(rng-ref, cleanup_rng);
+ mutex_unlock(rng_mutex);
+}
+
static inline int hwrng_init(struct hwrng *rng)
{
if (rng-init) {
@@ -113,12 +167,6 @@ static inline int hwrng_init(struct hwrng *rng)
return 0;
}
-static inline void hwrng_cleanup(struct hwrng *rng)
-{
- if (rng rng-cleanup)
- rng-cleanup(rng);
-}
-
static int rng_dev_open(struct inode *inode, struct file *filp)
{
/* enforce read-only access to this chrdev */
@@ -154,21 +202,22 @@ static ssize_t rng_dev_read(struct file *filp, char
__user *buf,
ssize_t ret = 0;
int err = 0;
int bytes_read, len;
+ struct hwrng *rng;
while (size) {
- if (mutex_lock_interruptible(rng_mutex)) {
- err = -ERESTARTSYS;
+ rng = get_current_rng();
+ if (IS_ERR(rng)) {
+ err = PTR_ERR(rng);
goto out;
}
-
- if (!current_rng) {
+ if (!rng) {
err = -ENODEV;
- goto out_unlock;
+ goto out;
}
mutex_lock(reading_mutex);
if (!data_avail) {
- bytes_read = rng_get_data(current_rng, rng_buffer,
+ bytes_read = rng_get_data(rng, rng_buffer,
rng_buffer_size(),
!(filp-f_flags O_NONBLOCK));
if (bytes_read 0) {
@@ -200,7 +249,6 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
ret += len;
}
- mutex_unlock(rng_mutex);
mutex_unlock(reading_mutex);
if (need_resched())
@@ -210,15 +258,16 @@ static ssize_t rng_dev_read(struct file *filp, char
__user *buf,
err = -ERESTARTSYS;
goto out;
}
+
+ put_rng(rng);
}
out:
return ret ? : err;
-out_unlock:
- mutex_unlock(rng_mutex);
- goto out;
+
out_unlock_reading:
mutex_unlock(reading_mutex);
- goto out_unlock;
+ put_rng(rng);
+ goto out;
}
@@ -257,8 +306,8 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
err = hwrng_init(rng);
if (err)
break;
- hwrng_cleanup(current_rng);
- current_rng = rng;
+ drop_current_rng();
+
