thr3ads.net - Linux Virtualization - [PATCH v2 4/6] hw_random: fix unregister race. [Oct 2014]

If this information is useful, please help other people find it:
Share via:

Amos Kong

2014-Sep-18 12:37 UTC

[PATCH v2 0/6] fix hw_random stuck

When I hotunplug a busy virtio-rng device or try to access
hwrng attributes in non-smp guest, it gets stuck.

My original was pain, Rusty posted a real fix. This patchset
fixed two issue in v1, and tested by my 6+ cases.


| 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

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 | 166 +++++++++++++++++++++++++++++-------------
 include/linux/hw_random.h     |   2 +
 2 files changed, 117 insertions(+), 51 deletions(-)

-- 
1.9.3

Amos Kong

2014-Sep-18 12:37 UTC

head link

[PATCH v2 1/6] hw_random: place mutex around read functions and buffers.

From: Rusty Russell <rusty at rustcorp.com.au>

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 <rusty at rustcorp.com.au>
---
 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

Amos Kong

2014-Sep-18 12:37 UTC

head link

[PATCH v2 2/6] hw_random: move some code out mutex_lock for avoiding underlying deadlock

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 <akong at redhat.com>
---
 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

Amos Kong

2014-Sep-18 12:37 UTC

head link

[PATCH v2 3/6] hw_random: use reference counts on each struct hwrng.

From: Rusty Russell <rusty at rustcorp.com.au>

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.

V2: reduce reference count in signal_pending path

Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
Signed-off-by: Amos Kong <akong at redhat.com>
---
 drivers/char/hw_random/core.c | 136 +++++++++++++++++++++++++++++-------------
 include/linux/hw_random.h     |   2 +
 2 files changed, 95 insertions(+), 43 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index a0905c8..ee9e504 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())
@@ -208,17 +256,19 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
 
 		if (signal_pending(current)) {
 			err = -ERESTARTSYS;
+			put_rng(rng);
 			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 +307,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 +322,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 +405,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 +475,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 +508,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

Amos Kong

2014-Sep-18 12:37 UTC

head link

[PATCH v2 4/6] hw_random: fix unregister race.

From: Rusty Russell <rusty at rustcorp.com.au>

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.

Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
---
 drivers/char/hw_random/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index ee9e504..9f6f5bb 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,7 @@ static inline void cleanup_rng(struct kref *kref)
 
 	if (rng->cleanup)
 		rng->cleanup(rng);
+	wake_up_all(&rng_done);
 }
 
 static void set_current_rng(struct hwrng *rng)
-- 
1.9.3

Amos Kong

2014-Sep-18 12:37 UTC

head link

[PATCH v2 5/6] hw_random: don't double-check old_rng.

From: Rusty Russell <rusty at rustcorp.com.au>

Interesting anti-pattern.

Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
---
 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 9f6f5bb..6420409 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -473,14 +473,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

Amos Kong

2014-Sep-18 12:37 UTC

head link

[PATCH v2 6/6] hw_random: don't init list element we're about to add to list.

From: Rusty Russell <rusty at rustcorp.com.au>

Another interesting anti-pattern.

Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
---
 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 6420409..12187dd 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -486,7 +486,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

Amos Kong

2014-Sep-18 17:08 UTC

head link

[PATCH v2 3/6] hw_random: use reference counts on each struct hwrng.

On Thu, Sep 18, 2014 at 08:37:44PM +0800, Amos Kong
wrote:> From: Rusty Russell <rusty at rustcorp.com.au>
> 
> 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.
> 
> V2: reduce reference count in signal_pending path
> 
> Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
> Signed-off-by: Amos Kong <akong at redhat.com>
Reply myself.
> ---
>  drivers/char/hw_random/core.c | 136
+++++++++++++++++++++++++++++-------------
>  include/linux/hw_random.h     |   2 +
>  2 files changed, 95 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index a0905c8..ee9e504 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())
> @@ -208,17 +256,19 @@ static ssize_t rng_dev_read(struct file *filp, char
__user *buf,
>  
>  		if (signal_pending(current)) {
>  			err = -ERESTARTSYS;
> +			put_rng(rng);
>  			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 +307,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);
We got a warning in boot stage when above set_current_rng() is executed,
it can be fixed by init rng->ref in hwrng_init().


@@ -166,6 +169,8 @@ static inline int hwrng_init(struct hwrng *rng)
        if (current_quality > 0 && !hwrng_fill)
                start_khwrngd();
 
+       kref_init(&rng->ref);
+
        return 0;
 }


