hw_random: core: Reset user selected rng by writing "" to rng_current

PATCH In review PrasannaKumar Muralidharan 3 participants 6 messages Oct 27, 2017 – Nov 3, 2017

Subsystems (2)

Crypto API Hardware random number generator core

lore

Share & tools

lore

PrasannaKumar Muralidharan

author

2017-10-27 17:05 UTC #0 lore raw

PATCH 1/1 hw_random: core: Reset user selected rng by writing "" to rng_current

User is able to select a chosen rng by writing its name to rng_current but there is no way to reset it without unbinding the rng. Let user write "" to rng_current and delesect the chosen rng.

Signed-off-by PrasannaKumar Muralidharan <[email protected]>

---
 drivers/char/hw_random/core.c | 51 +++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 19 deletions(-)

drivers/char/hw_random/core.c +32 ~~-19~~

index 9701ac7..be03024 100644
@@ -292,26 +292,48 @@ static struct miscdevice rng_miscdev = {
 	.groups		= rng_dev_groups,
 };
 
+static int enable_best_rng(void)
+{
+	int ret = -ENODEV;
+
+	BUG_ON(!mutex_is_locked(&rng_mutex));
+
+	/* rng_list is sorted by quality, use the best (=first) one */
+	if (!list_empty(&rng_list)) {
+		struct hwrng *new_rng;
+
+		new_rng = list_entry(rng_list.next, struct hwrng, list);
+		ret = ((new_rng == current_rng) ? 0 : set_current_rng(new_rng));
+		if (!ret)
+			cur_rng_set_by_user = 0;
+	}
+
+	return ret;
+}
+
 static ssize_t hwrng_attr_current_store(struct device *dev,
 					struct device_attribute *attr,
 					const char *buf, size_t len)
 {
-	int err;
+	int err = -ENODEV;
 	struct hwrng *rng;
 
 	err = mutex_lock_interruptible(&rng_mutex);
 	if (err)
 		return -ERESTARTSYS;
-	err = -ENODEV;
-	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)
+
+	if (sysfs_streq(buf, "")) {
+		err = enable_best_rng();
+	} else {
+		list_for_each_entry(rng, &rng_list, list) {
+			if (sysfs_streq(rng->name, buf)) {
+				cur_rng_set_by_user = 1;
 				err = set_current_rng(rng);
-			break;
+				break;
+			}
 		}
 	}
+
 	mutex_unlock(&rng_mutex);
 
 	return err ? : len;

@@ -493,17 +515,8 @@ void hwrng_unregister(struct hwrng *rng)
 	mutex_lock(&rng_mutex);
 
 	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;
