When I hotunplug a busy virtio-rng device or try to access
hwrng attributes in non-smp guest, it gets stuck.
My hotplug tests:
| test 0:
| hotunplug rng device from qemu monitor
|
| test 1:
| guest) # dd if=/dev/hwrng of=/dev/null &
| hotunplug rng device from qemu monitor
|
| test 2:
| guest) # dd if=/dev/random of=/dev/null &
| hotunplug rng device from qemu monitor
|
| test 4:
| guest) # dd if=/dev/hwrng of=/dev/null &
| cat /sys/devices/virtual/misc/hw_random/rng_*
|
| test 5:
| 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
|
| test 6:
| use a fifo as rng backend, execute test 0 ~ 5 with no input of fifo
V5: reset cleanup_done flag, drop redundant init of reference count, use
compiler barrier to prevent recording.
V4: update patch 4 to fix corrupt, decrease last reference for triggering
the cleanup, fix unregister race pointed by Herbert
V3: initialize kref to 1
V2: added patch 2 to fix a deadlock, update current patch 3 to fix reference
counting issue
Amos Kong (1):
hw_random: move some code out mutex_lock for avoiding underlying
deadlock
Rusty Russell (5):
hw_random: place mutex around read functions and buffers.
hw_random: use reference counts on each struct hwrng.
hw_random: fix unregister race.
hw_random: don't double-check old_rng.
hw_random: don't init list element we're about to add to list.
drivers/char/hw_random/core.c | 173 ++++++++++++++++++++++++++++++------------
include/linux/hw_random.h | 3 +
2 files changed, 126 insertions(+), 50 deletions(-)
--
1.9.3
From: Rusty Russell <[email protected]>
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.
v5: drop redundant kref_init()
v4: decrease last reference for triggering the cleanup
v3: initialize kref (thanks Amos Kong)
v2: fix missing put_rng() on exit path (thanks Amos Kong)
Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Amos Kong <[email protected]>
---
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 a0905c8..83516cb 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,60 @@ 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;
+
+ /* decrease last reference for triggering the cleanup */
+ kref_put(¤t_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 +168,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 +203,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,8 +250,8 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf,
ret += len;
}
- mutex_unlock(&rng_mutex);
mutex_unlock(&reading_mutex);
+ put_rng(rng);
if (need_resched())
schedule_timeout_interruptible(1);
@@ -213,12 +263,11 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf,
}
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()) {
- if (!current_rng)
+ struct hwrng *rng;
+
+ rng = get_current_rng();
+ if (IS_ERR(rng) || !rng)
break;
mutex_lock(&reading_mutex);
- rc = rng_get_data(current_rng, rng_fillbuf,
+ rc = rng_get_data(rng, rng_fillbuf,
rng_buffer_size(), 1);
mutex_unlock(&reading_mutex);
+ put_rng(rng);
if (rc <= 0) {
pr_warn("hwrng: no data available\n");
msleep_interruptible(10000);
@@ -423,14 +474,13 @@ int hwrng_register(struct hwrng *rng)
err = hwrng_init(rng);
if (err)
goto out_unlock;
- current_rng = rng;
+ set_current_rng(rng);
}
err = 0;
if (!old_rng) {
err = register_miscdev();
if (err) {
- hwrng_cleanup(rng);
- current_rng = NULL;
+ drop_current_rng();
goto out_unlock;
}
}
@@ -457,22 +507,21 @@ EXPORT_SYMBOL_GPL(hwrng_register);
void hwrng_unregister(struct hwrng *rng)
{
- int err;
-
mutex_lock(&rng_mutex);
list_del(&rng->list);
if (current_rng == rng) {
- hwrng_cleanup(rng);
- if (list_empty(&rng_list)) {
- current_rng = NULL;
- } else {
- current_rng = list_entry(rng_list.prev, struct hwrng, list);
- err = hwrng_init(current_rng);
- if (err)
- current_rng = NULL;
+ drop_current_rng();
+ if (!list_empty(&rng_list)) {
+ struct hwrng *tail;
+
+ tail = list_entry(rng_list.prev, struct hwrng, list);
+
+ if (hwrng_init(tail) == 0)
+ set_current_rng(tail);
}
}
+
if (list_empty(&rng_list)) {
mutex_unlock(&rng_mutex);
unregister_miscdev();
diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index 914bb08..c212e71 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -14,6 +14,7 @@
#include <linux/types.h>
#include <linux/list.h>
+#include <linux/kref.h>
/**
* struct hwrng - Hardware Random Number Generator driver
@@ -44,6 +45,7 @@ struct hwrng {
/* internal. */
struct list_head list;
+ struct kref ref;
};
/** Register a new Hardware Random Number Generator driver. */
--
1.9.3
From: Rusty Russell <[email protected]>
Interesting anti-pattern.
Signed-off-by: Rusty Russell <[email protected]>
---
drivers/char/hw_random/core.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 067270b..a9286bf 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -476,14 +476,13 @@ int hwrng_register(struct hwrng *rng)
}
old_rng = current_rng;
+ err = 0;
if (!old_rng) {
err = hwrng_init(rng);
if (err)
goto out_unlock;
set_current_rng(rng);
- }
- err = 0;
- if (!old_rng) {
+
err = register_miscdev();
if (err) {
drop_current_rng();
--
1.9.3
In next patch, we use reference counting for each struct hwrng,
changing reference count also needs to take mutex_lock. Before
releasing the lock, if we try to stop a kthread that waits to
take the lock to reduce the referencing count, deadlock will
occur.
Signed-off-by: Amos Kong <[email protected]>
---
drivers/char/hw_random/core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index b1b6042..a0905c8 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -474,12 +474,12 @@ void hwrng_unregister(struct hwrng *rng)
}
}
if (list_empty(&rng_list)) {
+ mutex_unlock(&rng_mutex);
unregister_miscdev();
if (hwrng_fill)
kthread_stop(hwrng_fill);
- }
-
- mutex_unlock(&rng_mutex);
+ } else
+ mutex_unlock(&rng_mutex);
}
EXPORT_SYMBOL_GPL(hwrng_unregister);
--
1.9.3
From: Rusty Russell <[email protected]>
Another interesting anti-pattern.
Signed-off-by: Rusty Russell <[email protected]>
---
drivers/char/hw_random/core.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index a9286bf..4d13ac5 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -489,7 +489,6 @@ int hwrng_register(struct hwrng *rng)
goto out_unlock;
}
}
- INIT_LIST_HEAD(&rng->list);
list_add_tail(&rng->list, &rng_list);
if (old_rng && !rng->init) {
--
1.9.3
From: Rusty Russell <[email protected]>
There's currently a big lock around everything, and it means that we
can't query sysfs (eg /sys/devices/virtual/misc/hw_random/rng_current)
while the rng is reading. This is a real problem when the rng is slow,
or blocked (eg. virtio_rng with qemu's default /dev/random backend)
This doesn't help (it leaves the current lock untouched), just adds a
lock to protect the read function and the static buffers, in preparation
for transition.
Signed-off-by: Rusty Russell <[email protected]>
---
drivers/char/hw_random/core.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index aa30a25..b1b6042 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -53,7 +53,10 @@
static struct hwrng *current_rng;
static struct task_struct *hwrng_fill;
static LIST_HEAD(rng_list);
+/* Protects rng_list and current_rng */
static DEFINE_MUTEX(rng_mutex);
+/* Protects rng read functions, data_avail, rng_buffer and rng_fillbuf */
+static DEFINE_MUTEX(reading_mutex);
static int data_avail;
static u8 *rng_buffer, *rng_fillbuf;
static unsigned short current_quality;
@@ -81,7 +84,9 @@ static void add_early_randomness(struct hwrng *rng)
unsigned char bytes[16];
int bytes_read;
+ mutex_lock(&reading_mutex);
bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
+ mutex_unlock(&reading_mutex);
if (bytes_read > 0)
add_device_randomness(bytes, bytes_read);
}
@@ -128,6 +133,7 @@ static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size,
int wait) {
int present;
+ BUG_ON(!mutex_is_locked(&reading_mutex));
if (rng->read)
return rng->read(rng, (void *)buffer, size, wait);
@@ -160,13 +166,14 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf,
goto out_unlock;
}
+ mutex_lock(&reading_mutex);
if (!data_avail) {
bytes_read = rng_get_data(current_rng, rng_buffer,
rng_buffer_size(),
!(filp->f_flags & O_NONBLOCK));
if (bytes_read < 0) {
err = bytes_read;
- goto out_unlock;
+ goto out_unlock_reading;
}
data_avail = bytes_read;
}
@@ -174,7 +181,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf,
if (!data_avail) {
if (filp->f_flags & O_NONBLOCK) {
err = -EAGAIN;
- goto out_unlock;
+ goto out_unlock_reading;
}
} else {
len = data_avail;
@@ -186,7 +193,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf,
if (copy_to_user(buf + ret, rng_buffer + data_avail,
len)) {
err = -EFAULT;
- goto out_unlock;
+ goto out_unlock_reading;
}
size -= len;
@@ -194,6 +201,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf,
}
mutex_unlock(&rng_mutex);
+ mutex_unlock(&reading_mutex);
if (need_resched())
schedule_timeout_interruptible(1);
@@ -208,6 +216,9 @@ out:
out_unlock:
mutex_unlock(&rng_mutex);
goto out;
+out_unlock_reading:
+ mutex_unlock(&reading_mutex);
+ goto out_unlock;
}
@@ -348,13 +359,16 @@ static int hwrng_fillfn(void *unused)
while (!kthread_should_stop()) {
if (!current_rng)
break;
+ mutex_lock(&reading_mutex);
rc = rng_get_data(current_rng, rng_fillbuf,
rng_buffer_size(), 1);
+ mutex_unlock(&reading_mutex);
if (rc <= 0) {
pr_warn("hwrng: no data available\n");
msleep_interruptible(10000);
continue;
}
+ /* Outside lock, sure, but y'know: randomness. */
add_hwgenerator_randomness((void *)rng_fillbuf, rc,
rc * current_quality * 8 >> 10);
}
--
1.9.3
From: Rusty Russell <[email protected]>
The previous patch added one potential problem: we can still be
reading from a hwrng when it's unregistered. Add a wait for zero
in the hwrng_unregister path.
v5: reset cleanup_done flag, use compiler barrier to prevent recording.
v4: add cleanup_done flag to insure that cleanup is done
Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Amos Kong <[email protected]>
---
drivers/char/hw_random/core.c | 12 ++++++++++++
include/linux/hw_random.h | 1 +
2 files changed, 13 insertions(+)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 83516cb..067270b 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -60,6 +60,7 @@ static DEFINE_MUTEX(rng_mutex);
static DEFINE_MUTEX(reading_mutex);
static int data_avail;
static u8 *rng_buffer, *rng_fillbuf;
+static DECLARE_WAIT_QUEUE_HEAD(rng_done);
static unsigned short current_quality;
static unsigned short default_quality; /* = 0; default to "off" */
@@ -98,6 +99,11 @@ static inline void cleanup_rng(struct kref *kref)
if (rng->cleanup)
rng->cleanup(rng);
+
+ /* cleanup_done should be updated after cleanup finishes */
+ smp_wmb();
+ rng->cleanup_done = true;
+ wake_up_all(&rng_done);
}
static void set_current_rng(struct hwrng *rng)
@@ -498,6 +504,8 @@ int hwrng_register(struct hwrng *rng)
add_early_randomness(rng);
}
+ rng->cleanup_done = false;
+
out_unlock:
mutex_unlock(&rng_mutex);
out:
@@ -529,6 +537,10 @@ void hwrng_unregister(struct hwrng *rng)
kthread_stop(hwrng_fill);
} else
mutex_unlock(&rng_mutex);
+
+ /* Just in case rng is reading right now, wait. */
+ wait_event(rng_done, rng->cleanup_done &&
+ atomic_read(&rng->ref.refcount) == 0);
}
EXPORT_SYMBOL_GPL(hwrng_unregister);
diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index c212e71..7832e50 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -46,6 +46,7 @@ struct hwrng {
/* internal. */
struct list_head list;
struct kref ref;
+ bool cleanup_done;
};
/** Register a new Hardware Random Number Generator driver. */
--
1.9.3
On Mon, Dec 08, 2014 at 04:50:34PM +0800, Amos Kong wrote:
> When I hotunplug a busy virtio-rng device or try to access
> hwrng attributes in non-smp guest, it gets stuck.
>
> My hotplug tests:
>
> | test 0:
> | hotunplug rng device from qemu monitor
> |
> | test 1:
> | guest) # dd if=/dev/hwrng of=/dev/null &
> | hotunplug rng device from qemu monitor
> |
> | test 2:
> | guest) # dd if=/dev/random of=/dev/null &
> | hotunplug rng device from qemu monitor
> |
> | test 4:
> | guest) # dd if=/dev/hwrng of=/dev/null &
> | cat /sys/devices/virtual/misc/hw_random/rng_*
> |
> | test 5:
> | 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
> |
> | test 6:
> | use a fifo as rng backend, execute test 0 ~ 5 with no input of fifo
All applied. Thanks a lot!
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
A bit late to a party, but:
On Mon, Dec 8, 2014 at 12:50 AM, Amos Kong <[email protected]> wrote:
> From: Rusty Russell <[email protected]>
>
> There's currently a big lock around everything, and it means that we
> can't query sysfs (eg /sys/devices/virtual/misc/hw_random/rng_current)
> while the rng is reading. This is a real problem when the rng is slow,
> or blocked (eg. virtio_rng with qemu's default /dev/random backend)
>
> This doesn't help (it leaves the current lock untouched), just adds a
> lock to protect the read function and the static buffers, in preparation
> for transition.
>
> Signed-off-by: Rusty Russell <[email protected]>
> ---
...
>
> @@ -160,13 +166,14 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf,
> goto out_unlock;
> }
>
> + mutex_lock(&reading_mutex);
I think this breaks O_NONBLOCK: we have hwrng core thread that is
constantly pumps underlying rng for data; the thread takes the mutex
and calls rng_get_data() that blocks until RNG responds. This means
that even user specified O_NONBLOCK here we'll be waiting until
[hwrng] thread releases reading_mutex before we can continue.
> if (!data_avail) {
> bytes_read = rng_get_data(current_rng, rng_buffer,
> rng_buffer_size(),
> !(filp->f_flags & O_NONBLOCK));
> if (bytes_read < 0) {
> err = bytes_read;
> - goto out_unlock;
> + goto out_unlock_reading;
> }
> data_avail = bytes_read;
> }
Thanks.
--
Dmitry
>
> A bit late to a party, but:
>
> On Mon, Dec 8, 2014 at 12:50 AM, Amos Kong <[email protected]> wrote:
> > From: Rusty Russell <[email protected]>
> >
> > There's currently a big lock around everything, and it means that we
> > can't query sysfs (eg /sys/devices/virtual/misc/hw_random/rng_current)
> > while the rng is reading. This is a real problem when the rng is slow,
> > or blocked (eg. virtio_rng with qemu's default /dev/random backend)
> >
> > This doesn't help (it leaves the current lock untouched), just adds a
> > lock to protect the read function and the static buffers, in preparation
> > for transition.
> >
> > Signed-off-by: Rusty Russell <[email protected]>
> > ---
> ...
> >
> > @@ -160,13 +166,14 @@ static ssize_t rng_dev_read(struct file *filp, char
> > __user *buf,
> > goto out_unlock;
> > }
> >
> > + mutex_lock(&reading_mutex);
>
> I think this breaks O_NONBLOCK: we have hwrng core thread that is
> constantly pumps underlying rng for data; the thread takes the mutex
> and calls rng_get_data() that blocks until RNG responds. This means
> that even user specified O_NONBLOCK here we'll be waiting until
> [hwrng] thread releases reading_mutex before we can continue.
I think for 'virtio_rng' for 'O_NON_BLOCK' 'rng_get_data' returns
without waiting for data which can let mutex to be used by other
threads waiting if any?
rng_dev_read
rng_get_data
virtio_read
static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
{
int ret;
struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
if (vi->hwrng_removed)
return -ENODEV;
if (!vi->busy) {
vi->busy = true;
init_completion(&vi->have_data);
register_buffer(vi, buf, size);
}
if (!wait)
return 0;
ret = wait_for_completion_killable(&vi->have_data);
if (ret < 0)
return ret;
vi->busy = false;
return vi->data_avail;
}
>
> > if (!data_avail) {
> > bytes_read = rng_get_data(current_rng, rng_buffer,
> > rng_buffer_size(),
> > !(filp->f_flags & O_NONBLOCK));
> > if (bytes_read < 0) {
> > err = bytes_read;
> > - goto out_unlock;
> > + goto out_unlock_reading;
> > }
> > data_avail = bytes_read;
> > }
>
> Thanks.
>
> --
> Dmitry
>
On Tue, Sep 26, 2017 at 02:36:57AM -0400, Pankaj Gupta wrote:
>
> >
> > A bit late to a party, but:
> >
> > On Mon, Dec 8, 2014 at 12:50 AM, Amos Kong <[email protected]> wrote:
> > > From: Rusty Russell <[email protected]>
> > >
> > > There's currently a big lock around everything, and it means that we
> > > can't query sysfs (eg /sys/devices/virtual/misc/hw_random/rng_current)
> > > while the rng is reading. This is a real problem when the rng is slow,
> > > or blocked (eg. virtio_rng with qemu's default /dev/random backend)
> > >
> > > This doesn't help (it leaves the current lock untouched), just adds a
> > > lock to protect the read function and the static buffers, in preparation
> > > for transition.
> > >
> > > Signed-off-by: Rusty Russell <[email protected]>
> > > ---
> > ...
> > >
> > > @@ -160,13 +166,14 @@ static ssize_t rng_dev_read(struct file *filp, char
> > > __user *buf,
> > > goto out_unlock;
> > > }
> > >
> > > + mutex_lock(&reading_mutex);
> >
> > I think this breaks O_NONBLOCK: we have hwrng core thread that is
> > constantly pumps underlying rng for data; the thread takes the mutex
> > and calls rng_get_data() that blocks until RNG responds. This means
> > that even user specified O_NONBLOCK here we'll be waiting until
> > [hwrng] thread releases reading_mutex before we can continue.
>
> I think for 'virtio_rng' for 'O_NON_BLOCK' 'rng_get_data' returns
> without waiting for data which can let mutex to be used by other
> threads waiting if any?
>
> rng_dev_read
> rng_get_data
> virtio_read
As I said in the paragraph above the code that potentially holds the
mutex for long time is the thread in hwrng core: hwrng_fillfn(). As it
calls rng_get_data() with "wait" argument == 1 it may block while
holding reading_mutex, which, in turn, will block rng_dev_read(), even
if it was called with O_NONBLOCK.
Thanks.
--
Dmitry
>
> On Tue, Sep 26, 2017 at 02:36:57AM -0400, Pankaj Gupta wrote:
> >
> > >
> > > A bit late to a party, but:
> > >
> > > On Mon, Dec 8, 2014 at 12:50 AM, Amos Kong <[email protected]> wrote:
> > > > From: Rusty Russell <[email protected]>
> > > >
> > > > There's currently a big lock around everything, and it means that we
> > > > can't query sysfs (eg /sys/devices/virtual/misc/hw_random/rng_current)
> > > > while the rng is reading. This is a real problem when the rng is slow,
> > > > or blocked (eg. virtio_rng with qemu's default /dev/random backend)
> > > >
> > > > This doesn't help (it leaves the current lock untouched), just adds a
> > > > lock to protect the read function and the static buffers, in
> > > > preparation
> > > > for transition.
> > > >
> > > > Signed-off-by: Rusty Russell <[email protected]>
> > > > ---
> > > ...
> > > >
> > > > @@ -160,13 +166,14 @@ static ssize_t rng_dev_read(struct file *filp,
> > > > char
> > > > __user *buf,
> > > > goto out_unlock;
> > > > }
> > > >
> > > > + mutex_lock(&reading_mutex);
> > >
> > > I think this breaks O_NONBLOCK: we have hwrng core thread that is
> > > constantly pumps underlying rng for data; the thread takes the mutex
> > > and calls rng_get_data() that blocks until RNG responds. This means
> > > that even user specified O_NONBLOCK here we'll be waiting until
> > > [hwrng] thread releases reading_mutex before we can continue.
> >
> > I think for 'virtio_rng' for 'O_NON_BLOCK' 'rng_get_data' returns
> > without waiting for data which can let mutex to be used by other
> > threads waiting if any?
> >
> > rng_dev_read
> > rng_get_data
> > virtio_read
>
> As I said in the paragraph above the code that potentially holds the
> mutex for long time is the thread in hwrng core: hwrng_fillfn(). As it
> calls rng_get_data() with "wait" argument == 1 it may block while
> holding reading_mutex, which, in turn, will block rng_dev_read(), even
> if it was called with O_NONBLOCK.
yes, 'hwrng_fillfn' does not consider O_NONBLOCK and can result in mutex wait
for other tasks. What if we pass zero for wait to 'hwrng_fill' to return early
if there is no data?
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -403,7 +403,7 @@ static int hwrng_fillfn(void *unused)
break;
mutex_lock(&reading_mutex);
rc = rng_get_data(rng, rng_fillbuf,
- rng_buffer_size(), 1);
+ rng_buffer_size(), 0);
mutex_unlock(&reading_mutex);
put_rng(rng);
if (rc <= 0) {
Thanks,
Pankaj
>
> Thanks.
>
> --
> Dmitry
>