Re: bug: truncate to zero + setuid

[Timothy Shimmin <tes@sgi.com>]

Hi Roger,

Roger Willcocks wrote:
> Timothy Shimmin wrote:
>> I presume it was done where it was done so that the inode was locked 
>> and we
>> were under the XFS_AT_SIZE predicate.
>>
>> I was just thinking of something like...
>> but I'm probably missing something.
>>
>> Index: 2.6.x-xfs/fs/xfs/xfs_vnodeops.c
>> ===================================================================
>> --- 2.6.x-xfs.orig/fs/xfs/xfs_vnodeops.c        2007-10-12 
>> 16:06:15.000000000 +1000
>> +++ 2.6.x-xfs/fs/xfs/xfs_vnodeops.c     2007-10-30 14:59:46.418837757 
>> +1100
>> @@ -304,6 +304,24 @@
>>         }
>>
>>         /*
>> +        * Short circuit the truncate case for zero length files.
>> +        * If more mask bits are set, then just remove the SIZE one
>> +        * and keep going.
>> +        */
>> +       if (mask & XFS_AT_SIZE) {
>> +               xfs_ilock(ip, XFS_ILOCK_SHARED);
>> +               if ((vap->va_size == 0) && (ip->i_size == 0) && 
>> (ip->i_d.di_nextents == 0)) {
>> +                       if (mask & ~XFS_AT_SIZE) {
>> +                               mask &= ~XFS_AT_SIZE;
>> +                       } else {
>> +                               xfs_iunlock(ip, XFS_ILOCK_SHARED);
>> +                               return 0;
>> +                       }
>> +               }
>> +               xfs_iunlock(ip, XFS_ILOCK_SHARED);
>> +       }
>> +
>> +       /*
>>          * For the other attributes, we acquire the inode lock and
>>          * first do an error checking pass.
>>          */
>> @@ -451,17 +469,6 @@
>>          * Truncate file.  Must have write permission and not be a 
>> directory.
>>          */
>>         if (mask & XFS_AT_SIZE) {
>> -               /* Short circuit the truncate case for zero length 
>> files */
>> -               if ((vap->va_size == 0) &&
>> -                  (ip->i_size == 0) && (ip->i_d.di_nextents == 0)) {
>> -                       xfs_iunlock(ip, XFS_ILOCK_EXCL);
>> -                       lock_flags &= ~XFS_ILOCK_EXCL;
>> -                       if (mask & XFS_AT_CTIME)
>> -                               xfs_ichgtime(ip, XFS_ICHGTIME_MOD | 
>> XFS_ICHGTIME_CHG);
>> -                       code = 0;
>> -                       goto error_return;
>> -               }
>> -
>>                 if (VN_ISDIR(vp)) {
>>                         code = XFS_ERROR(EISDIR);
>>                         goto error_return;
 >
> This misses setting the access and changed times which still need to be 
> touched even if the file's already zero bytes. How about this:
> (noting that open.c/do_truncate uses XFS_AT_SIZE | XFS_AT_CTIME)
> 

Well, if XFS_AT_CTIME was set then my patch wouldn't
return straight away and would continue processing the mask fields.
And I was presuming that the times would be set doing this.

However, it doesn't look quite so simple and consistent:

* going down the normal (non-short-circuit) path for AT_SIZE,
   it doesn't test for CTIME but rather just does:

       /*
        * Have to do this even if the file's size doesn't change.
        */
       timeflags |= XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;

   And yet our short-circuit case only does it if AT_CTIME is set.
   Doesn't look consistent to me.

* So what in the vfs would call it...

   -----------------------
   open(O_TRUNC)...

   int may_open(struct nameidata *nd, int acc_mode, int flag)
   {

         if (flag & O_TRUNC) {
   ...
                         error = do_truncate(dentry, 0, ATTR_MTIME|ATTR_CTIME, NULL);

   ------------------------

   static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
   {
         if (!error)
                 error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, file);

   ------------------------

   This is NOR the case for
   * do_sys_truncate() and
   * do_coredump(),
   however, which send in zero for those bits.
   Not to mention other ways to get to these calls.

   Anyway, open(O_TRUNC) will be a good candidate for the optimization
   by the looks of it as it always trunc's to zero.

   So in the 2 truncate cases, it is setting MTIME and CTIME.

   And how is MTIME handled in the xfs code...

         /*
          * Change file access or modified times.
          */
         if (mask & (XFS_AT_ATIME|XFS_AT_MTIME)) {
                 if (mask & XFS_AT_ATIME) {
                         ip->i_d.di_atime.t_sec = vap->va_atime.tv_sec;
                         ip->i_d.di_atime.t_nsec = vap->va_atime.tv_nsec;
                         ip->i_update_core = 1;
                         timeflags &= ~XFS_ICHGTIME_ACC;
                 }
                 if (mask & XFS_AT_MTIME) {
                         ip->i_d.di_mtime.t_sec = vap->va_mtime.tv_sec;
                         ip->i_d.di_mtime.t_nsec = vap->va_mtime.tv_nsec;
                         timeflags &= ~XFS_ICHGTIME_MOD;
                         timeflags |= XFS_ICHGTIME_CHG;
                 }
                 if (tp && (flags & ATTR_UTIME))
                         xfs_trans_log_inode (tp, ip, XFS_ILOG_CORE);
         }

         /*
          * Send out timestamp changes that need to be set to the
          * current time.  Not done when called by a DMI function.
          */
         if (timeflags && !(flags & ATTR_DMI))
                 xfs_ichgtime(ip, timeflags);

   And note that MTIME, will actually turn on the timeflags for XFS_ICHGTIME_CHG,
   which is the time associated with XFS_AT_CTIME.

   So for the 2 do_truncate calls paths,
   it will set the mtime based
   on the va_mtime and set the ctime based on current time (nanotime()).
   Our shortcut is setting both to current time.
   I don't know if anyone really cares.


I don't like all these inconsistencies.
One way to reduce inconsistencies is to allow code to go thru
common paths so we can do the same strange thing in the one spot ;-)

It looks like in the AT_SIZE, we should always set those timeflags
irrespective of AT_CTIME.

BTW, your locking looks wrong - it appears you don't unlock when the
file is non-zero size.

--Tim


> --- xfs_vnodeops.c      2007-09-04 15:57:40.000000000 +0100
> +++ /tmp/xfs_vnodeops.c 2007-10-30 17:11:32.000000000 +0000
> @@ -378,6 +378,24 @@
>                        return (code);
>        }
> 
> +
> +       if ((mask & XFS_AT_SIZE) && (vap->va_size == 0)) {
> +
> +               /* Short circuit the truncate case for zero length files */
> +
> +               xfs_ilock(ip, XFS_ILOCK_EXCL);
> +               if ((ip->i_d.di_size == 0) && (ip->i_d.di_nextents == 0)) {
> +                       xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +                       if (mask & XFS_AT_CTIME)
> +                               xfs_ichgtime(ip, 
> XFS_ICHGTIME_MOD|XFS_ICHGTIME_CHG);
> +                       mask &= ~(XFS_AT_SIZE|XFS_AT_CTIME);
> +                       if (mask == 0) {
> +                               code = 0;
> +                               goto error_return;
> +                       }
> +               }
> +       }
> +
>        /*
>         * For the other attributes, we acquire the inode lock and
>         * first do an error checking pass.
> @@ -528,17 +546,6 @@
>         * Truncate file.  Must have write permission and not be a 
> directory.
>         */
>        if (mask & XFS_AT_SIZE) {
> -               /* Short circuit the truncate case for zero length files */
> -               if ((vap->va_size == 0) &&
> -                  (ip->i_d.di_size == 0) && (ip->i_d.di_nextents == 0)) {
> -                       xfs_iunlock(ip, XFS_ILOCK_EXCL);
> -                       lock_flags &= ~XFS_ILOCK_EXCL;
> -                       if (mask & XFS_AT_CTIME)
> -                               xfs_ichgtime(ip, XFS_ICHGTIME_MOD | 
> XFS_ICHGTIME_CHG);
> -                       code = 0;
> -                       goto error_return;
> -               }
> -
>                if (vp->v_type == VDIR) {
>                        code = XFS_ERROR(EISDIR);
>                        goto error_return;