Re: [patch] fix race in __block_prepare_write (again)

[Anton Altaparmakov <aia21@cam.ac.uk>]

And one more thing...

On Thu, 2005-04-21 at 08:01 +0100, Anton Altaparmakov wrote:
> On Thu, 21 Apr 2005, Nick Piggin wrote:
> > ... I somehow didn't send it to Andrew last time.
> > 
> > Fix a race where __block_prepare_write can leak out an in-flight
> > read against a bh if get_block returns an error. This can lead to
> > the page becoming unlocked while the buffer is locked and the read
> > still in flight. __mpage_writepage BUGs on this condition.
> [snip]
> > --- linux-2.6.orig/fs/buffer.c	2005-04-21 11:55:17.549614278 
> +1000
> > +++ linux-2.6/fs/buffer.c	2005-04-21 15:55:41.483826075 +1000
> > @@ -1988,6 +1988,7 @@
> >  			*wait_bh++=bh;
> >  		}
> >  	}
> > +out:
> >  	/*
> >  	 * If we issued read requests - let them complete.
> >  	 */
> > @@ -1996,8 +1997,9 @@
> >  		if (!buffer_uptodate(*wait_bh))
> >  			return -EIO;

This return is now wrong after your patch.  It should be "err = -EIO;"
otherwise you do not zero newly allocated blocks and thus risk exposing
stale data on buffer i/o errors. 

> >  	}
> > -	return 0;
> > -out:
> > +	if (!err)
> > +		return err;
> > +
> >  	/*
> >  	 * Zero out any newly allocated blocks to avoid exposing stale
> >  	 * data.  If BH_New is set, we know that the block was newly
> 
> Any reason why you left the goto out?  It would be IMO much cleaner to 
> remove the label "out" altogether and replace the single "goto out" with a 
> "break" (which is fine since the goto happens inside the for loop 
> immediately after which you place the label.)

Best regards,

        Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/