[0/5] hwrng: Fix kref warning and underlying bugs

[0/5] hwrng: Fix kref warning and underlying bugs

[Herbert Xu]

On Mon, Dec 22, 2014 at 07:09:56PM -0800, Fengguang Wu wrote:
> Greetings,
> 
> 0day kernel testing robot got the below dmesg and the first bad commit is

Sigh, looks like more work is needed on this.

Here is a series of patches that should fix this along with the
underlying issue exposed by it and some other related problems.

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

[PATCH 1/5] hwrng: core - Use struct completion for cleanup_done

[Herbert Xu]

There is no point in doing a manual completion for cleanup_done
when struct completion fits in perfectly.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/char/hw_random/core.c |   12 +++---------
 include/linux/hw_random.h     |    3 ++-
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 6ec4225..3dba2cf 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -60,7 +60,6 @@ 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" */
 
@@ -100,10 +99,7 @@ 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);
+	complete(&rng->cleanup_done);
 }
 
 static void set_current_rng(struct hwrng *rng)
@@ -498,7 +494,7 @@ int hwrng_register(struct hwrng *rng)
 		add_early_randomness(rng);
 	}
 
-	rng->cleanup_done = false;
+	init_completion(&rng->cleanup_done);
 
 out_unlock:
 	mutex_unlock(&rng_mutex);
@@ -532,9 +528,7 @@ void hwrng_unregister(struct hwrng *rng)
 	} 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);
+	wait_for_completion(&rng->cleanup_done);
 }
 EXPORT_SYMBOL_GPL(hwrng_unregister);
 
diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index 7832e50..eb7b414 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -12,6 +12,7 @@
 #ifndef LINUX_HWRANDOM_H_
 #define LINUX_HWRANDOM_H_
 
+#include <linux/completion.h>
 #include <linux/types.h>
 #include <linux/list.h>
 #include <linux/kref.h>
@@ -46,7 +47,7 @@ struct hwrng {
 	/* internal. */
 	struct list_head list;
 	struct kref ref;
-	bool cleanup_done;
+	struct completion cleanup_done;
 };
 
 /** Register a new Hardware Random Number Generator driver. */

[PATCH 2/5] hwrng: core - Fix current_rng init/cleanup race yet again

[Herbert Xu]

The kref solution is still buggy because we were only focusing
on the register/unregister race.  The same race affects the
setting of current_rng through sysfs.

This patch fixes it by using kref_get_unless_zero.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/char/hw_random/core.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 3dba2cf..42827fd 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -105,7 +105,6 @@ static inline void cleanup_rng(struct kref *kref)
 static void set_current_rng(struct hwrng *rng)
 {
 	BUG_ON(!mutex_is_locked(&rng_mutex));
-	kref_get(&rng->ref);
 	current_rng = rng;
 }
 
@@ -150,6 +149,9 @@ static void put_rng(struct hwrng *rng)
 
 static inline int hwrng_init(struct hwrng *rng)
 {
+	if (kref_get_unless_zero(&rng->ref))
+		goto skip_init;
+
 	if (rng->init) {
 		int ret;
 
@@ -157,6 +159,11 @@ static inline int hwrng_init(struct hwrng *rng)
 		if (ret)
 			return ret;
 	}
+
+	kref_init(&rng->ref);
+	reinit_completion(&rng->cleanup_done);
+
+skip_init:
 	add_early_randomness(rng);
 
 	current_quality = rng->quality ? : default_quality;
@@ -467,6 +474,9 @@ int hwrng_register(struct hwrng *rng)
 			goto out_unlock;
 	}
 
+	init_completion(&rng->cleanup_done);
+	complete(&rng->cleanup_done);
+
 	old_rng = current_rng;
 	err = 0;
 	if (!old_rng) {
@@ -494,8 +504,6 @@ int hwrng_register(struct hwrng *rng)
 		add_early_randomness(rng);
 	}
 
-	init_completion(&rng->cleanup_done);
-
 out_unlock:
 	mutex_unlock(&rng_mutex);
 out:

[PATCH 3/5] hwrng: core - Do not register device opportunistically

[Herbert Xu]

Currently we only register the device when a valid RNG is added.
However the way it's done is buggy because we test whether there
is a current RNG to determine whether we need to register.  As
the current RNG may be missing due to a reinitialisation error
this can lead to a reregistration of the device.

As the device already has to handle a NULL current RNG anyway,
let's just register the device always and remove the complexity.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/char/hw_random/core.c |   23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 42827fd..1d342f0 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -372,14 +372,14 @@ static DEVICE_ATTR(rng_available, S_IRUGO,
 		   NULL);
 
 
