On 03/18/2013 07:53 AM, Dave Chinner wrote:
> On Sun, Mar 17, 2013 at 11:01:08PM +0800, Jeff Liu wrote:
>> 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
>> reproduced on the latest upstream 38 to 39-rc2 if CONFIG_XFS_DEBUG is
>> According to my investigation, if the request pos offset exceeding 32-bit
>> range, evaluate block_offset with pos & PAGE_MASK will cause overflows so
>> 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 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!