* [PATCH 1/9] random: fix typos / spelling errors in comments
2013-11-13 8:07 [PATCH v2 0/9] random: code cleanups Greg Price
@ 2013-11-13 8:07 ` Greg Price
2013-11-29 19:56 ` Theodore Ts'o
2013-11-13 8:07 ` [PATCH 2/9] random: fix comment on proc_do_uuid Greg Price
` (7 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Greg Price @ 2013-11-13 8:07 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: linux-kernel
Cc: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Greg Price <price@mit.edu>
---
drivers/char/random.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index cdf4cfb..ed72be5 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -308,7 +308,7 @@ static int random_read_wakeup_thresh = 64;
static int random_write_wakeup_thresh = 28 * OUTPUT_POOL_WORDS;
/*
- * The minimum number of seconds between urandom pool resending. We
+ * The minimum number of seconds between urandom pool reseeding. We
* do this to limit the amount of entropy that can be drained from the
* input pool even if there are heavy demands on /dev/urandom.
*/
@@ -325,7 +325,7 @@ static int random_min_urandom_seed = 60;
* Register. (See M. Matsumoto & Y. Kurita, 1992. Twisted GFSR
* generators. ACM Transactions on Modeling and Computer Simulation
* 2(3):179-194. Also see M. Matsumoto & Y. Kurita, 1994. Twisted
- * GFSR generators II. ACM Transactions on Mdeling and Computer
+ * GFSR generators II. ACM Transactions on Modeling and Computer
* Simulation 4:254-266)
*
* Thanks to Colin Plumb for suggesting this.
@@ -1040,7 +1040,7 @@ static void extract_buf(struct entropy_store *r, __u8 *out)
sha_transform(hash.w, (__u8 *)(r->pool + i), workspace);
/*
- * If we have a architectural hardware random number
+ * If we have an architectural hardware random number
* generator, mix that in, too.
*/
for (i = 0; i < LONGS(20); i++) {
--
1.8.3.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 2/9] random: fix comment on proc_do_uuid
2013-11-13 8:07 [PATCH v2 0/9] random: code cleanups Greg Price
2013-11-13 8:07 ` [PATCH 1/9] random: fix typos / spelling errors in comments Greg Price
@ 2013-11-13 8:07 ` Greg Price
2013-11-29 19:59 ` Theodore Ts'o
2013-11-13 8:07 ` [PATCH 3/9] random: fix description of get_random_bytes Greg Price
` (6 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Greg Price @ 2013-11-13 8:07 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: linux-kernel
There's only one function here now, as uuid_strategy is long gone.
Also make the bit about "If accesses via ..." clearer.
Cc: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Greg Price <price@mit.edu>
---
drivers/char/random.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index ed72be5..e11ee4e 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1516,13 +1516,13 @@ static int max_write_thresh = INPUT_POOL_WORDS * 32;
static char sysctl_bootid[16];
/*
- * These functions is used to return both the bootid UUID, and random
+ * This function is used to return both the bootid UUID, and random
* UUID. The difference is in whether table->data is NULL; if it is,
* then a new UUID is generated and returned to the user.
*
- * If the user accesses this via the proc interface, it will be returned
- * as an ASCII string in the standard UUID format. If accesses via the
- * sysctl system call, it is returned as 16 bytes of binary data.
+ * If the user accesses this via the proc interface, the UUID will be
+ * returned as an ASCII string in the standard UUID format; if via the
+ * sysctl system call, as 16 bytes of binary data.
*/
static int proc_do_uuid(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos)
--
1.8.3.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 3/9] random: fix description of get_random_bytes
2013-11-13 8:07 [PATCH v2 0/9] random: code cleanups Greg Price
2013-11-13 8:07 ` [PATCH 1/9] random: fix typos / spelling errors in comments Greg Price
2013-11-13 8:07 ` [PATCH 2/9] random: fix comment on proc_do_uuid Greg Price
@ 2013-11-13 8:07 ` Greg Price
2013-11-29 20:00 ` Theodore Ts'o
2013-11-13 8:07 ` [PATCH 4/9] random: simplify loop in random_read Greg Price
` (5 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Greg Price @ 2013-11-13 8:07 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: linux-kernel
After this remark was written, commit d2e7c96af added a use of
arch_get_random_long() inside the get_random_bytes codepath.
The main point stands, but it needs to be reworded.
Cc: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Greg Price <price@mit.edu>
---
drivers/char/random.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index e11ee4e..6d3627d 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1174,8 +1174,9 @@ static ssize_t extract_entropy_user(struct entropy_store *r, void __user *buf,
/*
* This function is the exported kernel interface. It returns some
* number of good random numbers, suitable for key generation, seeding
- * TCP sequence numbers, etc. It does not use the hw random number
- * generator, if available; use get_random_bytes_arch() for that.
+ * TCP sequence numbers, etc. It does not rely on the hardware random
+ * number generator. For random bytes direct from the hardware RNG
+ * (when available), use get_random_bytes_arch().
*/
void get_random_bytes(void *buf, int nbytes)
{
--
1.8.3.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 4/9] random: simplify loop in random_read
2013-11-13 8:07 [PATCH v2 0/9] random: code cleanups Greg Price
` (2 preceding siblings ...)
2013-11-13 8:07 ` [PATCH 3/9] random: fix description of get_random_bytes Greg Price
@ 2013-11-13 8:07 ` Greg Price
2013-11-29 20:09 ` Theodore Ts'o
2013-11-13 8:08 ` [PATCH 5/9] random: fix comment on "account" Greg Price
` (4 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Greg Price @ 2013-11-13 8:07 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: linux-kernel
The loop condition never changes until just before a break, so we
might as well write it as a constant. Also since v2.6.33-rc7~40^2~2
("random: drop weird m_time/a_time manipulation") we don't do
anything after the loop finishes, so the 'break's might as well
return directly. Some other simplifications.
The behavior should be identical except that I've changed a debug
message.
Cc: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Greg Price <price@mit.edu>
---
drivers/char/random.c | 57 ++++++++++++++++-----------------------------------
1 file changed, 18 insertions(+), 39 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 6d3627d..cf1357b 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1289,53 +1289,32 @@ void rand_initialize_disk(struct gendisk *disk)
static ssize_t
random_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
{
- ssize_t n, retval = 0, count = 0;
+ ssize_t n;
if (nbytes == 0)
return 0;
- while (nbytes > 0) {
- n = nbytes;
- if (n > SEC_XFER_SIZE)
- n = SEC_XFER_SIZE;
-
- n = extract_entropy_user(&blocking_pool, buf, n);
-
- if (n < 0) {
- retval = n;
- break;
- }
-
+ nbytes = min_t(size_t, nbytes, SEC_XFER_SIZE);
+ while (1) {
+ n = extract_entropy_user(&blocking_pool, buf, nbytes);
+ if (n < 0)
+ return n;
trace_random_read(n*8, (nbytes-n)*8,
ENTROPY_BITS(&blocking_pool),
ENTROPY_BITS(&input_pool));
-
- if (n == 0) {
- if (file->f_flags & O_NONBLOCK) {
- retval = -EAGAIN;
- break;
- }
-
- wait_event_interruptible(random_read_wait,
- ENTROPY_BITS(&input_pool) >=
- random_read_wakeup_thresh);
-
- if (signal_pending(current)) {
- retval = -ERESTARTSYS;
- break;
- }
-
- continue;
- }
-
- count += n;
- buf += n;
- nbytes -= n;
- break; /* This break makes the device work */
- /* like a named pipe */
+ if (n > 0)
+ return n;
+ /* Pool is (near) empty. Maybe wait and retry. */
+
+ if (file->f_flags & O_NONBLOCK)
+ return -EAGAIN;
+
+ wait_event_interruptible(random_read_wait,
+ ENTROPY_BITS(&input_pool) >=
+ random_read_wakeup_thresh);
+ if (signal_pending(current))
+ return -ERESTARTSYS;
}
-
- return (count ? count : retval);
}
static ssize_t
--
1.8.3.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 4/9] random: simplify loop in random_read
2013-11-13 8:07 ` [PATCH 4/9] random: simplify loop in random_read Greg Price
@ 2013-11-29 20:09 ` Theodore Ts'o
2013-11-30 5:30 ` Greg Price
0 siblings, 1 reply; 22+ messages in thread
From: Theodore Ts'o @ 2013-11-29 20:09 UTC (permalink / raw)
To: Greg Price; +Cc: linux-kernel
On Wed, Nov 13, 2013 at 03:07:40AM -0500, Greg Price wrote:
> The loop condition never changes until just before a break, so we
> might as well write it as a constant. Also since v2.6.33-rc7~40^2~2
> ("random: drop weird m_time/a_time manipulation") we don't do
> anything after the loop finishes, so the 'break's might as well
> return directly. Some other simplifications.
>
> The behavior should be identical except that I've changed a debug
> message.
>
> Cc: "Theodore Ts'o" <tytso@mit.edu>
> Signed-off-by: Greg Price <price@mit.edu>
Applied, although I slightly edited the commit description the the
debug message mentioned in the description was removed since the first
version of your patch series, and it wasn't updated as part of the
rebase.
- Ted
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 5/9] random: fix comment on "account"
2013-11-13 8:07 [PATCH v2 0/9] random: code cleanups Greg Price
` (3 preceding siblings ...)
2013-11-13 8:07 ` [PATCH 4/9] random: simplify loop in random_read Greg Price
@ 2013-11-13 8:08 ` Greg Price
2013-11-29 20:53 ` Theodore Ts'o
2013-11-13 8:08 ` [PATCH 6/9] random: simplify accounting logic Greg Price
` (3 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Greg Price @ 2013-11-13 8:08 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: linux-kernel
This comment didn't quite keep up as extract_entropy() was split into
four functions. Put each bit by the function it describes.
Cc: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Greg Price <price@mit.edu>
---
drivers/char/random.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index cf1357b..fe7ecdd 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -962,17 +962,9 @@ static void push_to_pool(struct work_struct *work)
}
/*
- * These functions extracts randomness from the "entropy pool", and
- * returns it in a buffer.
- *
- * The min parameter specifies the minimum amount we can pull before
- * failing to avoid races that defeat catastrophic reseeding while the
- * reserved parameter indicates how much entropy we must leave in the
- * pool after each pull to avoid starving other readers.
- *
- * Note: extract_entropy() assumes that .poolwords is a multiple of 16 words.
+ * This function decides how many bytes to actually take from the
+ * given pool, and also debits the entropy count accordingly.
*/
-
static size_t account(struct entropy_store *r, size_t nbytes, int min,
int reserved)
{
@@ -1022,6 +1014,12 @@ retry:
return ibytes;
}
+/*
+ * This function does the actual extraction for extract_entropy and
+ * extract_entropy_user.
+ *
+ * Note: we assume that .poolwords is a multiple of 16 words.
+ */
static void extract_buf(struct entropy_store *r, __u8 *out)
{
int i;
@@ -1083,6 +1081,15 @@ static void extract_buf(struct entropy_store *r, __u8 *out)
memset(&hash, 0, sizeof(hash));
}
+/*
+ * This function extracts randomness from the "entropy pool", and
+ * returns it in a buffer.
+ *
+ * The min parameter specifies the minimum amount we can pull before
+ * failing to avoid races that defeat catastrophic reseeding while the
+ * reserved parameter indicates how much entropy we must leave in the
+ * pool after each pull to avoid starving other readers.
+ */
static ssize_t extract_entropy(struct entropy_store *r, void *buf,
size_t nbytes, int min, int reserved)
{
@@ -1133,6 +1140,10 @@ static ssize_t extract_entropy(struct entropy_store *r, void *buf,
return ret;
}
+/*
+ * This function extracts randomness from the "entropy pool", and
+ * returns it in a userspace buffer.
+ */
static ssize_t extract_entropy_user(struct entropy_store *r, void __user *buf,
size_t nbytes)
{
--
1.8.3.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 6/9] random: simplify accounting logic
2013-11-13 8:07 [PATCH v2 0/9] random: code cleanups Greg Price
` (4 preceding siblings ...)
2013-11-13 8:08 ` [PATCH 5/9] random: fix comment on "account" Greg Price
@ 2013-11-13 8:08 ` Greg Price
2013-11-30 1:04 ` Theodore Ts'o
2013-11-13 8:08 ` [PATCH 7/9] random: forget lock in lockless accounting Greg Price
` (2 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Greg Price @ 2013-11-13 8:08 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: linux-kernel, Jiri Kosina, H. Peter Anvin
This logic is exactly equivalent to the old logic, but it should
be easier to see what it's doing.
The equivalence depends on one fact from outside this function:
when 'r->limit' is false, 'reserved' is zero. (Well, two facts;
the other is that 'reserved' is never negative.)
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Signed-off-by: Greg Price <price@mit.edu>
---
drivers/char/random.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index fe7ecdd..5dffca8 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -988,14 +988,10 @@ retry:
ibytes = 0;
} else {
/* If limited, never pull more than available */
- if (r->limit && ibytes + reserved >= have_bytes)
- ibytes = have_bytes - reserved;
-
- if (have_bytes >= ibytes + reserved)
- entropy_count -= ibytes << (ENTROPY_SHIFT + 3);
- else
- entropy_count = reserved << (ENTROPY_SHIFT + 3);
-
+ if (r->limit)
+ ibytes = min_t(size_t, ibytes, have_bytes - reserved);
+ entropy_count = max_t(int, 0,
+ entropy_count - (ibytes << (ENTROPY_SHIFT + 3)));
if (cmpxchg(&r->entropy_count, orig, entropy_count) != orig)
goto retry;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 6/9] random: simplify accounting logic
2013-11-13 8:08 ` [PATCH 6/9] random: simplify accounting logic Greg Price
@ 2013-11-30 1:04 ` Theodore Ts'o
0 siblings, 0 replies; 22+ messages in thread
From: Theodore Ts'o @ 2013-11-30 1:04 UTC (permalink / raw)
To: Greg Price; +Cc: linux-kernel, Jiri Kosina, H. Peter Anvin
On Wed, Nov 13, 2013 at 03:08:21AM -0500, Greg Price wrote:
> This logic is exactly equivalent to the old logic, but it should
> be easier to see what it's doing.
>
> The equivalence depends on one fact from outside this function:
> when 'r->limit' is false, 'reserved' is zero. (Well, two facts;
> the other is that 'reserved' is never negative.)
Applied, thanks.
- Ted
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 7/9] random: forget lock in lockless accounting
2013-11-13 8:07 [PATCH v2 0/9] random: code cleanups Greg Price
` (5 preceding siblings ...)
2013-11-13 8:08 ` [PATCH 6/9] random: simplify accounting logic Greg Price
@ 2013-11-13 8:08 ` Greg Price
2013-11-30 1:10 ` Theodore Ts'o
2013-11-13 8:08 ` [PATCH 8/9] random: tighten bound on random_read_wakeup_thresh Greg Price
2013-11-13 8:08 ` [PATCH 9/9] random: simplify accounting code Greg Price
8 siblings, 1 reply; 22+ messages in thread
From: Greg Price @ 2013-11-13 8:08 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: linux-kernel, Jiri Kosina
The only mutable data accessed here is ->entropy_count, but since
902c098a3 ("random: use lockless techniques in the interrupt path")
that member isn't protected by the lock anyway. Since 10b3a32d2
("random: fix accounting race condition") we use cmpxchg to protect
our accesses to ->entropy_count here. Drop the use of the lock.
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Greg Price <price@mit.edu>
---
drivers/char/random.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 5dffca8..1f43f9e 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -974,9 +974,6 @@ static size_t account(struct entropy_store *r, size_t nbytes, int min,
int entropy_count, orig;
size_t ibytes;
- /* Hold lock while accounting */
- spin_lock_irqsave(&r->lock, flags);
-
BUG_ON(r->entropy_count > r->poolinfo->poolfracbits);
/* Can we pull enough? */
@@ -999,7 +996,6 @@ retry:
< random_write_wakeup_thresh)
wakeup_write = 1;
}
- spin_unlock_irqrestore(&r->lock, flags);
trace_debit_entropy(r->name, 8 * ibytes);
if (wakeup_write) {
--
1.8.3.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 7/9] random: forget lock in lockless accounting
2013-11-13 8:08 ` [PATCH 7/9] random: forget lock in lockless accounting Greg Price
@ 2013-11-30 1:10 ` Theodore Ts'o
0 siblings, 0 replies; 22+ messages in thread
From: Theodore Ts'o @ 2013-11-30 1:10 UTC (permalink / raw)
To: Greg Price; +Cc: linux-kernel, Jiri Kosina
On Wed, Nov 13, 2013 at 03:08:28AM -0500, Greg Price wrote:
> The only mutable data accessed here is ->entropy_count, but since
> 902c098a3 ("random: use lockless techniques in the interrupt path")
> that member isn't protected by the lock anyway. Since 10b3a32d2
> ("random: fix accounting race condition") we use cmpxchg to protect
> our accesses to ->entropy_count here. Drop the use of the lock.
Applied, with a slight change to clarify the commit description:
The only mutable data accessed here is ->entropy_count, but since
10b3a32d2 ("random: fix accounting race condition") we use cmpxchg to
protect our accesses to ->entropy_count here. Drop the use of the
lock.
Thanks!
- Ted
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 8/9] random: tighten bound on random_read_wakeup_thresh
2013-11-13 8:07 [PATCH v2 0/9] random: code cleanups Greg Price
` (6 preceding siblings ...)
2013-11-13 8:08 ` [PATCH 7/9] random: forget lock in lockless accounting Greg Price
@ 2013-11-13 8:08 ` Greg Price
2013-12-06 0:31 ` Theodore Ts'o
2013-11-13 8:08 ` [PATCH 9/9] random: simplify accounting code Greg Price
8 siblings, 1 reply; 22+ messages in thread
From: Greg Price @ 2013-11-13 8:08 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: linux-kernel
We use this value in a few places other than its literal meaning,
in particular in _xfer_secondary_pool() as a minimum number of
bits to pull from the input pool at a time into either output
pool. It doesn't make sense to pull more bits than the whole size
of an output pool.
We could and possibly should separate the quantities "how much
should the input pool have to have to wake up /dev/random readers"
and "how much should we transfer from the input to an output pool
at a time", but nobody is likely to be sad they can't set the first
quantity to more than 1024 bits, so for now just limit them both.
Cc: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Greg Price <price@mit.edu>
---
drivers/char/random.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 1f43f9e..865c8db 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1494,7 +1494,7 @@ EXPORT_SYMBOL(generate_random_uuid);
#include <linux/sysctl.h>
static int min_read_thresh = 8, min_write_thresh;
-static int max_read_thresh = INPUT_POOL_WORDS * 32;
+static int max_read_thresh = OUTPUT_POOL_WORDS * 32;
static int max_write_thresh = INPUT_POOL_WORDS * 32;
static char sysctl_bootid[16];
--
1.8.3.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 8/9] random: tighten bound on random_read_wakeup_thresh
2013-11-13 8:08 ` [PATCH 8/9] random: tighten bound on random_read_wakeup_thresh Greg Price
@ 2013-12-06 0:31 ` Theodore Ts'o
0 siblings, 0 replies; 22+ messages in thread
From: Theodore Ts'o @ 2013-12-06 0:31 UTC (permalink / raw)
To: Greg Price; +Cc: linux-kernel
On Wed, Nov 13, 2013 at 03:08:35AM -0500, Greg Price wrote:
> We use this value in a few places other than its literal meaning,
> in particular in _xfer_secondary_pool() as a minimum number of
> bits to pull from the input pool at a time into either output
> pool. It doesn't make sense to pull more bits than the whole size
> of an output pool.
>
> We could and possibly should separate the quantities "how much
> should the input pool have to have to wake up /dev/random readers"
> and "how much should we transfer from the input to an output pool
> at a time", but nobody is likely to be sad they can't set the first
> quantity to more than 1024 bits, so for now just limit them both.
>
> Cc: "Theodore Ts'o" <tytso@mit.edu>
> Signed-off-by: Greg Price <price@mit.edu>
Thanks, applied.
- Ted
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 9/9] random: simplify accounting code
2013-11-13 8:07 [PATCH v2 0/9] random: code cleanups Greg Price
` (7 preceding siblings ...)
2013-11-13 8:08 ` [PATCH 8/9] random: tighten bound on random_read_wakeup_thresh Greg Price
@ 2013-11-13 8:08 ` Greg Price
2013-12-06 1:13 ` Theodore Ts'o
8 siblings, 1 reply; 22+ messages in thread
From: Greg Price @ 2013-11-13 8:08 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: linux-kernel, Jiri Kosina, H. Peter Anvin
With this we handle "reserved" in just one place. As a bonus the
code becomes less nested, and the "wakeup_write" flag variable
becomes unnecessary. The variable "flags" was already unused.
This code behaves identically to the previous version except in
two pathological cases that don't occur. If the argument "nbytes"
is already less than "min", then we didn't previously enforce
"min". If r->limit is false while "reserved" is nonzero, then we
previously applied "reserved" in checking whether we had enough
bits, even though we don't apply it to actually limit how many we
take. The callers of account() never exercise either of these cases.
Before the previous commit, it was possible for "nbytes" to be less
than "min" if userspace chose a pathological configuration, but no
longer.
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Signed-off-by: Greg Price <price@mit.edu>
---
drivers/char/random.c | 27 ++++++++++-----------------
1 file changed, 10 insertions(+), 17 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 865c8db..87b41b0 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -968,8 +968,6 @@ static void push_to_pool(struct work_struct *work)
static size_t account(struct entropy_store *r, size_t nbytes, int min,
int reserved)
{
- unsigned long flags;
- int wakeup_write = 0;
int have_bytes;
int entropy_count, orig;
size_t ibytes;
@@ -981,24 +979,19 @@ retry:
entropy_count = orig = ACCESS_ONCE(r->entropy_count);
have_bytes = entropy_count >> (ENTROPY_SHIFT + 3);
ibytes = nbytes;
- if (have_bytes < min + reserved) {
+ /* If limited, never pull more than available */
+ if (r->limit)
+ ibytes = min_t(size_t, ibytes, have_bytes - reserved);
+ if (ibytes < min)
ibytes = 0;
- } else {
- /* If limited, never pull more than available */
- if (r->limit)
- ibytes = min_t(size_t, ibytes, have_bytes - reserved);
- entropy_count = max_t(int, 0,
- entropy_count - (ibytes << (ENTROPY_SHIFT + 3)));
- if (cmpxchg(&r->entropy_count, orig, entropy_count) != orig)
- goto retry;
-
- if ((r->entropy_count >> ENTROPY_SHIFT)
- < random_write_wakeup_thresh)
- wakeup_write = 1;
- }
+ entropy_count = max_t(int, 0,
+ entropy_count - (ibytes << (ENTROPY_SHIFT + 3)));
+ if (ibytes && cmpxchg(&r->entropy_count, orig, entropy_count) != orig)
+ goto retry;
trace_debit_entropy(r->name, 8 * ibytes);
- if (wakeup_write) {
+ if (ibytes &&
+ (r->entropy_count >> ENTROPY_SHIFT) < random_write_wakeup_thresh) {
wake_up_interruptible(&random_write_wait);
kill_fasync(&fasync, SIGIO, POLL_OUT);
}
--
1.8.3.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 9/9] random: simplify accounting code
2013-11-13 8:08 ` [PATCH 9/9] random: simplify accounting code Greg Price
@ 2013-12-06 1:13 ` Theodore Ts'o
2013-12-06 18:25 ` Greg Price
0 siblings, 1 reply; 22+ messages in thread
From: Theodore Ts'o @ 2013-12-06 1:13 UTC (permalink / raw)
To: Greg Price; +Cc: linux-kernel, Jiri Kosina, H. Peter Anvin
On Wed, Nov 13, 2013 at 03:08:41AM -0500, Greg Price wrote:
> + entropy_count = max_t(int, 0,
> + entropy_count - (ibytes << (ENTROPY_SHIFT + 3)));
> + if (ibytes && cmpxchg(&r->entropy_count, orig, entropy_count) != orig)
> + goto retry;
I wonder if we would be better dropping the test for ibytes here, so
the above condition reads:
if (cmpxchg(&r->entropy_count, orig, entropy_count) != orig)
goto retry;
It further simplifies the code, and it means that if we it turns out
that ibytes is zero (meaning there was no entropy available) but some
additional entropy comes in, we might acutally end up retrying and
successfully grabbing that entropy for the caller.
- Ted
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 9/9] random: simplify accounting code
2013-12-06 1:13 ` Theodore Ts'o
@ 2013-12-06 18:25 ` Greg Price
2013-12-07 14:48 ` Theodore Ts'o
0 siblings, 1 reply; 22+ messages in thread
From: Greg Price @ 2013-12-06 18:25 UTC (permalink / raw)
To: Theodore Ts'o, linux-kernel, Jiri Kosina, H. Peter Anvin
On Thu, Dec 05, 2013 at 08:13:47PM -0500, Theodore Ts'o wrote:
> On Wed, Nov 13, 2013 at 03:08:41AM -0500, Greg Price wrote:
> > + if (ibytes && cmpxchg(&r->entropy_count, orig, entropy_count) != orig)
> > + goto retry;
>
> I wonder if we would be better dropping the test for ibytes here, so
> the above condition reads:
>
> if (cmpxchg(&r->entropy_count, orig, entropy_count) != orig)
> goto retry;
>
> It further simplifies the code, and it means that if we it turns out
> that ibytes is zero (meaning there was no entropy available) but some
> additional entropy comes in, we might acutally end up retrying and
> successfully grabbing that entropy for the caller.
Sure, that'd be reasonable. I didn't do that mainly because I wanted
to preserve existing behavior wherever possible in these cleanups, to
make them easy to read and review.
Greg
^ permalink raw reply [flat|nested] 22+ messages in thread