[    2.754303] ------------[ cut here ]------------
[    2.756018] WARNING: at include/linux/kref.h:47 kref_get.part.2+0x1e/0x27()
[    2.758150] Modules linked in: virtio_rng(+) parport_pc(+) parport mperf xfs
libcrc32c sd_mod sr_mod cdrom crc_t10dif crct10dif_common ata_generic pata_acpi
virtio_net cirrus syscopyarea sysfillrect sysimgblt drm_kms_helper ttm ata_piix
drm libata virtio_pci virtio_ring virtio i2c_core floppy dm_mirror
dm_region_hash dm_log dm_mod
[    2.765686] CPU: 0 PID: 470 Comm: systemd-udevd Not tainted
3.10.0-161.el7.rusty.x86_64 #1
[    2.767806] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
[    2.770449]  0000000000000000 0000000075730141 ffff88007bb13b68
ffffffff815f0673
[    2.772570]  ffff88007bb13ba0 ffffffff8106bc41 ffffffff819a0e50
ffff880036cd1000
[    2.774970]  ffff8800370a2cc0 0000000000000000 ffffffffa025c0e0
ffff88007bb13bb0
[    2.777267] Call Trace:
[    2.778843]  [<ffffffff815f0673>] dump_stack+0x19/0x1b
[    2.780824]  [<ffffffff8106bc41>] warn_slowpath_common+0x61/0x80
[    2.782726]  [<ffffffff8106bd6a>] warn_slowpath_null+0x1a/0x20
[    2.784566]  [<ffffffff815f106f>] kref_get.part.2+0x1e/0x27
[    2.786382]  [<ffffffff813b2d1a>] set_current_rng+0x3a/0x50
[    2.788184]  [<ffffffff813b32f8>] hwrng_register+0x148/0x290
[    2.790175]  [<ffffffffa005f7c0>] ? vp_reset+0x90/0x90 [virtio_pci]
[    2.792456]  [<ffffffffa025a149>] virtrng_scan+0x19/0x30 [virtio_rng]
[    2.794424]  [<ffffffffa0022208>] virtio_dev_probe+0x118/0x150 [virtio]
[    2.796391]  [<ffffffff813cac57>] driver_probe_device+0x87/0x390
[    2.798579]  [<ffffffff813cb033>] __driver_attach+0x93/0xa0
[    2.800576]  [<ffffffff813cafa0>] ? __device_attach+0x40/0x40
[    2.802500]  [<ffffffff813c89e3>] bus_for_each_dev+0x73/0xc0
[    2.804387]  [<ffffffff813ca6ae>] driver_attach+0x1e/0x20
[    2.806284]  [<ffffffff813ca200>] bus_add_driver+0x200/0x2d0
[    2.808511]  [<ffffffffa0034000>] ? 0xffffffffa0033fff
[    2.810313]  [<ffffffff813cb6b4>] driver_register+0x64/0xf0
[    2.812265]  [<ffffffffa0022540>] register_virtio_driver+0x20/0x30
[virtio]
[    2.814246]  [<ffffffffa0034010>] virtio_rng_driver_init+0x10/0x1000
[virtio_rng]
[    2.816253]  [<ffffffff810020b8>] do_one_initcall+0xb8/0x230
[    2.818053]  [<ffffffff810da4fa>] load_module+0x131a/0x1b20
[    2.819835]  [<ffffffff812ede80>] ? ddebug_proc_write+0xf0/0xf0
[    2.821635]  [<ffffffff810d6a93>] ?
copy_module_from_fd.isra.43+0x53/0x150
[    2.823723]  [<ffffffff810daeb6>] SyS_finit_module+0xa6/0xd0
[    2.825892]  [<ffffffff81600669>] system_call_fastpath+0x16/0x1b
[    2.827768] ---[ end trace bf8396ed0ec968f2 ]---

>  			err = 0;
>  			break;
>  		}
> @@ -272,17 +322,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 +405,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 +475,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 +508,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
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
-- 
			Amos.

Rusty Russell

2014-Oct-20 00:08 UTC

head link

[PATCH v2 3/6] hw_random: use reference counts on each struct hwrng.

Amos Kong <akong at redhat.com> writes:> From: Rusty Russell <rusty at rustcorp.com.au>
>
> 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.
>
> V2: reduce reference count in signal_pending path
OK, I changed it to do the put_rng before the check, so instead of:
> @@ -208,17 +256,19 @@ static ssize_t rng_dev_read(struct file *filp, char
__user *buf,
>  
>  		if (signal_pending(current)) {
>  			err = -ERESTARTSYS;
> +			put_rng(rng);
>  			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;
>  }
We have:


		mutex_unlock(&reading_mutex);
		put_rng(rng);

		if (need_resched())
			schedule_timeout_interruptible(1);

		if (signal_pending(current)) {
			err = -ERESTARTSYS;
			goto out;
		}
	}
out:
	return ret ? : err;

out_unlock_reading:
	mutex_unlock(&reading_mutex);
	put_rng(rng);
	goto out;
}


Cheers,
Rusty.

Herbert Xu

2014-Oct-21 14:17 UTC

head link

[PATCH v2 4/6] hw_random: fix unregister race.

On Thu, Sep 18, 2014 at 08:37:45PM +0800, Amos Kong
wrote:> From: Rusty Russell <rusty at rustcorp.com.au>
> 
> 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.
> 
> Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
You totally corrupted Rusty's patch.  If you're going to repost
his series you better make sure that you've got the right patches.

Just as well though as it made me think a little more about this
patch :)

Cheers,
-- 
Email: Herbert Xu <herbert at gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Maybe Matching Threads

Search for more apparently analagous threads

Linux Virtualization - Oct 2014 - [PATCH v2 4/6] hw_random: fix unregister race.

[PATCH v2 0/6] fix hw_random stuck

[PATCH v2 1/6] hw_random: place mutex around read functions and buffers.

[PATCH v2 2/6] hw_random: move some code out mutex_lock for avoiding underlying deadlock

[PATCH v2 3/6] hw_random: use reference counts on each struct hwrng.

[PATCH v2 4/6] hw_random: fix unregister race.

[PATCH v2 5/6] hw_random: don't double-check old_rng.

[PATCH v2 6/6] hw_random: don't init list element we're about to add to list.

[PATCH v2 3/6] hw_random: use reference counts on each struct hwrng.

[PATCH v2 3/6] hw_random: use reference counts on each struct hwrng.

[PATCH v2 4/6] hw_random: fix unregister race.

Maybe Matching Threads