Re: bug: truncate to zero + setuid

[Roger Willcocks <roger@filmlight.ltd.uk>]

Tim Shimmin wrote:
> Hi Roger,
>
> Roger Willcocks wrote:
>> The nfsv3 setattr call permits a simultaneous truncate + setuid/gid 
>> operation. Normally XFS handles this fine, but if the file's being 
>> truncated to zero, and the file's already empty, XFS simply ignores 
>> the setuid/gid part, returning 'success'.
>>
>> The error's in xfs_vnodeops.c/xfs_setattr below the comment 'Short 
>> circuit the truncate case for zero length files', which bypasses all 
>> other changes.
>>
>> The simplest fix is to test whether this is the only change that's 
>> happening, otherwise you get tangled in transactions.
>>
>>        if (mask & XFS_AT_SIZE) {
>>                /* Short circuit the truncate case for zero length 
>> files */
>> -               if ((vap->va_size == 0) &&
>> +               if (((mask & ~XFS_AT_SIZE) == 0) && (vap->va_size == 
>> 0) &&
>>                   (ip->i_d.di_size == 0) && (ip->i_d.di_nextents == 
>> 0)) {
>>
>>
>> -- 
>> Roger
>>
>
> Yeah, I see your problem.
> It is interesting to know how much of multiple actions (different
> bits of mask) are supported.
> XFS_AT_UPDTIMES doesn't support anything else and will return quickly.
> As far as code which goes via error_return, it looks like the
> only non-error case is the short-circuit one mentioned above -
> truncating to zero an already zeroed file.
>
> I see your fix will get what we want although, it will mean that we
> will go thru the truncating path when we don't need to.
> It would be nice if we could act like the XFS_AT_SIZE was never set
> on the call in the case that other bits are set.
> Though when we first test the AT_SIZE, we don't have the inode locked.
> And I presume in that case we don't necessarily need to send a
> DMAPI truncate event?
> So I wonder if before the 1st AT_SIZE test that we have now,
> in the AT_SIZE case and with other bits set,
> we could lock the inode and test
> for the zero truncate of zero-len file and if so then remove the 
> AT_SIZE bit
> and then go on to the other testing as if AT_SIZE was never set.
> Hmmm, may be that just complicates things too much.
>
Yes I looked at simply unsetting the XFS_AT_SIZE bit (you'd also need to set
timeflags appropriately) but the problem is the bit's used to check when 
to create
a transaction - either up front as  XFS_TRANS_SETATTR_NOT_SIZE, or
later on, after the inode's been locked and unlocked again, as
XFS_TRANS_SETATTR_SIZE. So if you unset the bit (and there's still more
to do) you need to build a not_size transaction, at which point you 
might as well
rewrite the whole routine...

--
Roger