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

[Jeff Liu <jeff.liu@oracle.com>]

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@redhat.com>
>>>
>>> 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@redhat.com>
> ....
>>> @@ -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

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