-
-			new_rng = list_entry(rng_list.next, struct hwrng, list);
-			set_current_rng(new_rng);
-		}
-	}
+	if (current_rng == rng)
+		enable_best_rng();
 
 	if (list_empty(&rng_list)) {
 		mutex_unlock(&rng_mutex);

Signature · 1 line

2.10.0

Harald Freudenberger

2017-10-30 08:10 UTC #1 lore raw

Quoted 80 lines · PrasannaKumar Muralidharan

User is able to select a chosen rng by writing its name to rng_current but there is no way to reset it without unbinding the rng. Let user write "" to rng_current and delesect the chosen rng.

Signed-off-by PrasannaKumar Muralidharan <[email protected]>

---
 drivers/char/hw_random/core.c | 51 +++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 19 deletions(-)

drivers/char/hw_random/core.c +30 ~~-14~~

index 9701ac7..be03024 100644
@@ -292,26 +292,48 @@ static struct miscdevice rng_miscdev = {
 	.groups		= rng_dev_groups,
 };
 
+static int enable_best_rng(void)
+{
+	int ret = -ENODEV;
+
+	BUG_ON(!mutex_is_locked(&rng_mutex));
+
+	/* rng_list is sorted by quality, use the best (=first) one */
+	if (!list_empty(&rng_list)) {
+		struct hwrng *new_rng;
+
+		new_rng = list_entry(rng_list.next, struct hwrng, list);
+		ret = ((new_rng == current_rng) ? 0 : set_current_rng(new_rng));
+		if (!ret)
+			cur_rng_set_by_user = 0;
+	}
+
+	return ret;
+}
+
 static ssize_t hwrng_attr_current_store(struct device *dev,
 					struct device_attribute *attr,
 					const char *buf, size_t len)
 {
-	int err;
+	int err = -ENODEV;
 	struct hwrng *rng;
 
 	err = mutex_lock_interruptible(&rng_mutex);
 	if (err)
 		return -ERESTARTSYS;
-	err = -ENODEV;
-	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)
+
+	if (sysfs_streq(buf, "")) {
+		err = enable_best_rng();
+	} else {
+		list_for_each_entry(rng, &rng_list, list) {
+			if (sysfs_streq(rng->name, buf)) {
+				cur_rng_set_by_user = 1;
 				err = set_current_rng(rng);
-			break;
+				break;
+			}
 		}
 	}
+
 	mutex_unlock(&rng_mutex);
 
 	return err ? : len;

@@ -493,17 +515,8 @@ void hwrng_unregister(struct hwrng *rng)
 	mutex_lock(&rng_mutex);
 
 	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;

drivers/char/hw_random/core.c void hwrng_unregister(struct hwrng *rng) · line 522

Quoted 10 lines

drivers/char/hw_random/core.c +2 -5

-
-			new_rng = list_entry(rng_list.next, struct hwrng, list);
-			set_current_rng(new_rng);
-		}
-	}
+	if (current_rng == rng)
+		enable_best_rng();
 
 	if (list_empty(&rng_list)) {
 		mutex_unlock(&rng_mutex);

That's a really good idea. I also thought about something like that. regards
Harald Freudenberger

PrasannaKumar Muralidharan

author

2017-10-30 12:59 UTC #2 lore raw

Hi Harald,

Quoted 1 line · Harald Freudenberger

That's a really good idea. I also thought about something like that.

Do you mind reviewing the code?

Thanks,
PrasannaKumar

Harald Freudenberger

Reviewed-by on v1 1/1

2017-10-30 13:28 UTC #3 lore raw

In reply to PrasannaKumar Muralidharan

Quoted 32 lines · PrasannaKumar Muralidharan

User is able to select a chosen rng by writing its name to rng_current but there is no way to reset it without unbinding the rng. Let user write "" to rng_current and delesect the chosen rng.

Signed-off-by PrasannaKumar Muralidharan <[email protected]>

---
 drivers/char/hw_random/core.c | 51 +++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 19 deletions(-)

drivers/char/hw_random/core.c +15 -0

index 9701ac7..be03024 100644
@@ -292,26 +292,48 @@ static struct miscdevice rng_miscdev = {
 	.groups		= rng_dev_groups,
 };
 
+static int enable_best_rng(void)
+{
+	int ret = -ENODEV;
+
+	BUG_ON(!mutex_is_locked(&rng_mutex));
+
+	/* rng_list is sorted by quality, use the best (=first) one */
+	if (!list_empty(&rng_list)) {
+		struct hwrng *new_rng;
+
+		new_rng = list_entry(rng_list.next, struct hwrng, list);
+		ret = ((new_rng == current_rng) ? 0 : set_current_rng(new_rng));
+		if (!ret)
+			cur_rng_set_by_user = 0;
+	}

drivers/char/hw_random/core.c static struct miscdevice rng_miscdev = { · line 318

Quoted 10 lines

drivers/char/hw_random/core.c +5 -1

+
+	return ret;
+}
+
 static ssize_t hwrng_attr_current_store(struct device *dev,
 					struct device_attribute *attr,
 					const char *buf, size_t len)
 {
-	int err;
+	int err = -ENODEV;

Two lines later err is assigned the return value of mutex_lock_interruptible(). So the -ENODEV value assignment does nothing.

Quoted 38 lines

drivers/char/hw_random/core.c +10 ~~-13~~

 	struct hwrng *rng;
 
 	err = mutex_lock_interruptible(&rng_mutex);
 	if (err)
 		return -ERESTARTSYS;
-	err = -ENODEV;
-	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)
+
+	if (sysfs_streq(buf, "")) {
+		err = enable_best_rng();
+	} else {
+		list_for_each_entry(rng, &rng_list, list) {
+			if (sysfs_streq(rng->name, buf)) {
+				cur_rng_set_by_user = 1;
 				err = set_current_rng(rng);
-			break;
+				break;
+			}
 		}
 	}
+
 	mutex_unlock(&rng_mutex);
 
 	return err ? : len;

@@ -493,17 +515,8 @@ void hwrng_unregister(struct hwrng *rng)
 	mutex_lock(&rng_mutex);
 
 	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;

drivers/char/hw_random/core.c void hwrng_unregister(struct hwrng *rng) · line 522

Quoted 10 lines

drivers/char/hw_random/core.c +2 -5

-
-			new_rng = list_entry(rng_list.next, struct hwrng, list);
-			set_current_rng(new_rng);
-		}
-	}
+	if (current_rng == rng)
+		enable_best_rng();
 
 	if (list_empty(&rng_list)) {
 		mutex_unlock(&rng_mutex);

looks like for me.

Reviewed-by Harald Freudenberger <[email protected]>

PrasannaKumar Muralidharan

author

2017-10-30 13:42 UTC #4 lore raw

Hi Harald,

Quoted 37 lines · Harald Freudenberger

Quoted 36 lines · PrasannaKumar Muralidharan

User is able to select a chosen rng by writing its name to rng_current but there is no way to reset it without unbinding the rng. Let user write "" to rng_current and delesect the chosen rng.

Signed-off-by PrasannaKumar Muralidharan <[email protected]>

---
 drivers/char/hw_random/core.c | 51 +++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 19 deletions(-)

drivers/char/hw_random/core.c +19 -0

index 9701ac7..be03024 100644
@@ -292,26 +292,48 @@ static struct miscdevice rng_miscdev = {
      .groups         = rng_dev_groups,
 };
 
+static int enable_best_rng(void)
+{
+     int ret = -ENODEV;
+
+     BUG_ON(!mutex_is_locked(&rng_mutex));
+
+     /* rng_list is sorted by quality, use the best (=first) one */
+     if (!list_empty(&rng_list)) {
+             struct hwrng *new_rng;
+
+             new_rng = list_entry(rng_list.next, struct hwrng, list);
+             ret = ((new_rng == current_rng) ? 0 : set_current_rng(new_rng));
+             if (!ret)
+                     cur_rng_set_by_user = 0;
+     }
+
+     return ret;
+}
+

Quoted 8 lines

drivers/char/hw_random/core.c static struct miscdevice rng_miscdev = { · line 318
Quoted 6 lines
drivers/char/hw_random/core.c +1 -1
 static ssize_t hwrng_attr_current_store(struct device *dev,
                                      struct device_attribute *attr,
                                      const char *buf, size_t len)
 {
-     int err;
+     int err = -ENODEV;
Two lines later err is assigned the return value of mutex_lock_interruptible(). So the -ENODEV value assignment does nothing.

Indeed true. Will change this when I make a v2.

I am even thinking if it makes sense to return error when user writes "" to rng_current and there is no rng device registered. I think returning 0 should be fine.

Quoted 46 lines

drivers/char/hw_random/core.c +12 ~~-18~~

      struct hwrng *rng;
 
      err = mutex_lock_interruptible(&rng_mutex);
      if (err)
              return -ERESTARTSYS;
-     err = -ENODEV;
-     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)
+
+     if (sysfs_streq(buf, "")) {
+             err = enable_best_rng();
+     } else {
+             list_for_each_entry(rng, &rng_list, list) {
+                     if (sysfs_streq(rng->name, buf)) {
+                             cur_rng_set_by_user = 1;
                              err = set_current_rng(rng);
-                     break;
+                             break;
+                     }
              }
      }
+
      mutex_unlock(&rng_mutex);
 
      return err ? : len;

@@ -493,17 +515,8 @@ void hwrng_unregister(struct hwrng *rng)
      mutex_lock(&rng_mutex);
 
      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;
-
-                     new_rng = list_entry(rng_list.next, struct hwrng, list);
-                     set_current_rng(new_rng);
-             }
-     }
+     if (current_rng == rng)
+             enable_best_rng();

Quoted 5 lines

drivers/char/hw_random/core.c void hwrng_unregister(struct hwrng *rng) · line 522
Quoted 2 lines
drivers/char/hw_random/core.c +0 -0
      if (list_empty(&rng_list)) {
              mutex_unlock(&rng_mutex);
looks like for me.
Reviewed-by Harald Freudenberger <[email protected]>

Thank you.

Thanks,
PrasannaKumar

Herbert Xu

maintainer

2017-11-03 14:28 UTC #5 lore raw

In reply to PrasannaKumar Muralidharan

Quoted 5 lines · PrasannaKumar Muralidharan

User is able to select a chosen rng by writing its name to rng_current but there is no way to reset it without unbinding the rng. Let user write "" to rng_current and delesect the chosen rng.
Signed-off-by PrasannaKumar Muralidharan <[email protected]>

Patch applied. Thanks.

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt