Re: [PATCH] xfs: fix delalloc quota accounting on failure

[Eric Sandeen <sandeen@sandeen.net>]

On 5/8/12 9:28 AM, Eric Sandeen wrote:
> On 5/8/12 5:48 AM, Dave Chinner wrote:
>> From: Dave Chinner <dchinner@redhat.com>
>>
>> xfstest 270 was causing quota reservations way beyond what was sane
>> (ten to hundreds of TB) for a 4GB filesystem. There's a sign problem
>> in the error handling path of xfs_bmapi_reserve_delalloc() because
>> xfs_trans_unreserve_quota_nblks() simple negates the value passed -
>> which doesn't work for an unsigned variable. This causes
>> reservations of close to 2^32 block instead of removing a
>> reservation of a handful of blocks.
>>
>> Fix the same problem in the other xfs_trans_unreserve_quota_nblks()
>> callers where unsigned integer variables are used, too.
>>
>> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> 
> Ouch!
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> as far as it goes, but a couple thoughts:
> 
> 1) Should the cast be done in the macro so new callers don't get tripped up?
> 2) Should we just remove the ninos argument from the macro?  It's always passed as 0 (and could potentially suffer the same problem)
> 
> something like:
> 
> diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
> index b50ec5b..f771838 100644
> --- a/fs/xfs/xfs_quota.h
> +++ b/fs/xfs/xfs_quota.h
> @@ -370,8 +370,8 @@ static inline int xfs_trans_reserve_quota_bydquots(struct xfs_trans *tp,
>  #define xfs_qm_unmount_quotas(mp)
>  #endif /* CONFIG_XFS_QUOTA */
>  
> -#define xfs_trans_unreserve_quota_nblks(tp, ip, nblks, ninos, flags) \
> -	xfs_trans_reserve_quota_nblks(tp, ip, -(nblks), -(ninos), flags)
> +#define xfs_trans_unreserve_quota_nblks(tp, ip, nblks, flags) \
> +	xfs_trans_reserve_quota_nblks(tp, ip, -((long)nblks), 0, flags)
>  #define xfs_trans_reserve_quota(tp, mp, ud, gd, nb, ni, f) \
>  	xfs_trans_reserve_quota_bydquots(tp, mp, ud, gd, nb, ni, \
>  				f | XFS_QMOPT_RES_REGBLKS)
> 

There are also 2 other callers that "already" fixed this, sortakinda:

xfs_bunmapi()

                        /* Update realtime/data freespace, unreserve quota */
...
                                (void)xfs_trans_reserve_quota_nblks(NULL,
                                        ip, -((long)del.br_blockcount), 0,
                                        XFS_QMOPT_RES_REGBLKS);

                                (void)xfs_trans_reserve_quota_nblks(NULL,
                                        ip, -((long)del.br_blockcount), 0,
                                        XFS_QMOPT_RES_REGBLKS);

those could be an unreserve call instead, with the sign fix embedded in the macro.

And while we're at it it seems nobody calls xfs_trans_reserve_quota_nblks() with ninos != 0 so I think that arg could be removed from that function too, not just the macro, in another patch.

-Eric



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