lkml.org 
[lkml]   [2008]   [Aug]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: drivers/char/random.c line 728 BUG
On Thu, 28 Aug 2008 15:59:24 -0700
Aaron Straus <aaron@merfinllc.com> wrote:

> Hi,
>
> On Aug 26 03:59 PM, Aaron Straus wrote:
> > kernel BUG at drivers/char/random.c:728!
>
> OK so that's (outside spinlock):
>
> BUG_ON(r->entropy_count > r->poolinfo->POOLBITS);
>
> in credit_entropy_bits we do (inside spinlock):
>
> r->entropy_count += nbits;
> if (r->entropy_count < 0) {
> DEBUG_ENT("negative entropy/overflow\n");
> r->entropy_count = 0;
> } else if (r->entropy_count > r->poolinfo->POOLBITS)
> r->entropy_count = r->poolinfo->POOLBITS;
>
> I wonder if we got unlucky and did the:
>
> r->entropy_count += nbits
>
> - overflowed the entropy_count THEN
> - another thread hits the BUG before this thread reaches
>
> r->entropy_count = r->poolinfo->POOLBITS;
>
> --
>
> I notice before this commit:
>
> commit adc782dae6c4c0f6fb679a48a544cfbcd79ae3dc
> Author: Matt Mackall <mpm@selenic.com>
> Date: Tue Apr 29 01:03:07 2008 -0700
>
> random: simplify and rename credit_entropy_store
>
> The credit_entropy_store function looks like this:
>
> spin_lock_irqsave(&r->lock, flags);
>
> if (r->entropy_count + nbits < 0) {
> DEBUG_ENT("negative entropy/overflow (%d+%d)\n",
> r->entropy_count, nbits);
> r->entropy_count = 0;
> } else if (r->entropy_count + nbits > r->poolinfo->POOLBITS) {
> r->entropy_count = r->poolinfo->POOLBITS;
> } else {
> r->entropy_count += nbits;
> if (nbits)
> DEBUG_ENT("added %d entropy credits to %s\n",
> nbits, r->name);
> }
>
>
> Notice the old version is careful not to overflow r->entropy_count at
> any point (even within the spinlock). So perhaps that's why we didn't
> hit this BUG() in the past?
>

yep, I would agree with all that.

How's this look?


From: Andrew Morton <akpm@linux-foundation.org>

Fix a bug reported by and diagnosed by Aaron Straus.

This is a regression intruduced into 2.6.26 by

commit adc782dae6c4c0f6fb679a48a544cfbcd79ae3dc
Author: Matt Mackall <mpm@selenic.com>
Date: Tue Apr 29 01:03:07 2008 -0700

random: simplify and rename credit_entropy_store

credit_entropy_bits() does:

spin_lock_irqsave(&r->lock, flags);
...
if (r->entropy_count > r->poolinfo->POOLBITS)
r->entropy_count = r->poolinfo->POOLBITS;

so there is a time window in which this BUG_ON():

static size_t account(struct entropy_store *r, size_t nbytes, int min,
int reserved)
{
unsigned long flags;

BUG_ON(r->entropy_count > r->poolinfo->POOLBITS);

/* Hold lock while accounting */
spin_lock_irqsave(&r->lock, flags);

can trigger.

We could fix this by moving the assertion inside the lock, but it seems
safer and saner to revert to the old behaviour wherein
entropy_store.entropy_count at no time exceeds
entropy_store.poolinfo->POOLBITS.

Reported-by: Aaron Straus <aaron@merfinllc.com>
Cc: Matt Mackall <mpm@selenic.com>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: <stable@kernel.org> [2.6.26.x]
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

drivers/char/random.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)

diff -puN drivers/char/random.c~drivers-char-randomc-fix-a-race-which-can-lead-to-a-bogus-bug drivers/char/random.c
--- a/drivers/char/random.c~drivers-char-randomc-fix-a-race-which-can-lead-to-a-bogus-bug
+++ a/drivers/char/random.c
@@ -520,6 +520,7 @@ static void mix_pool_bytes(struct entrop
static void credit_entropy_bits(struct entropy_store *r, int nbits)
{
unsigned long flags;
+ int entropy_count;

if (!nbits)
return;
@@ -527,20 +528,20 @@ static void credit_entropy_bits(struct e
spin_lock_irqsave(&r->lock, flags);

DEBUG_ENT("added %d entropy credits to %s\n", nbits, r->name);
- r->entropy_count += nbits;
- if (r->entropy_count < 0) {
+ entropy_count = r->entropy_count;
+ entropy_count += nbits;
+ if (entropy_count < 0) {
DEBUG_ENT("negative entropy/overflow\n");
- r->entropy_count = 0;
- } else if (r->entropy_count > r->poolinfo->POOLBITS)
- r->entropy_count = r->poolinfo->POOLBITS;
+ entropy_count = 0;
+ } else if (entropy_count > r->poolinfo->POOLBITS)
+ entropy_count = r->poolinfo->POOLBITS;

/* should we wake readers? */
- if (r == &input_pool &&
- r->entropy_count >= random_read_wakeup_thresh) {
+ if (r == &input_pool && entropy_count >= random_read_wakeup_thresh) {
wake_up_interruptible(&random_read_wait);
kill_fasync(&fasync, SIGIO, POLL_IN);
}
-
+ r->entropy_count = entropy_count;
spin_unlock_irqrestore(&r->lock, flags);
}

_


\
 
 \ /
  Last update: 2008-08-29 21:51    [W:0.092 / U:5.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site