[PATCH 0/3] crypto hwrng consider quality value, remember user choice

[PATCH 0/3] crypto hwrng consider quality value, remember user choice

[Harald Freudenberger]

The hwrng core implementation currently doesn't consider the
quality field of the struct hwrng. So the first registered rng
is the winner and further rng sources even with much better
quality are ignored.

The behavior should be that always the best rng with the highest
quality rate should be used as current rng source. Only if the
user explicitly chooses a rng source (via writing a rng name
to /sys/class/misc/hw_random/rng_current) the decision for the
best quality should be suppressed.

This set of patches makes hwrng always hold a list of registered
rng sources sorted decreasing by quality. On registration of a new
hwrng source the list is updated and if the current rng source was
not chosen by user and the new rng provides better quality set as
new current rng source. Similar on unregistration of an rng, if it
was the current used rng source the one with the next highest quality
is used. If a rng source has been set via sysfs from userland as
long as this one doesn't unregister it is kept as current rng
regardless of registration of 'better' rng sources.

Patch 1 introduces the sorted list of registered rngs and the
always use the best quality behavior.

Patch 2 makes hwrng remember that the user has selected an
rng via echo to /sys/class/misc/hw_random/rng_current.

Patch 3 adds a new sysfs attribute file 'rng_selected' to the
rng core. This file shows the chosen rng name if a selection
from userspace took place otherwise 'none'.

Patch 3 is just a simple implementation of an possible improvement
and may act as a starting point for further discussions. For example,
the implementation could be reworked to accept also currently not
known rng sources and upon appearing instantly select this user
chosen rng. However, this would require to hold an string buffer
and this would introduce some string length limit on the rng name.
Another idea is that there should be a possibility to unselect
the user's choice. An echo 'none' to rng_current may be a way to
remove the selection and the hwrng may act by using the quality best
rng.

Harald Freudenberger (3):
  crypto: hwrng use rng source with best quality
  crypto: hwrng remember rng chosen by user
  crypto: hwrng add sysfs attribute to show user selected rng

 drivers/char/hw_random/core.c | 43 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH 1/3] crypto: hwrng use rng source with best quality

[Harald Freudenberger]

This patch rewoks the hwrng to always use the
rng source with best entropy quality.

On registation and unregistration the hwrng now
tries to choose the best (= highest quality value)
rng source. The handling of the internal list
of registered rng sources is now always sorted
by quality and the top most rng chosen.

Signed-off-by: Harald Freudenberger <freude@linux.vnet.ibm.com>
---
 drivers/char/hw_random/core.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 503a41d..e9dda16 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -29,6 +29,7 @@
 
 static struct hwrng *current_rng;
 static struct task_struct *hwrng_fill;
+/* list of registered rngs, sorted decending by quality */
 static LIST_HEAD(rng_list);
 /* Protects rng_list and current_rng */
 static DEFINE_MUTEX(rng_mutex);
