lkml.org 
[lkml]   [1999]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch] fix for buffer hash leakage
Hi,

On Sat, 20 Mar 1999 12:53:36 -0800 (PST), Linus Torvalds
<torvalds@transmeta.com> said:

> On Sat, 20 Mar 1999, Linus Torvalds wrote:
>>
>> Stephen, would you mind thinking about this for five minutes? I wonder if
>> it shouldn't look something like this:

> Having thought about it for five minutes I convinced myself that the patch
> as Andrea had it was potentially horribly deadly, and could result in
> total disk corruption. Bad.

It depends entirely on how it gets called, and as you point out, the
original version was just as likely to do the Wrong Thing anyway (making
the buffer B_NODEV as we used to, instead of B_FREE, would mean it would
take a little longer to reuse it, but it could still be reused before IO
completion). I did have a check before giving the patch the OK to make
sure that it was being called safely everywhere, and ext2/truncate.c
does in fact absolutely guarantee that the buffer has been waited on and
count==1 before calling bforget.

The only other place I can see bforget being used right now is in Ingo's
new raid1/5 check_consistency code, which looks safe, and in the ufs
code, which admittedly looks decidedly dangerous.

However, as it stands, the routine is definitely a loaded gun just
waiting for an opportunity to do serious damage.

It depends on whether you want to make the entry conditions for bforget
explicit, or just do the safe thing and add a few redundant checks which
are guaranteed to be purely cosmetic in the current kernel. And yes,
things are easier in the future if we just add the test.

> /*
> * bforget() is like brelse(), except it puts the buffer on the
> * free list if it can.. We can NOT free the buffer if:
> * - there are other users of it
> * - it is locked and thus can have active IO
> */
> void __bforget(struct buffer_head * buf)
> {
> if (buf->b_count != 1 || buffer_locked(buf)) {
> __brelse(buf);
> return;
> }
> remove_from_queues(buf);
> put_last_free(buf);
> }

> Can anybody see any downsides with this patch?

Looks fine. This is essentially precisely what Chuck has been testing
already, it seems, but with the extra check in place.

--Stephen

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 13:50    [W:0.386 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site