Possible memory corruption in xfs_buf_iomove()

Possible memory corruption in xfs_buf_iomove()

[Dale Stephenson]

At Snap Appliance, we were running into appeared to be random oops  
when setting/reading large attributes.  Trevor Heathorn was able to  
trace the problem down to the function xfs_buf_iomove() in linux-2.6- 
xfs/xfs_buf.c.  The size of the copy within the loop is calculated as  
a minimum of the page size and bp->b_count_desired, minus the  
applicable offsets.  This can result in a copy size greater than the  
remaining size of the data length, which in the case of a read caused  
whatever else is in the buffer to be written past the end of the data  
area we had allocated.  He found that this fix prevented the oops:

--- xfs_buf.c	17 Sep 2009 00:19:49 -0000	1.4
+++ xfs_buf.c	31 Jan 2011 20:22:59 -0000	1.5
@@ -1313,6 +1313,7 @@
  			      PAGE_CACHE_SIZE-cpoff, bp->b_count_desired-boff);

  		ASSERT(((csize + cpoff) <= PAGE_CACHE_SIZE));
+		csize = min(csize, (bend - boff));

  		switch (mode) {
  		case XBRW_ZERO:

This fix also keeps the write from copying past the end of the buffer,  
though at least in that case it won't be written to unallocated  
memory.  It also may keep the zero from zeroing to the end of the  
buffer if the bsize passed in doesn't justify it, though I'm not sure  
if there are any callers of this function that do not want it zeroed  
to the end.

Dale J. Stephenson
dstephenson@overlandstorage.com
dalestephenson@mac.com 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: Possible memory corruption in xfs_buf_iomove()

[Dave Chinner]

On Mon, Jan 31, 2011 at 04:29:16PM -0500, Dale Stephenson wrote:
> At Snap Appliance, we were running into appeared to be random oops
> when setting/reading large attributes.  Trevor Heathorn was able to
> trace the problem down to the function xfs_buf_iomove() in
> linux-2.6-xfs/xfs_buf.c.  The size of the copy within the loop is
> calculated as a minimum of the page size and bp->b_count_desired,
> minus the applicable offsets.  This can result in a copy size
> greater than the remaining size of the data length, which in the
> case of a read caused whatever else is in the buffer to be written
> past the end of the data area we had allocated.  He found that this
> fix prevented the oops:

Good catch! Do you have a test case that reproduced the problem? If
so, can you post that, too, so we can push it into xfstests as a
regression test?

[ As an aside, if you've got other regression tests that you'd like
to have run regularly during mainline developement and can be
integrated into xfstests, we can help make that happen. ]

> --- xfs_buf.c	17 Sep 2009 00:19:49 -0000	1.4
> +++ xfs_buf.c	31 Jan 2011 20:22:59 -0000	1.5
> @@ -1313,6 +1313,7 @@
>  			      PAGE_CACHE_SIZE-cpoff, bp->b_count_desired-boff);
> 
>  		ASSERT(((csize + cpoff) <= PAGE_CACHE_SIZE));
> +		csize = min(csize, (bend - boff));
> 
>  		switch (mode) {
>  		case XBRW_ZERO:

Yup, definitely needed. Can you add a Signed-off-by tag to this with
an appropriate commit message (pretty much what you've already
included with this report), we can get this fix into mainline
quickly.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs