On 5/8/12 9:28 AM, Eric Sandeen wrote:
> On 5/8/12 5:48 AM, Dave Chinner wrote:
>> From: Dave Chinner <dchinner@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>
> Ouch!
>
> Reviewed-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> 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@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