-static void unregister_miscdev(void)
+static void __exit unregister_miscdev(void)
 {
 	device_remove_file(rng_miscdev.this_device, &dev_attr_rng_available);
 	device_remove_file(rng_miscdev.this_device, &dev_attr_rng_current);
 	misc_deregister(&rng_miscdev);
 }
 
-static int register_miscdev(void)
+static int __init register_miscdev(void)
 {
 	int err;
 
@@ -484,12 +484,6 @@ int hwrng_register(struct hwrng *rng)
 		if (err)
 			goto out_unlock;
 		set_current_rng(rng);
-
-		err = register_miscdev();
-		if (err) {
-			drop_current_rng();
-			goto out_unlock;
-		}
 	}
 	list_add_tail(&rng->list, &rng_list);
 
@@ -530,7 +524,6 @@ void hwrng_unregister(struct hwrng *rng)
 
 	if (list_empty(&rng_list)) {
 		mutex_unlock(&rng_mutex);
-		unregister_miscdev();
 		if (hwrng_fill)
 			kthread_stop(hwrng_fill);
 	} else
@@ -540,16 +533,24 @@ void hwrng_unregister(struct hwrng *rng)
 }
 EXPORT_SYMBOL_GPL(hwrng_unregister);
 
-static void __exit hwrng_exit(void)
+static int __init hwrng_modinit(void)
+{
+	return register_miscdev();
+}
+
+static void __exit hwrng_modexit(void)
 {
 	mutex_lock(&rng_mutex);
 	BUG_ON(current_rng);
 	kfree(rng_buffer);
 	kfree(rng_fillbuf);
 	mutex_unlock(&rng_mutex);
+
+	unregister_miscdev();
 }
 
-module_exit(hwrng_exit);
+module_init(hwrng_modinit);
+module_exit(hwrng_modexit);
 
 MODULE_DESCRIPTION("H/W Random Number Generator (RNG) driver");
 MODULE_LICENSE("GPL");

[PATCH 4/5] hwrng: core - Drop current rng in set_current_rng

[Herbert Xu]

Rather than having callers of set_current_rng call drop_current_rng,
we can do it directly in set_current_rng.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/char/hw_random/core.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 1d342f0..787ef42 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -70,6 +70,7 @@ 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 void start_khwrngd(void);
 
 static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size,
@@ -105,6 +106,7 @@ static inline void cleanup_rng(struct kref *kref)
 static void set_current_rng(struct hwrng *rng)
 {
 	BUG_ON(!mutex_is_locked(&rng_mutex));
+	drop_current_rng();
 	current_rng = rng;
 }
 
@@ -315,7 +317,6 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
 			err = hwrng_init(rng);
 			if (err)
 				break;
-			drop_current_rng();
 			set_current_rng(rng);
 			err = 0;
 			break;

[PATCH 5/5] hwrng: core - Move hwrng_init call into set_current_rng

[Herbert Xu]

We always do hwrng_init in set_current_rng.  In fact, our current
reference count system relies on this.  So make this explicit by
moving hwrng_init into set_current_rng.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/char/hw_random/core.c |   30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 787ef42..32a8a86 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -71,6 +71,7 @@ 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);
 
 static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size,
@@ -103,11 +104,20 @@ static inline void cleanup_rng(struct kref *kref)
 	complete(&rng->cleanup_done);
 }
 
-static void set_current_rng(struct hwrng *rng)
+static int set_current_rng(struct hwrng *rng)
 {
+	int err;
+
 	BUG_ON(!mutex_is_locked(&rng_mutex));
+
+	err = hwrng_init(rng);
+	if (err)
+		return err;
+
 	drop_current_rng();
 	current_rng = rng;
+
+	return 0;
 }
 
 static void drop_current_rng(void)
@@ -149,7 +159,7 @@ static void put_rng(struct hwrng *rng)
 	mutex_unlock(&rng_mutex);
 }
 
-static inline int hwrng_init(struct hwrng *rng)
+static int hwrng_init(struct hwrng *rng)
 {
 	if (kref_get_unless_zero(&rng->ref))
 		goto skip_init;
@@ -310,15 +320,9 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
 	err = -ENODEV;
 	list_for_each_entry(rng, &rng_list, list) {
 		if (strcmp(rng->name, buf) == 0) {
-			if (rng == current_rng) {
-				err = 0;
-				break;
-			}
-			err = hwrng_init(rng);
-			if (err)
-				break;
-			set_current_rng(rng);
 			err = 0;
+			if (rng != current_rng)
+				err = set_current_rng(rng);
 			break;
 		}
 	}
@@ -481,10 +485,9 @@ int hwrng_register(struct hwrng *rng)
 	old_rng = current_rng;
 	err = 0;
 	if (!old_rng) {
-		err = hwrng_init(rng);
+		err = set_current_rng(rng);
 		if (err)
 			goto out_unlock;
-		set_current_rng(rng);
 	}
 	list_add_tail(&rng->list, &rng_list);
 
@@ -518,8 +521,7 @@ void hwrng_unregister(struct hwrng *rng)
 
 			tail = list_entry(rng_list.prev, struct hwrng, list);
 
-			if (hwrng_init(tail) == 0)
-				set_current_rng(tail);
+			set_current_rng(tail);
 		}
 	}
 

