Messages in this thread | | | Date | Sat, 20 Mar 1999 21:14:56 GMT | From | "Stephen C. Tweedie" <> | Subject | Re: [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/
| |