Messages in this thread Patch in this message | | | Date | Fri, 29 Aug 2008 12:48:07 -0700 | From | Andrew Morton <> | Subject | Re: 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); } _
| |