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

[Jeff Liu]

Hi Dave,

On 03/18/2013 07:53 AM, Dave Chinner wrote:
> 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.
Exactly.  It's not an overflow as the block_offset is 64-bit, but the
original evaluated value missing the high 32-bits of 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....
Yep. so we figure the 'from' out with the lower 12 bits in pos only, and
have block_offset with left higher bits in this way.
I will resent a patch according to your comments.
> 
> 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". :)
Thanks for your teaching!

-Jeff