Re: [patch] truncate fixes

Andrew Morton (akpm@zip.com.au)
Sun, 06 Jan 2002 20:28:37 -0800


Andrea Arcangeli wrote:
>
> > (I think I'll add a buffer_mapped() test to this code as well. It's
> > a bit redundant because the fs shouldn't go setting BH_New and not
> > BH_Mapped, but this code is _very_ rarely executed, and I haven't
> > tested all filesystems...)
>
> correct, it shouldn't be necessary. I wouldn't add it. if a fs breaks the
> buffer_new semantics it's the one that should be fixed methinks.

You mean "don't be lazy. Audit all the filesystems"? Sigh. OK.

> >
> > @@ -1633,12 +1660,22 @@ static int __block_prepare_write(struct
> > */
> > while(wait_bh > wait) {
> > wait_on_buffer(*--wait_bh);
> > - err = -EIO;
> > if (!buffer_uptodate(*wait_bh))
> > - goto out;
> > + return -EIO;
> > }
> > return 0;
> > out:
> > + bh = head;
> > + block_start = 0;
> > + do {
> > + if (buffer_new(bh) && buffer_mapped(bh) && !buffer_uptodate(bh)) {
> > + memset(kaddr+block_start, 0, bh->b_size);
> > + set_bit(BH_Uptodate, &bh->b_state);
> > + mark_buffer_dirty(bh);
> > + }
> > + block_start += bh->b_size;
> > + bh = bh->b_this_page;
> > + } while (bh != head);
>
> I found another problem, we really need to keep track of which bh are
> been created by us during the failing prepare_write (buffer_new right
> now, not a long time ago), or we risk to corrupt data with a write
> passing over many bh, where the first bh of the page contained vaild
> data since a long time ago. To do this: 1) we either keep track of it
> on the kernel stack with some local variable or 2) we change
> the buffer_new semantics so that they indicate an "instant buffer_new"
> to clear just after checking it

Fair enough. How does this (untested) approach look?

@@ -1600,6 +1627,7 @@ static int __block_prepare_write(struct
if (block_start >= to)
break;
if (!buffer_mapped(bh)) {
+ clear_bit(BH_New, &bh->b_state);
err = get_block(inode, block, bh, 1);
if (err)
goto out;
@@ -1633,12 +1661,30 @@ static int __block_prepare_write(struct
*/
while(wait_bh > wait) {
wait_on_buffer(*--wait_bh);
- err = -EIO;
if (!buffer_uptodate(*wait_bh))
- goto out;
+ return -EIO;
}
return 0;
out:
+ /*
+ * Zero out any newly allocated blocks to avoid exposing stale
+ * data. If BH_New is set, we know that the block was newly
+ * allocated in the above loop.
+ */
+ bh = head;
+ block_start = 0;
+ do {
+ if (buffer_new(bh)) {
+ if (buffer_uptodate(bh))
+ printk(KERN_ERR __FUNCTION__
+ ": zeroing uptodate buffer!\n");
+ memset(kaddr+block_start, 0, bh->b_size);
+ set_bit(BH_Uptodate, &bh->b_state);
+ mark_buffer_dirty(bh);
+ }
+ block_start += bh->b_size;
+ bh = bh->b_this_page;
+ } while (bh != head);
return err;
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/