All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] crypto: Make hwrng choose rng source by quality.
@ 2017-06-29 10:03 Harald Freudenberger
  2017-06-30  5:27 ` PrasannaKumar Muralidharan
  0 siblings, 1 reply; 3+ messages in thread
From: Harald Freudenberger @ 2017-06-29 10:03 UTC (permalink / raw)
  To: linux-crypto; +Cc: herbert, arnd, gregkh, schwidefsky, 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) the decision for the best quality
should be suppressed.

This patch 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.

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

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 503a41d..7fe47f8 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -28,7 +28,10 @@
 #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);
 /* Protects rng_list and current_rng */
 static DEFINE_MUTEX(rng_mutex);
@@ -308,6 +311,8 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
 			break;
 		}
 	}
+	if (!err)
+		cur_rng_set_by_user = 1;
 	mutex_unlock(&rng_mutex);
 
 	return err ? : len;
@@ -417,6 +422,7 @@ int hwrng_register(struct hwrng *rng)
 {
 	int err = -EINVAL;
 	struct hwrng *old_rng, *tmp;
+	struct list_head *ptr;
 
 	if (!rng->name || (!rng->data_read && !rng->read))
 		goto out;
@@ -432,14 +438,26 @@ 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(ptr, &rng_list) {
+		tmp = list_entry(ptr, struct hwrng, list);
+		if (tmp->quality < rng->quality)
+			break;
+	}
+	list_add_tail(&rng->list, ptr);
+
 	old_rng = current_rng;
 	err = 0;
-	if (!old_rng) {
+	if (!old_rng ||
+	    (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
+		/*
+		 * Set new rng as current if no current rng or rng was
+		 * not chosen by user and the new one has better 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 +484,13 @@ 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 *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

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH 1/2] crypto: Make hwrng choose rng source by quality.
  2017-06-29 10:03 [PATCH 1/2] crypto: Make hwrng choose rng source by quality Harald Freudenberger
@ 2017-06-30  5:27 ` PrasannaKumar Muralidharan
  2017-06-30 12:01   ` Harald Freudenberger
  0 siblings, 1 reply; 3+ messages in thread
From: PrasannaKumar Muralidharan @ 2017-06-30  5:27 UTC (permalink / raw)
  To: Harald Freudenberger
  Cc: linux-crypto, Herbert Xu, Arnd Bergmann, Greg KH, schwidefsky

Hi Harald,

Can you split this patch into two? One patch to choose rng based on
the quality and another for not overriding user decided rng.

Some more minor comments below.

On 29 June 2017 at 15:33, Harald Freudenberger
<freude@linux.vnet.ibm.com> wrote:
> 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) the decision for the best quality
> should be suppressed.
>
> This patch 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.

Nice to see the patch. This is indeed required.

> Signed-off-by: Harald Freudenberger <freude@linux.vnet.ibm.com>
> ---
>  drivers/char/hw_random/core.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 503a41d..7fe47f8 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -28,7 +28,10 @@
>  #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;

Letting the user know that the current rng was selected based on user
input would be a good option I guess. Any thoughts on this?

>  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);
> @@ -308,6 +311,8 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
>                         break;
>                 }
>         }
> +       if (!err)
> +               cur_rng_set_by_user = 1;

This can be put inside the loop. The if condition will go away in that case.

>         mutex_unlock(&rng_mutex);
>
>         return err ? : len;
> @@ -417,6 +422,7 @@ int hwrng_register(struct hwrng *rng)
>  {
>         int err = -EINVAL;
>         struct hwrng *old_rng, *tmp;
> +       struct list_head *ptr;

Any better name instead of ptr?

>         if (!rng->name || (!rng->data_read && !rng->read))
>                 goto out;
> @@ -432,14 +438,26 @@ 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(ptr, &rng_list) {
> +               tmp = list_entry(ptr, struct hwrng, list);
> +               if (tmp->quality < rng->quality)
> +                       break;
> +       }
> +       list_add_tail(&rng->list, ptr);
> +
>         old_rng = current_rng;
>         err = 0;
> -       if (!old_rng) {
> +       if (!old_rng ||
> +           (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
> +               /*
> +                * Set new rng as current if no current rng or rng was
> +                * not chosen by user and the new one has better 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 +484,13 @@ 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 *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
>

This patch looks good. I am fine with this patch as is. Reviewed-by:
PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>

If this patch is split into please go ahead and my reviewed-by tag.

Regards,
PrasannaKumar

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH 1/2] crypto: Make hwrng choose rng source by quality.
  2017-06-30  5:27 ` PrasannaKumar Muralidharan
