xfs
[Top] [All Lists]

Re: [PATCH] xfs: remote attribute overwrite causes transaction overrun

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: remote attribute overwrite causes transaction overrun
From: Jeff Liu <jeff.liu@xxxxxxxxxx>
Date: Wed, 23 Apr 2014 10:13:24 +0800
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140422234628.GN18672@dastard>
References: <1398149949-11324-1-git-send-email-david@xxxxxxxxxxxxx> <535659F7.4060702@xxxxxxxxxx> <20140422234628.GN18672@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0
On 04/23 2014 07:46 AM, Dave Chinner wrote:
> On Tue, Apr 22, 2014 at 08:00:55PM +0800, Jeff Liu wrote:
>>
>> On 04/22 2014 14:59 PM, Dave Chinner wrote:
>>> From: Dave Chinner <dchinner@xxxxxxxxxx>
>>>
>>> Commit e461fcb ("xfs: remote attribute lookups require the value
>>> length") passes the remote attribute length in the xfs_da_args
>>> structure on lookup so that CRC calculations and validity checking
>>> can be performed correctly by related code. This, unfortunately has
>>> the side effect of changing the args->valuelen parameter in cases
>>> where it shouldn't.
>>>
>>> That is, when we replace a remote attribute, the incoming
>>> replacement stores the value and length in args->value and
>>> args->valuelen, but then the lookup which finds the existing remote
>>> attribute overwrites args->valuelen with the length of the remote
>>> attribute being replaced. Hence when we go to create the new
>>> attribute, we create it of the size of the existing remote
>>> attribute, not the size it is supposed to be. When the new attribute
>>> is much smaller than the old attribute, this results in a
>>> transaction overrun and an ASSERT() failure on a debug kernel:
>>>
>>> XFS: Assertion failed: tp->t_blk_res_used <= tp->t_blk_res, file: 
>>> fs/xfs/xfs_trans.c, line: 331
>>>
>>> Fix this by keeping the remote attribute value length separate to
>>> the attribute value length in the xfs_da_args structure. The enables
>>> us to pass the length of the remote attribute to be removed without
>>> overwriting the new attribute's length.
>>>
>>> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ....
>>> @@ -348,6 +348,11 @@ xfs_attr_rmtval_get(
>>>  
>>>     ASSERT(!(args->flags & ATTR_KERNOVAL));
>>>  
>>> +   /* remote value might be different size to the buffer supplied. */
>>> +   if (args->rmtvaluelen = args->valuelen)
>>                           ^^^
>> Here is a typo...
> 
> Oh, well spotted.
> 
> But, hold on a minute, shouldn't gcc be catching those sorts of
> typos?
> 
> /me groans
> 
> $ gcc t.c
> $ gcc -Wall t.c
> t.c: In function ¿main¿:
> t.c:10:2: warning: suggest parentheses around assignment used as truth value 
> [-Wparentheses]
>   if (foo = bar)
>   ^
> $
> 
> Only if -Wparentheses is specified.
> 
> Why the hell are we suppressing useful warnings on the kernel build?
> 

Well, maybe it's due to the different gcc release? I can hit this warning via
gcc-4.8.1 which is the default version on Ubuntu 13.10 as below:

# make SUBDIRS=fs/xfs -j4
...
fs/xfs/xfs_attr_remote.c: In function ‘xfs_attr_rmtval_get’:
fs/xfs/xfs_attr_remote.c:352:2: warning: suggest parentheses around assignment 
used as truth value [-Wparentheses]
  if (args->rmtvaluelen = args->valuelen)
  ^
  CC [M]  fs/xfs/xfs_bmap_btree.o
  CC [M]  fs/xfs/xfs_btree.o
...


Thanks,
-Jeff

<Prev in Thread] Current Thread [Next in Thread>