Re: [PATCH 1/5] hwrng: core - Use struct completion for cleanup_done

[Rusty Russell]

Herbert Xu <herbert@gondor.apana.org.au> writes:
> There is no point in doing a manual completion for cleanup_done
> when struct completion fits in perfectly.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Indeed.

Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Thanks,
Rusty.

> ---
>
>  drivers/char/hw_random/core.c |   12 +++---------
>  include/linux/hw_random.h     |    3 ++-
>  2 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 6ec4225..3dba2cf 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -60,7 +60,6 @@ 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" */
>  
> @@ -100,10 +99,7 @@ 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);
> +	complete(&rng->cleanup_done);
>  }
>  
>  static void set_current_rng(struct hwrng *rng)
> @@ -498,7 +494,7 @@ int hwrng_register(struct hwrng *rng)
>  		add_early_randomness(rng);
>  	}
>  
> -	rng->cleanup_done = false;
> +	init_completion(&rng->cleanup_done);
>  
>  out_unlock:
>  	mutex_unlock(&rng_mutex);
> @@ -532,9 +528,7 @@ void hwrng_unregister(struct hwrng *rng)
>  	} 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);
> +	wait_for_completion(&rng->cleanup_done);
>  }
>  EXPORT_SYMBOL_GPL(hwrng_unregister);
>  
> diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
> index 7832e50..eb7b414 100644
> --- a/include/linux/hw_random.h
> +++ b/include/linux/hw_random.h
> @@ -12,6 +12,7 @@
>  #ifndef LINUX_HWRANDOM_H_
>  #define LINUX_HWRANDOM_H_
>  
> +#include <linux/completion.h>
>  #include <linux/types.h>
>  #include <linux/list.h>
>  #include <linux/kref.h>
> @@ -46,7 +47,7 @@ struct hwrng {
>  	/* internal. */
>  	struct list_head list;
>  	struct kref ref;
> -	bool cleanup_done;
> +	struct completion cleanup_done;
>  };
>  
>  /** Register a new Hardware Random Number Generator driver. */

Re: [PATCH 2/5] hwrng: core - Fix current_rng init/cleanup race yet again

[Rusty Russell]

Herbert Xu <herbert@gondor.apana.org.au> writes:
> The kref solution is still buggy because we were only focusing
> on the register/unregister race.  The same race affects the
> setting of current_rng through sysfs.
>
> This patch fixes it by using kref_get_unless_zero.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

This patch scares me a little!

I'll have to pull the tree to review it properly, but the theory was
that the reference count was counting users of the rng.  Now I don't
know what it's counting:

>  static inline int hwrng_init(struct hwrng *rng)
>  {
> +	if (kref_get_unless_zero(&rng->ref))
> +		goto skip_init;
> +
>  	if (rng->init) {
>  		int ret;

OK, so this skip_init branch is triggered when the rng is being
shut down as it's no longer current_rng?

> +
> +	kref_init(&rng->ref);
> +	reinit_completion(&rng->cleanup_done);
> +
> +skip_init:
>  	add_early_randomness(rng);

Then we use it to add randomness?

>  
>  	current_quality = rng->quality ? : default_quality;
> @@ -467,6 +474,9 @@ int hwrng_register(struct hwrng *rng)
>  			goto out_unlock;
>  	}
>  
> +	init_completion(&rng->cleanup_done);
> +	complete(&rng->cleanup_done);
> +

This code smells very bad.

Cheers,
Rusty.

Re: [PATCH 3/5] hwrng: core - Do not register device opportunistically

[Rusty Russell]

Herbert Xu <herbert@gondor.apana.org.au> writes:
> Currently we only register the device when a valid RNG is added.
> However the way it's done is buggy because we test whether there
> is a current RNG to determine whether we need to register.  As
> the current RNG may be missing due to a reinitialisation error
> this can lead to a reregistration of the device.
>
> As the device already has to handle a NULL current RNG anyway,
> let's just register the device always and remove the complexity.

Does this break userspace by creating a device which will just return
-ENODEV on read?  Sure, callers *should* handle it...

I far prefer this (simpler) model, though.

Thanks,
Rusty.

>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---
>
>  drivers/char/hw_random/core.c |   23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 42827fd..1d342f0 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -372,14 +372,14 @@ static DEVICE_ATTR(rng_available, S_IRUGO,
>  		   NULL);
>  
>  
> -static void unregister_miscdev(void)
> +static void __exit unregister_miscdev(void)
>  {
>  	device_remove_file(rng_miscdev.this_device, &dev_attr_rng_available);
>  	device_remove_file(rng_miscdev.this_device, &dev_attr_rng_current);
>  	misc_deregister(&rng_miscdev);
>  }
>  
> -static int register_miscdev(void)
> +static int __init register_miscdev(void)
>  {
>  	int err;
>  
> @@ -484,12 +484,6 @@ int hwrng_register(struct hwrng *rng)
>  		if (err)
>  			goto out_unlock;
>  		set_current_rng(rng);
> -
> -		err = register_miscdev();
> -		if (err) {
> -			drop_current_rng();
> -			goto out_unlock;
> -		}
>  	}
>  	list_add_tail(&rng->list, &rng_list);
>  
> @@ -530,7 +524,6 @@ void hwrng_unregister(struct hwrng *rng)
>  
>  	if (list_empty(&rng_list)) {
>  		mutex_unlock(&rng_mutex);
> -		unregister_miscdev();
>  		if (hwrng_fill)
>  			kthread_stop(hwrng_fill);
>  	} else
> @@ -540,16 +533,24 @@ void hwrng_unregister(struct hwrng *rng)
>  }
>  EXPORT_SYMBOL_GPL(hwrng_unregister);
>  
> -static void __exit hwrng_exit(void)
> +static int __init hwrng_modinit(void)
> +{
> +	return register_miscdev();
> +}
> +
> +static void __exit hwrng_modexit(void)
>  {
>  	mutex_lock(&rng_mutex);
>  	BUG_ON(current_rng);
>  	kfree(rng_buffer);
>  	kfree(rng_fillbuf);
>  	mutex_unlock(&rng_mutex);
> +
> +	unregister_miscdev();
>  }
>  
> -module_exit(hwrng_exit);
> +module_init(hwrng_modinit);
> +module_exit(hwrng_modexit);
>  
>  MODULE_DESCRIPTION("H/W Random Number Generator (RNG) driver");
>  MODULE_LICENSE("GPL");

Re: [PATCH 2/5] hwrng: core - Fix current_rng init/cleanup race yet again

[Herbert Xu]

On Wed, Dec 24, 2014 at 09:56:36AM +1030, Rusty Russell wrote:
>
> I'll have to pull the tree to review it properly, but the theory was
> that the reference count was counting users of the rng.  Now I don't
> know what it's counting:

The reference count starts off at zero, meaning that the RNG
has not been initialised.  It becomes one when we do hwrng_init.
After that each user adds/subtracts from it.  When it hits zero
we will clean it up and await for either deregistration or the
next hwrng_init.

> >  static inline int hwrng_init(struct hwrng *rng)
> >  {
> > +	if (kref_get_unless_zero(&rng->ref))
> > +		goto skip_init;
> > +
> >  	if (rng->init) {
> >  		int ret;
> 
> OK, so this skip_init branch is triggered when the rng is being
> shut down as it's no longer current_rng?

Right, if it triggers it means that we have already been initialised
previously and we have not yet executed cleanup on the RNG even
though at some point another RNG came in and became current_rng.

Bottom line is that this RNG is still good and we don't need to
(can't) initialise it.

> > +
> > +	kref_init(&rng->ref);
> > +	reinit_completion(&rng->cleanup_done);
> > +
> > +skip_init:
> >  	add_early_randomness(rng);
> 
> Then we use it to add randomness?

Honestly I don't care about whether we add randomness or not
in this case but this is what the code did before.  If you dislike
that feel free to send in a patch to kill this too.

> > @@ -467,6 +474,9 @@ int hwrng_register(struct hwrng *rng)
> >  			goto out_unlock;
> >  	}
> >  
> > +	init_completion(&rng->cleanup_done);
> > +	complete(&rng->cleanup_done);
> > +
> 
> This code smells very bad.

Well, it would smell better if someone added a helper that lets
you initialise a completion that is already complete :)

Point is the RNG must only be in two states.  Either it's the
current RNG in which case cleanup_done must be false/incomplete,
or it's not and cleanup_done must be either already complete or
will be complete at some point in the future.

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

Re: [PATCH 3/5] hwrng: core - Do not register device opportunistically

[Herbert Xu]

On Wed, Dec 24, 2014 at 09:59:41AM +1030, Rusty Russell wrote:
>
> Does this break userspace by creating a device which will just return
> -ENODEV on read?  Sure, callers *should* handle it...

If somebody complains we could easily fix this by making open
fail.  In fact, if such applications exist then they're likely
to want to hold onto a reference to the RNG after then open...

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