@ 2017-06-30 12:01   ` Harald Freudenberger
  0 siblings, 0 replies; 3+ messages in thread
From: Harald Freudenberger @ 2017-06-30 12:01 UTC (permalink / raw)
  To: PrasannaKumar Muralidharan
  Cc: linux-crypto, Herbert Xu, Arnd Bergmann, Greg KH, schwidefsky

On 06/30/2017 07:27 AM, PrasannaKumar Muralidharan wrote:
> Hi Harald,
>
> Can you split this patch into two? One patch to choose rng based on
> the quality and another for not overriding user decided rng.
>
> Some more minor comments below.
>
> On 29 June 2017 at 15:33, Harald Freudenberger
> <freude@linux.vnet.ibm.com> wrote:
>> 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) the decision for the best quality
>> should be suppressed.
>>
>> This patch 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.
> Nice to see the patch. This is indeed required.
>
>> Signed-off-by: Harald Freudenberger <freude@linux.vnet.ibm.com>
>> ---
>>  drivers/char/hw_random/core.c | 31 +++++++++++++++++++++++++------
>>  1 file changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
>> index 503a41d..7fe47f8 100644
>> --- a/drivers/char/hw_random/core.c
>> +++ b/drivers/char/hw_random/core.c
>> @@ -28,7 +28,10 @@
>>  #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;
> Letting the user know that the current rng was selected based on user
> input would be a good option I guess. Any thoughts on this?
>
>>  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);
>> @@ -308,6 +311,8 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
>>                         break;
>>                 }
>>         }
>> +       if (!err)
>> +               cur_rng_set_by_user = 1;
> This can be put inside the loop. The if condition will go away in that case.
>
>>         mutex_unlock(&rng_mutex);
>>
>>         return err ? : len;
>> @@ -417,6 +422,7 @@ int hwrng_register(struct hwrng *rng)
>>  {
>>         int err = -EINVAL;
>>         struct hwrng *old_rng, *tmp;
>> +       struct list_head *ptr;
> Any better name instead of ptr?
>
>>         if (!rng->name || (!rng->data_read && !rng->read))
>>                 goto out;
>> @@ -432,14 +438,26 @@ 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(ptr, &rng_list) {
>> +               tmp = list_entry(ptr, struct hwrng, list);
>> +               if (tmp->quality < rng->quality)
>> +                       break;
>> +       }
>> +       list_add_tail(&rng->list, ptr);
>> +
>>         old_rng = current_rng;
>>         err = 0;
>> -       if (!old_rng) {
>> +       if (!old_rng ||
>> +           (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
>> +               /*
>> +                * Set new rng as current if no current rng or rng was
>> +                * not chosen by user and the new one has better 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 +484,13 @@ 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 *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
>>
> This patch looks good. I am fine with this patch as is. Reviewed-by:
> PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
>
> If this patch is split into please go ahead and my reviewed-by tag.
>
> Regards,
> PrasannaKumar
>
Thanks for this feedback.
I will split into two and work in some improvements.
regards Harald Freudenberger

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2017-06-30 12:01 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-29 10:03 [PATCH 1/2] crypto: Make hwrng choose rng source by quality Harald Freudenberger
2017-06-30  5:27 ` PrasannaKumar Muralidharan
2017-06-30 12:01   ` Harald Freudenberger

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.