Re: [PATCH] xfs: fix ASSERTION failure in xfs_vm_write_failed()

[Dave Chinner <david@fromorbit.com>]

On Sun, Mar 17, 2013 at 11:01:08PM +0800, Jeff Liu wrote:
> Hello,
> 
> Yesterday, Michael reported that ran xfstest-078 got kernel panic when growing a
> small partition to a huge size(64-bit) on 32-bit system, this issue can be easily
> reproduced on the latest upstream 38 to 39-rc2 if CONFIG_XFS_DEBUG is enabled.
> According to my investigation, if the request pos offset exceeding 32-bit integer
> range, evaluate block_offset with pos & PAGE_MASK will cause overflows so that
> it most likely that the assert is not true.

Hi Jeff,

Nice work finding the root of the problem, but I think your fix
is wrong. Here's how I look at the problem - all the parameters are
loff_t, so everything should validate cleanly as 64 bit values.

typedef __kernel_loff_t         loff_t
typedef long long       __kernel_loff_t;

So there shouldn't be any overflows occurring that need masking like
this:

> This patch fix it by checking "block_offset + from == (pos & ~0UL)" instead.

So, what's the real problem?

#define PAGE_SIZE       (_AC(1,UL) << PAGE_SHIFT)
#define PAGE_MASK       (~(PAGE_SIZE-1))

On a 32 bit system, PAGE_MASK = 0xfffff000, as an unsigned long.

And so this:

loff_t                  block_offset = pos & PAGE_MASK;

is also masking off the high 32 bits in pos.

That's why:

> +	/*
> +	 * Evaluate block_offset via (pos & PAGE_MASK) on 32-bit system
> +	 * can cause overflow if the request pos is 64-bit.  Hence we
> +	 * have to verify the write offset with (pos & ~0UL) to avoid it.
> +	 */
> +	ASSERT(block_offset + from == (pos & ~0UL));

Masking off the high bits of pos here makes the ASSERT failure go
away.  However, it doesn't fix the problem - it just shuts up the
warning that there's a problem.

The bug is that block_offset is passed into
xfs_vm_kill_delalloc_range(), and from above we now know that
block_offset doesn't have the correct value. This is a
potential data corruption bug, and catching this problem was the
reason the ASSERT() was placed in the code. i.e. ensuring we are
punching at the correct block offset into the file.

IOWs, the intention of the code is that block_offset should be a 64
bit value with the lower 12 bits masked out. Something like this:

	block_offset = (pos >> PAGE_CACHE_SHIFT) << PAGE_CACHE_SHIFT;

Will clear the lower 12 bits, and then the ASSERT() should evaluate
correctly and the correct offset get passed to
xfs_vm_kill_delalloc_range(), fixing the bug....

i.e. whenever an ASSERT() fires, you need to look at the code for
bugs - more often than not the ASSERT() is correct and the fact that
it fired is indicative of a bug in the code. Hence changing the
ASSERT to stop it firing is almost always the wrong "fix". :)

Cheers,

Dave.


is telling us we have a bug in the code, not that the ASSERT needs
fixing.

i.e.

So, there's no overflow here - PAGE_MASK just doesn't work on 64 bit
offsets.

>  
>  	head = page_buffers(page);
>  	block_start = 0;
> -- 
> 1.7.9.5
> 
> -- 
> This message has been scanned for viruses and
> dangerous content by MailScanner, and is
> believed to be clean.
> 
> 

-- 
Dave Chinner
david@fromorbit.com

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