@@ -417,6 +418,7 @@ int hwrng_register(struct hwrng *rng)
 {
 	int err = -EINVAL;
 	struct hwrng *old_rng, *tmp;
+	struct list_head *rng_list_ptr;
 
 	if (!rng->name || (!rng->data_read && !rng->read))
 		goto out;
@@ -432,14 +434,25 @@ int hwrng_register(struct hwrng *rng)
 	init_completion(&rng->cleanup_done);
 	complete(&rng->cleanup_done);
 
+	/* rng_list is sorted by decreasing quality */
+	list_for_each(rng_list_ptr, &rng_list) {
+		tmp = list_entry(rng_list_ptr, struct hwrng, list);
+		if (tmp->quality < rng->quality)
+			break;
+	}
+	list_add_tail(&rng->list, rng_list_ptr);
+
 	old_rng = current_rng;
 	err = 0;
-	if (!old_rng) {
+	if (!old_rng || (rng->quality > old_rng->quality)) {
+		/*
+		 * Set new rng as current as the new rng source
+		 * provides better entropy quality.
+		 */
 		err = set_current_rng(rng);
 		if (err)
 			goto out_unlock;
 	}
-	list_add_tail(&rng->list, &rng_list);
 
 	if (old_rng && !rng->init) {
 		/*
@@ -466,12 +479,12 @@ void hwrng_unregister(struct hwrng *rng)
 	list_del(&rng->list);
 	if (current_rng == rng) {
 		drop_current_rng();
+		/* rng_list is sorted by quality, use the best (=first) one */
 		if (!list_empty(&rng_list)) {
-			struct hwrng *tail;
-
-			tail = list_entry(rng_list.prev, struct hwrng, list);
+			struct hwrng *new_rng;
 
-			set_current_rng(tail);
+			new_rng = list_entry(rng_list.next, struct hwrng, list);
+			set_current_rng(new_rng);
 		}
 	}
 
-- 
2.7.4

[PATCH 2/3] crypto: hwrng remember rng chosen by user

[Harald Freudenberger]

When a user chooses a rng source via sysfs
attribute this rng should be sticky, even
when other sources with better quality to
register. This patch introduces a simple way
to remember the user's choise.

Signed-off-by: Harald Freudenberger <freude@linux.vnet.ibm.com>
---
 drivers/char/hw_random/core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index e9dda16..ffd4e36 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -28,6 +28,8 @@
 #define RNG_MODULE_NAME		"hw_random"
 
 static struct hwrng *current_rng;
+/* the current rng has been explicitly chosen by user via sysfs */
+static int cur_rng_set_by_user;
 static struct task_struct *hwrng_fill;
 /* list of registered rngs, sorted decending by quality */
 static LIST_HEAD(rng_list);
@@ -304,6 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
 	list_for_each_entry(rng, &rng_list, list) {
 		if (sysfs_streq(rng->name, buf)) {
 			err = 0;
+			cur_rng_set_by_user = 1;
 			if (rng != current_rng)
 				err = set_current_rng(rng);
 			break;
@@ -444,10 +447,12 @@ int hwrng_register(struct hwrng *rng)
 
 	old_rng = current_rng;
 	err = 0;
-	if (!old_rng || (rng->quality > old_rng->quality)) {
+	if (!old_rng ||
+	    (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
 		/*
 		 * Set new rng as current as the new rng source
-		 * provides better entropy quality.
+		 * provides better entropy quality and was not
+		 * chosen by userspace.
 		 */
 		err = set_current_rng(rng);
 		if (err)
@@ -479,6 +484,7 @@ void hwrng_unregister(struct hwrng *rng)
 	list_del(&rng->list);
 	if (current_rng == rng) {
 		drop_current_rng();
+		cur_rng_set_by_user = 0;
 		/* rng_list is sorted by quality, use the best (=first) one */
 		if (!list_empty(&rng_list)) {
 			struct hwrng *new_rng;
-- 
2.7.4

[PATCH 3/3] crypto: hwrng add sysfs attribute to show user selected rng

[Harald Freudenberger]

This patch introduces a new sysfs attribute file 'rng_selected'
which shows the the rng chosen by userspace.

If a rng source is chosen by user via echo some valid string
to rng_current there should be a way to signal this choice to
userspace. The new attribute file 'rng_selected' shows either
the name of the rng chosen or 'none'.

Signed-off-by: Harald Freudenberger <freude@linux.vnet.ibm.com>
---
 drivers/char/hw_random/core.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index ffd4e36..6a6276a 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -28,8 +28,8 @@
 #define RNG_MODULE_NAME		"hw_random"
 
 static struct hwrng *current_rng;
-/* the current rng has been explicitly chosen by user via sysfs */
-static int cur_rng_set_by_user;
+/* the rng explicitly selected by user via sysfs */
+static struct hwrng *selected_rng;
 static struct task_struct *hwrng_fill;
 /* list of registered rngs, sorted decending by quality */
 static LIST_HEAD(rng_list);
@@ -306,7 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
 	list_for_each_entry(rng, &rng_list, list) {
 		if (sysfs_streq(rng->name, buf)) {
 			err = 0;
-			cur_rng_set_by_user = 1;
+			selected_rng = rng;
 			if (rng != current_rng)
 				err = set_current_rng(rng);
 			break;
@@ -355,16 +355,28 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
 	return strlen(buf);
 }
 
+static ssize_t hwrng_attr_selected_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			selected_rng ? selected_rng->name : "none");
+}
+
 static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
 		   hwrng_attr_current_show,
 		   hwrng_attr_current_store);
 static DEVICE_ATTR(rng_available, S_IRUGO,
 		   hwrng_attr_available_show,
 		   NULL);
+static DEVICE_ATTR(rng_selected, S_IRUGO,
+		   hwrng_attr_selected_show,
+		   NULL);
 
 static struct attribute *rng_dev_attrs[] = {
 	&dev_attr_rng_current.attr,
 	&dev_attr_rng_available.attr,
+	&dev_attr_rng_selected.attr,
 	NULL
 };
 
@@ -448,7 +460,7 @@ int hwrng_register(struct hwrng *rng)
 	old_rng = current_rng;
 	err = 0;
 	if (!old_rng ||
-	    (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
+	    (!selected_rng && rng->quality > old_rng->quality)) {
 		/*
 		 * Set new rng as current as the new rng source
 		 * provides better entropy quality and was not
@@ -484,7 +496,7 @@ void hwrng_unregister(struct hwrng *rng)
 	list_del(&rng->list);
 	if (current_rng == rng) {
 		drop_current_rng();
-		cur_rng_set_by_user = 0;
+		selected_rng = NULL;
 		/* rng_list is sorted by quality, use the best (=first) one */
 		if (!list_empty(&rng_list)) {
 			struct hwrng *new_rng;
-- 
2.7.4

Re: [PATCH 3/3] crypto: hwrng add sysfs attribute to show user selected rng

[PrasannaKumar Muralidharan]

On 3 July 2017 at 15:33, Harald Freudenberger <freude@linux.vnet.ibm.com> wrote:
> This patch introduces a new sysfs attribute file 'rng_selected'
> which shows the the rng chosen by userspace.
>
> If a rng source is chosen by user via echo some valid string
> to rng_current there should be a way to signal this choice to
> userspace. The new attribute file 'rng_selected' shows either
> the name of the rng chosen or 'none'.
>
> Signed-off-by: Harald Freudenberger <freude@linux.vnet.ibm.com>
> ---
>  drivers/char/hw_random/core.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index ffd4e36..6a6276a 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -28,8 +28,8 @@
>  #define RNG_MODULE_NAME                "hw_random"
>
>  static struct hwrng *current_rng;
> -/* the current rng has been explicitly chosen by user via sysfs */
> -static int cur_rng_set_by_user;
> +/* the rng explicitly selected by user via sysfs */
> +static struct hwrng *selected_rng;
>  static struct task_struct *hwrng_fill;
>  /* list of registered rngs, sorted decending by quality */
>  static LIST_HEAD(rng_list);
> @@ -306,7 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
>         list_for_each_entry(rng, &rng_list, list) {
>                 if (sysfs_streq(rng->name, buf)) {
>                         err = 0;
> -                       cur_rng_set_by_user = 1;
> +                       selected_rng = rng;
>                         if (rng != current_rng)
>                                 err = set_current_rng(rng);
>                         break;
> @@ -355,16 +355,28 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
>         return strlen(buf);
>  }
>
> +static ssize_t hwrng_attr_selected_show(struct device *dev,
> +                                       struct device_attribute *attr,
> +                                       char *buf)
> +{
> +       return snprintf(buf, PAGE_SIZE, "%s\n",
> +                       selected_rng ? selected_rng->name : "none");
> +}
> +
>  static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
>                    hwrng_attr_current_show,
>                    hwrng_attr_current_store);
>  static DEVICE_ATTR(rng_available, S_IRUGO,
>                    hwrng_attr_available_show,
>                    NULL);
> +static DEVICE_ATTR(rng_selected, S_IRUGO,
> +                  hwrng_attr_selected_show,
> +                  NULL);
>
>  static struct attribute *rng_dev_attrs[] = {
>         &dev_attr_rng_current.attr,
>         &dev_attr_rng_available.attr,
> +       &dev_attr_rng_selected.attr,
>         NULL
>  };
>
> @@ -448,7 +460,7 @@ int hwrng_register(struct hwrng *rng)
>         old_rng = current_rng;
>         err = 0;
>         if (!old_rng ||
> -           (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
> +           (!selected_rng && rng->quality > old_rng->quality)) {
>                 /*
>                  * Set new rng as current as the new rng source
>                  * provides better entropy quality and was not
> @@ -484,7 +496,7 @@ void hwrng_unregister(struct hwrng *rng)
>         list_del(&rng->list);
>         if (current_rng == rng) {
>                 drop_current_rng();
> -               cur_rng_set_by_user = 0;
> +               selected_rng = NULL;
>                 /* rng_list is sorted by quality, use the best (=first) one */
>                 if (!list_empty(&rng_list)) {
>                         struct hwrng *new_rng;
> --
> 2.7.4
>

The current_rng sysfs attribute shows currently selected rng. So this
new attribute can contain 1 to indicate user's choice, 0 otherwise.

Regards,
PrasannaKumar

Re: [PATCH 1/3] crypto: hwrng use rng source with best quality

[PrasannaKumar Muralidharan]

On 3 July 2017 at 15:33, Harald Freudenberger <freude@linux.vnet.ibm.com> wrote:
> This patch rewoks the hwrng to always use the
> rng source with best entropy quality.
>
> On registation and unregistration the hwrng now
> tries to choose the best (= highest quality value)
> rng source. The handling of the internal list
> of registered rng sources is now always sorted
> by quality and the top most rng chosen.
>
> Signed-off-by: Harald Freudenberger <freude@linux.vnet.ibm.com>
> ---
>  drivers/char/hw_random/core.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 503a41d..e9dda16 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -29,6 +29,7 @@
>
>  static struct hwrng *current_rng;
>  static struct task_struct *hwrng_fill;
> +/* list of registered rngs, sorted decending by quality */
>  static LIST_HEAD(rng_list);
>  /* Protects rng_list and current_rng */
>  static DEFINE_MUTEX(rng_mutex);
> @@ -417,6 +418,7 @@ int hwrng_register(struct hwrng *rng)
>  {
>         int err = -EINVAL;
>         struct hwrng *old_rng, *tmp;
> +       struct list_head *rng_list_ptr;
>
>         if (!rng->name || (!rng->data_read && !rng->read))
>                 goto out;
> @@ -432,14 +434,25 @@ int hwrng_register(struct hwrng *rng)
>         init_completion(&rng->cleanup_done);
>         complete(&rng->cleanup_done);
>
> +       /* rng_list is sorted by decreasing quality */
> +       list_for_each(rng_list_ptr, &rng_list) {
> +               tmp = list_entry(rng_list_ptr, struct hwrng, list);
> +               if (tmp->quality < rng->quality)
> +                       break;
> +       }
> +       list_add_tail(&rng->list, rng_list_ptr);
> +
>         old_rng = current_rng;
>         err = 0;
> -       if (!old_rng) {
> +       if (!old_rng || (rng->quality > old_rng->quality)) {
> +               /*
> +                * Set new rng as current as the new rng source
> +                * provides better entropy quality.
> +                */
>                 err = set_current_rng(rng);
>                 if (err)
>                         goto out_unlock;
>         }
> -       list_add_tail(&rng->list, &rng_list);
>
>         if (old_rng && !rng->init) {
>                 /*
> @@ -466,12 +479,12 @@ void hwrng_unregister(struct hwrng *rng)
>         list_del(&rng->list);
>         if (current_rng == rng) {
>                 drop_current_rng();
> +               /* rng_list is sorted by quality, use the best (=first) one */
>                 if (!list_empty(&rng_list)) {
> -                       struct hwrng *tail;
> -
> -                       tail = list_entry(rng_list.prev, struct hwrng, list);
> +                       struct hwrng *new_rng;
>
> -                       set_current_rng(tail);
> +                       new_rng = list_entry(rng_list.next, struct hwrng, list);
> +                       set_current_rng(new_rng);
>                 }
>         }
>
> --
> 2.7.4
>

Looks good to me.
Reviewed-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>.

Regards,
PrasannaKumar

Re: [PATCH 2/3] crypto: hwrng remember rng chosen by user

[PrasannaKumar Muralidharan]

Hi Harald,

On 3 July 2017 at 15:33, Harald Freudenberger <freude@linux.vnet.ibm.com> wrote:
> When a user chooses a rng source via sysfs
> attribute this rng should be sticky, even
> when other sources with better quality to
> register. This patch introduces a simple way
> to remember the user's choise.
>
> Signed-off-by: Harald Freudenberger <freude@linux.vnet.ibm.com>
> ---
>  drivers/char/hw_random/core.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index e9dda16..ffd4e36 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -28,6 +28,8 @@
>  #define RNG_MODULE_NAME                "hw_random"
>
>  static struct hwrng *current_rng;
> +/* the current rng has been explicitly chosen by user via sysfs */
> +static int cur_rng_set_by_user;
>  static struct task_struct *hwrng_fill;
>  /* list of registered rngs, sorted decending by quality */
>  static LIST_HEAD(rng_list);
> @@ -304,6 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
>         list_for_each_entry(rng, &rng_list, list) {
>                 if (sysfs_streq(rng->name, buf)) {
>                         err = 0;
> +                       cur_rng_set_by_user = 1;
>                         if (rng != current_rng)
>                                 err = set_current_rng(rng);
>                         break;
> @@ -444,10 +447,12 @@ int hwrng_register(struct hwrng *rng)
>
>         old_rng = current_rng;
>         err = 0;
> -       if (!old_rng || (rng->quality > old_rng->quality)) {
> +       if (!old_rng ||
> +           (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
>                 /*
>                  * Set new rng as current as the new rng source
> -                * provides better entropy quality.
> +                * provides better entropy quality and was not
> +                * chosen by userspace.
>                  */
>                 err = set_current_rng(rng);
>                 if (err)
> @@ -479,6 +484,7 @@ void hwrng_unregister(struct hwrng *rng)
>         list_del(&rng->list);
>         if (current_rng == rng) {
>                 drop_current_rng();
> +               cur_rng_set_by_user = 0;
>                 /* rng_list is sorted by quality, use the best (=first) one */
>                 if (!list_empty(&rng_list)) {
>                         struct hwrng *new_rng;
> --
> 2.7.4
>

Looks good to me. Reviewed-by: PrasannaKumar Muralidharan
<prasannatsmkumar@gmail.com>.

Regards,
PrasannaKumar

Re: [PATCH 3/3] crypto: hwrng add sysfs attribute to show user selected rng

[Harald Freudenberger]

On 07/04/2017 03:15 PM, PrasannaKumar Muralidharan wrote:
> On 3 July 2017 at 15:33, Harald Freudenberger <freude@linux.vnet.ibm.com> wrote:
>> This patch introduces a new sysfs attribute file 'rng_selected'
>> which shows the the rng chosen by userspace.
>>
>> If a rng source is chosen by user via echo some valid string
>> to rng_current there should be a way to signal this choice to
>> userspace. The new attribute file 'rng_selected' shows either
>> the name of the rng chosen or 'none'.
>>
>> Signed-off-by: Harald Freudenberger <freude@linux.vnet.ibm.com>
>> ---
>>  drivers/char/hw_random/core.c | 22 +++++++++++++++++-----
>>  1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
>> index ffd4e36..6a6276a 100644
>> --- a/drivers/char/hw_random/core.c
>> +++ b/drivers/char/hw_random/core.c
>> @@ -28,8 +28,8 @@
>>  #define RNG_MODULE_NAME                "hw_random"
>>
>>  static struct hwrng *current_rng;
>> -/* the current rng has been explicitly chosen by user via sysfs */
>> -static int cur_rng_set_by_user;
>> +/* the rng explicitly selected by user via sysfs */
>> +static struct hwrng *selected_rng;
>>  static struct task_struct *hwrng_fill;
>>  /* list of registered rngs, sorted decending by quality */
>>  static LIST_HEAD(rng_list);
>> @@ -306,7 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
>>         list_for_each_entry(rng, &rng_list, list) {
>>                 if (sysfs_streq(rng->name, buf)) {
>>                         err = 0;
>> -                       cur_rng_set_by_user = 1;
>> +                       selected_rng = rng;
>>                         if (rng != current_rng)
>>                                 err = set_current_rng(rng);
>>                         break;
>> @@ -355,16 +355,28 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
>>         return strlen(buf);
>>  }
>>
>> +static ssize_t hwrng_attr_selected_show(struct device *dev,
>> +                                       struct device_attribute *attr,
>> +                                       char *buf)
>> +{
>> +       return snprintf(buf, PAGE_SIZE, "%s\n",
>> +                       selected_rng ? selected_rng->name : "none");
>> +}
>> +
>>  static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
>>                    hwrng_attr_current_show,
>>                    hwrng_attr_current_store);
>>  static DEVICE_ATTR(rng_available, S_IRUGO,
>>                    hwrng_attr_available_show,
>>                    NULL);
>> +static DEVICE_ATTR(rng_selected, S_IRUGO,
>> +                  hwrng_attr_selected_show,
>> +                  NULL);
>>
>>  static struct attribute *rng_dev_attrs[] = {
>>         &dev_attr_rng_current.attr,
>>         &dev_attr_rng_available.attr,
>> +       &dev_attr_rng_selected.attr,
>>         NULL
>>  };
>>
>> @@ -448,7 +460,7 @@ int hwrng_register(struct hwrng *rng)
>>         old_rng = current_rng;
>>         err = 0;
>>         if (!old_rng ||
>> -           (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
>> +           (!selected_rng && rng->quality > old_rng->quality)) {
>>                 /*
>>                  * Set new rng as current as the new rng source
>>                  * provides better entropy quality and was not
>> @@ -484,7 +496,7 @@ void hwrng_unregister(struct hwrng *rng)
>>         list_del(&rng->list);
>>         if (current_rng == rng) {
>>                 drop_current_rng();
>> -               cur_rng_set_by_user = 0;
>> +               selected_rng = NULL;
>>                 /* rng_list is sorted by quality, use the best (=first) one */
>>                 if (!list_empty(&rng_list)) {
>>                         struct hwrng *new_rng;
>> --
>> 2.7.4
>>
> The current_rng sysfs attribute shows currently selected rng. So this
> new attribute can contain 1 to indicate user's choice, 0 otherwise.
>
> Regards,
> PrasannaKumar
>
Here is an updated version with just showing 0 or 1 in the new sysfs
attribute file:
========== cut ==========
From: Harald Freudenberger <freude@linux.vnet.ibm.com>
Date: Mon, 3 Jul 2017 10:19:22 +0200
Subject: [PATCH 3/3] crypto: hwrng add sysfs attribute to show user selected
 rng

This patch introduces a new sysfs attribute file 'rng_selected'
which shows if the current rng has been chosen by userspace.

If a rng source is chosen by user via echo some valid string
to rng_current there should be a way to signal this choice to
userspace. The new attribute file 'rng_selected' shows '1' for
user selecte rng and '0' otherwise.

Signed-off-by: Harald Freudenberger <freude@linux.vnet.ibm.com>
---
 drivers/char/hw_random/core.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index ffd4e36..2b4c7f6 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -28,8 +28,8 @@
 #define RNG_MODULE_NAME        "hw_random"
 
 static struct hwrng *current_rng;
-/* the current rng has been explicitly chosen by user via sysfs */
-static int cur_rng_set_by_user;
+/* the rng explicitly selected by user via sysfs */
+static struct hwrng *selected_rng;
 static struct task_struct *hwrng_fill;
 /* list of registered rngs, sorted decending by quality */
 static LIST_HEAD(rng_list);
@@ -306,7 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
     list_for_each_entry(rng, &rng_list, list) {
         if (sysfs_streq(rng->name, buf)) {
             err = 0;
-            cur_rng_set_by_user = 1;
+            selected_rng = rng;
             if (rng != current_rng)
                 err = set_current_rng(rng);
             break;
@@ -355,16 +355,27 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
     return strlen(buf);
 }
 
+static ssize_t hwrng_attr_selected_show(struct device *dev,
+                    struct device_attribute *attr,
+                    char *buf)
+{
+    return snprintf(buf, PAGE_SIZE, "%d\n", selected_rng ? 1 : 0);
+}
+
 static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
            hwrng_attr_current_show,
            hwrng_attr_current_store);
 static DEVICE_ATTR(rng_available, S_IRUGO,
            hwrng_attr_available_show,
            NULL);
+static DEVICE_ATTR(rng_selected, S_IRUGO,
+           hwrng_attr_selected_show,
+           NULL);
 
 static struct attribute *rng_dev_attrs[] = {
     &dev_attr_rng_current.attr,
     &dev_attr_rng_available.attr,
+    &dev_attr_rng_selected.attr,
     NULL
 };
 
@@ -448,7 +459,7 @@ int hwrng_register(struct hwrng *rng)
     old_rng = current_rng;
     err = 0;
     if (!old_rng ||
-        (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
+        (!selected_rng && rng->quality > old_rng->quality)) {
         /*
          * Set new rng as current as the new rng source
          * provides better entropy quality and was not
@@ -484,7 +495,7 @@ void hwrng_unregister(struct hwrng *rng)
     list_del(&rng->list);
     if (current_rng == rng) {
         drop_current_rng();
-        cur_rng_set_by_user = 0;
+        selected_rng = NULL;
         /* rng_list is sorted by quality, use the best (=first) one */
         if (!list_empty(&rng_list)) {
             struct hwrng *new_rng;
-- 
2.7.4
========== cut ==========

Re: [PATCH 3/3] crypto: hwrng add sysfs attribute to show user selected rng

[PrasannaKumar Muralidharan]

Hi Harald,

> Here is an updated version with just showing 0 or 1 in the new sysfs
> attribute file:
> ========== cut ==========
> From: Harald Freudenberger <freude@linux.vnet.ibm.com>
> Date: Mon, 3 Jul 2017 10:19:22 +0200
> Subject: [PATCH 3/3] crypto: hwrng add sysfs attribute to show user selected
>  rng
>
> This patch introduces a new sysfs attribute file 'rng_selected'
> which shows if the current rng has been chosen by userspace.
>
> If a rng source is chosen by user via echo some valid string
> to rng_current there should be a way to signal this choice to
> userspace. The new attribute file 'rng_selected' shows '1' for
> user selecte rng and '0' otherwise.
>
> Signed-off-by: Harald Freudenberger <freude@linux.vnet.ibm.com>
> ---
>  drivers/char/hw_random/core.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index ffd4e36..2b4c7f6 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -28,8 +28,8 @@
>  #define RNG_MODULE_NAME        "hw_random"
>
>  static struct hwrng *current_rng;
> -/* the current rng has been explicitly chosen by user via sysfs */
> -static int cur_rng_set_by_user;
> +/* the rng explicitly selected by user via sysfs */
> +static struct hwrng *selected_rng;
>  static struct task_struct *hwrng_fill;
>  /* list of registered rngs, sorted decending by quality */
>  static LIST_HEAD(rng_list);
> @@ -306,7 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
>      list_for_each_entry(rng, &rng_list, list) {
>          if (sysfs_streq(rng->name, buf)) {
>              err = 0;
> -            cur_rng_set_by_user = 1;
> +            selected_rng = rng;
>              if (rng != current_rng)
>                  err = set_current_rng(rng);
>              break;
> @@ -355,16 +355,27 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
>      return strlen(buf);
>  }
>
> +static ssize_t hwrng_attr_selected_show(struct device *dev,
> +                    struct device_attribute *attr,
> +                    char *buf)
> +{
> +    return snprintf(buf, PAGE_SIZE, "%d\n", selected_rng ? 1 : 0);
> +}
> +
>  static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
>             hwrng_attr_current_show,
>             hwrng_attr_current_store);
>  static DEVICE_ATTR(rng_available, S_IRUGO,
>             hwrng_attr_available_show,
>             NULL);
> +static DEVICE_ATTR(rng_selected, S_IRUGO,
> +           hwrng_attr_selected_show,
> +           NULL);
>
>  static struct attribute *rng_dev_attrs[] = {
>      &dev_attr_rng_current.attr,
>      &dev_attr_rng_available.attr,
> +    &dev_attr_rng_selected.attr,
>      NULL
>  };
>
> @@ -448,7 +459,7 @@ int hwrng_register(struct hwrng *rng)
>      old_rng = current_rng;
>      err = 0;
>      if (!old_rng ||
> -        (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
> +        (!selected_rng && rng->quality > old_rng->quality)) {
>          /*
>           * Set new rng as current as the new rng source
>           * provides better entropy quality and was not
> @@ -484,7 +495,7 @@ void hwrng_unregister(struct hwrng *rng)
>      list_del(&rng->list);
>      if (current_rng == rng) {
>          drop_current_rng();
> -        cur_rng_set_by_user = 0;
> +        selected_rng = NULL;
>          /* rng_list is sorted by quality, use the best (=first) one */
>          if (!list_empty(&rng_list)) {
>              struct hwrng *new_rng;
> --
> 2.7.4
> ========== cut ==========
>

Nice to see quick turn around time. Why not just use the
cur_rng_set_by_user instead of selected_rng? This way patch 3 won't
change things that patch 2 introduced and patch 3 will become much
smaller. Also I feel folding patch 3 and 2 will make sense as both are
related to user selected rng.

Regards,
PrasannaKumar

Re: [PATCH 3/3] crypto: hwrng add sysfs attribute to show user selected rng

[Harald Freudenberger]

On 07/06/2017 06:51 AM, PrasannaKumar Muralidharan wrote:
> Hi Harald,
>
>> Here is an updated version with just showing 0 or 1 in the new sysfs
>> attribute file:
>> ========== cut ==========
>> From: Harald Freudenberger <freude@linux.vnet.ibm.com>
>> Date: Mon, 3 Jul 2017 10:19:22 +0200
>> Subject: [PATCH 3/3] crypto: hwrng add sysfs attribute to show user selected
>>  rng
>>
>> This patch introduces a new sysfs attribute file 'rng_selected'
>> which shows if the current rng has been chosen by userspace.
...
> Nice to see quick turn around time. Why not just use the
> cur_rng_set_by_user instead of selected_rng? This way patch 3 won't
> change things that patch 2 introduced and patch 3 will become much
> smaller. Also I feel folding patch 3 and 2 will make sense as both are
> related to user selected rng.
>
> Regards,
> PrasannaKumar
>
Hello PrasannaKumar

here is now a version which combines patch 2 and patch 3 together
according to your suggestions:

========== cut ==========

From: Harald Freudenberger <freude@linux.vnet.ibm.com>
Date: Fri, 30 Jun 2017 17:06:40 +0200
Subject: [PATCH 2/2] crypto: hwrng remember rng chosen by user

When a user chooses a rng source via sysfs attribute
this rng should be sticky, even when other sources
with better quality to register. This patch introduces
a simple way to remember the user's choice. This is
reflected by a new sysfs attribute file 'rng_selected'
which shows if the current rng has been chosen by
userspace. The new attribute file shows '1' for user
selected rng and '0' otherwise.

Signed-off-by: Harald Freudenberger <freude@linux.vnet.ibm.com>
---
 drivers/char/hw_random/core.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index e9dda16..9701ac7 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -28,6 +28,8 @@
 #define RNG_MODULE_NAME        "hw_random"
 
 static struct hwrng *current_rng;
+/* the current rng has been explicitly chosen by user via sysfs */
+static int cur_rng_set_by_user;
 static struct task_struct *hwrng_fill;
 /* list of registered rngs, sorted decending by quality */
 static LIST_HEAD(rng_list);
@@ -304,6 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
     list_for_each_entry(rng, &rng_list, list) {
         if (sysfs_streq(rng->name, buf)) {
             err = 0;
+            cur_rng_set_by_user = 1;
             if (rng != current_rng)
                 err = set_current_rng(rng);
             break;
@@ -352,16 +355,27 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
     return strlen(buf);
 }
 
+static ssize_t hwrng_attr_selected_show(struct device *dev,
+                    struct device_attribute *attr,
+                    char *buf)
+{
+    return snprintf(buf, PAGE_SIZE, "%d\n", cur_rng_set_by_user);
+}
+
 static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
            hwrng_attr_current_show,
            hwrng_attr_current_store);
 static DEVICE_ATTR(rng_available, S_IRUGO,
            hwrng_attr_available_show,
            NULL);
+static DEVICE_ATTR(rng_selected, S_IRUGO,
+           hwrng_attr_selected_show,
+           NULL);
 
 static struct attribute *rng_dev_attrs[] = {
     &dev_attr_rng_current.attr,
     &dev_attr_rng_available.attr,
+    &dev_attr_rng_selected.attr,
     NULL
 };
 
@@ -444,10 +458,12 @@ int hwrng_register(struct hwrng *rng)
 
     old_rng = current_rng;
     err = 0;
-    if (!old_rng || (rng->quality > old_rng->quality)) {
+    if (!old_rng ||
+        (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
         /*
          * Set new rng as current as the new rng source
-         * provides better entropy quality.
+         * provides better entropy quality and was not
+         * chosen by userspace.
          */
         err = set_current_rng(rng);
         if (err)
@@ -479,6 +495,7 @@ void hwrng_unregister(struct hwrng *rng)
     list_del(&rng->list);
     if (current_rng == rng) {
         drop_current_rng();
+        cur_rng_set_by_user = 0;
         /* rng_list is sorted by quality, use the best (=first) one */
         if (!list_empty(&rng_list)) {
             struct hwrng *new_rng;
-- 
2.7.4

========== cut ==========

Re: [PATCH 3/3] crypto: hwrng add sysfs attribute to show user selected rng

[PrasannaKumar Muralidharan]

Hi Harald,

> Hello PrasannaKumar
>
> here is now a version which combines patch 2 and patch 3 together
> according to your suggestions:
>
> ========== cut ==========
>
> From: Harald Freudenberger <freude@linux.vnet.ibm.com>
> Date: Fri, 30 Jun 2017 17:06:40 +0200
> Subject: [PATCH 2/2] crypto: hwrng remember rng chosen by user
>
> When a user chooses a rng source via sysfs attribute
> this rng should be sticky, even when other sources
> with better quality to register. This patch introduces
> a simple way to remember the user's choice. This is
> reflected by a new sysfs attribute file 'rng_selected'
> which shows if the current rng has been chosen by
> userspace. The new attribute file shows '1' for user
> selected rng and '0' otherwise.
>
> Signed-off-by: Harald Freudenberger <freude@linux.vnet.ibm.com>
> ---
>  drivers/char/hw_random/core.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index e9dda16..9701ac7 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -28,6 +28,8 @@
>  #define RNG_MODULE_NAME        "hw_random"
>
>  static struct hwrng *current_rng;
> +/* the current rng has been explicitly chosen by user via sysfs */
> +static int cur_rng_set_by_user;
>  static struct task_struct *hwrng_fill;
>  /* list of registered rngs, sorted decending by quality */
>  static LIST_HEAD(rng_list);
> @@ -304,6 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
>      list_for_each_entry(rng, &rng_list, list) {
>          if (sysfs_streq(rng->name, buf)) {
>              err = 0;
> +            cur_rng_set_by_user = 1;
>              if (rng != current_rng)
>                  err = set_current_rng(rng);
>              break;
> @@ -352,16 +355,27 @@ static ssize_t hwrng_attr_available_show(struct device *dev,
>      return strlen(buf);
>  }
>
> +static ssize_t hwrng_attr_selected_show(struct device *dev,
> +                    struct device_attribute *attr,
> +                    char *buf)
> +{
> +    return snprintf(buf, PAGE_SIZE, "%d\n", cur_rng_set_by_user);
> +}
> +
>  static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
>             hwrng_attr_current_show,
>             hwrng_attr_current_store);
>  static DEVICE_ATTR(rng_available, S_IRUGO,
>             hwrng_attr_available_show,
>             NULL);
> +static DEVICE_ATTR(rng_selected, S_IRUGO,
> +           hwrng_attr_selected_show,
> +           NULL);
>
>  static struct attribute *rng_dev_attrs[] = {
>      &dev_attr_rng_current.attr,
>      &dev_attr_rng_available.attr,
> +    &dev_attr_rng_selected.attr,
>      NULL
>  };
>
> @@ -444,10 +458,12 @@ int hwrng_register(struct hwrng *rng)
>
>      old_rng = current_rng;
>      err = 0;
> -    if (!old_rng || (rng->quality > old_rng->quality)) {
> +    if (!old_rng ||
> +        (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
>          /*
>           * Set new rng as current as the new rng source
> -         * provides better entropy quality.
> +         * provides better entropy quality and was not
> +         * chosen by userspace.
>           */
>          err = set_current_rng(rng);
>          if (err)
> @@ -479,6 +495,7 @@ void hwrng_unregister(struct hwrng *rng)
>      list_del(&rng->list);
>      if (current_rng == rng) {
>          drop_current_rng();
> +        cur_rng_set_by_user = 0;
>          /* rng_list is sorted by quality, use the best (=first) one */
>          if (!list_empty(&rng_list)) {
>              struct hwrng *new_rng;
> --
> 2.7.4
>
> ========== cut ==========
>

Good work, patch looks good to me. Reviewed-by: PrasannaKumar
Muralidharan <prasannatsmkumar@gmail.com>.
I am wondering if you want to make a v2 of the whole series. You can
just put your patch 1 and 2 and send it again with description.
Requiring a v2 depends on how Herbert prefers to take patches in.

Herbert, Do you prefer a v2 of the patch series or you can take this
version of patch 2?

Note: If you are re-posting this patch as v2 please add my reviewed by
tag for both the patches.

Regards,
PrasannaKumar