Re: bug: truncate to zero + setuid

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

[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]

Timothy Shimmin wrote:
> Hi Roger,
>
...
> I don't like all these inconsistencies.

Take a look at the attached patch relative to the current cvs (it's a bit 
big to put
inline). The basic problem is it's currently unclear when to set the times 
from
va_atime etc. and when to set them to the current time. So I've used the 
already
defined XFS_AT_UPDxTIME flags to indicate that a time should be set to 'now'
and XFS_AT_xTIME to mean set it using va_xtime. This seems to fit well with
the current code and I wonder if that's how it was meant to work in the 
first
place. I've also removed the now redundant ATTR_UTIME flag and pulled
the null truncate to the top, which simplifies things.

One query: in both xfs_iops.c/xfs_vn_setattr and 
xfs_dm.c/xfs_dm_set_fileattr the
ATIME branch sets the inode's atime directly. This is probably something to 
do with
the comment above xfs_iops.c/xfs_ichgtime ('to make sure the access time 
update
will take') but it could probably be handled better.

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

Oops...

--
Roger


[-- Attachment #2: xfs_setattr.patch --]
[-- Type: application/octet-stream, Size: 5478 bytes --]

diff -ur xfs.orig/linux-2.6/xfs_iops.c xfs/linux-2.6/xfs_iops.c
--- xfs.orig/linux-2.6/xfs_iops.c	2007-11-04 10:59:00.923480296 +0000
+++ xfs/linux-2.6/xfs_iops.c	2007-11-04 12:43:15.702609336 +0000
@@ -655,13 +655,21 @@
 		vattr.va_size = attr->ia_size;
 	}
 	if (ia_valid & ATTR_ATIME) {
-		vattr.va_mask |= XFS_AT_ATIME;
-		vattr.va_atime = attr->ia_atime;
-		inode->i_atime = attr->ia_atime;
+		if (ia_valid & ATTR_ATIME_SET) {
+			vattr.va_mask |= XFS_AT_ATIME;
+			vattr.va_atime = attr->ia_atime;
+			inode->i_atime = attr->ia_atime;
+		} else {
+			vattr.va_mask |= XFS_AT_UPDATIME;
+		}
 	}
 	if (ia_valid & ATTR_MTIME) {
-		vattr.va_mask |= XFS_AT_MTIME;
-		vattr.va_mtime = attr->ia_mtime;
+		if (ia_valid & ATTR_MTIME_SET) {
+			vattr.va_mask |= XFS_AT_MTIME;
+			vattr.va_mtime = attr->ia_mtime;
+		} else {
+			vattr.va_mask |= XFS_AT_UPDMTIME;
+		}
 	}
 	if (ia_valid & ATTR_CTIME) {
 		vattr.va_mask |= XFS_AT_CTIME;
@@ -674,8 +682,6 @@
 			inode->i_mode &= ~S_ISGID;
 	}
 
-	if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET))
-		flags |= ATTR_UTIME;
 #ifdef ATTR_NO_BLOCK
 	if ((ia_valid & ATTR_NO_BLOCK))
 		flags |= ATTR_NONBLOCK;
diff -ur xfs.orig/linux-2.6/xfs_vnode.h xfs/linux-2.6/xfs_vnode.h
--- xfs.orig/linux-2.6/xfs_vnode.h	2007-11-04 10:59:00.923480296 +0000
+++ xfs/linux-2.6/xfs_vnode.h	2007-11-04 11:01:33.338309720 +0000
@@ -270,7 +270,6 @@
 /*
  * Flags to vop_setattr/getattr.
  */
-#define	ATTR_UTIME	0x01	/* non-default utime(2) request */
 #define	ATTR_DMI	0x08	/* invocation from a DMI function */
 #define	ATTR_LAZY	0x80	/* set/get attributes lazily */
 #define	ATTR_NONBLOCK	0x100	/* return EAGAIN if operation would block */
diff -ur xfs.orig/xfs_vnodeops.c xfs/xfs_vnodeops.c
--- xfs.orig/xfs_vnodeops.c	2007-11-04 10:59:00.917481208 +0000
+++ xfs/xfs_vnodeops.c	2007-11-04 12:07:44.917537904 +0000
@@ -214,10 +214,10 @@
 {
 	bhv_vnode_t		*vp = XFS_ITOV(ip);
 	xfs_mount_t		*mp = ip->i_mount;
-	xfs_trans_t		*tp;
+	xfs_trans_t		*tp = NULL;
 	int			mask;
 	int			code;
-	uint			lock_flags;
+	uint			lock_flags=0;
 	uint			commit_flags=0;
 	uid_t			uid=0, iuid=0;
 	gid_t			gid=0, igid=0;
@@ -244,21 +244,51 @@
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return XFS_ERROR(EIO);
 
+	olddquot1 = olddquot2 = NULL;
+	udqp = gdqp = NULL;
+
+	/*
+	 * Truncate is special because it changes the file as well as
+	 * the attributes.
+	 */
+	if (mask & XFS_AT_SIZE) {
+		/* Must have write permission and not be a directory. */
+		if (VN_ISDIR(vp)) {
+			code = XFS_ERROR(EISDIR);
+			goto error_return;
+		} else if (!VN_ISREG(vp)) {
+			code = XFS_ERROR(EINVAL);
+			goto error_return;
+		}
+		/*
+		 * Short circuit the truncate case for zero length files.
+		 */
+		if (vap->va_size == 0) {
+			xfs_ilock(ip, XFS_ILOCK_EXCL);
+			if ((ip->i_size == 0) && (ip->i_d.di_nextents == 0)) {
+				mask |= XFS_AT_UPDCTIME|XFS_AT_UPDMTIME;
+				mask &= ~XFS_AT_SIZE;
+			}
+			xfs_iunlock(ip, XFS_ILOCK_EXCL);
+		}
+	}
+
 	/*
 	 * Timestamps do not need to be logged and hence do not
 	 * need to be done within a transaction.
 	 */
 	if (mask & XFS_AT_UPDTIMES) {
-		ASSERT((mask & ~XFS_AT_UPDTIMES) == 0);
 		timeflags = ((mask & XFS_AT_UPDATIME) ? XFS_ICHGTIME_ACC : 0) |
 			    ((mask & XFS_AT_UPDCTIME) ? XFS_ICHGTIME_CHG : 0) |
 			    ((mask & XFS_AT_UPDMTIME) ? XFS_ICHGTIME_MOD : 0);
-		xfs_ichgtime(ip, timeflags);
-		return 0;
+		mask &= ~XFS_AT_UPDTIMES;
 	}
 
-	olddquot1 = olddquot2 = NULL;
-	udqp = gdqp = NULL;
+	if (mask == 0) {
+		if (timeflags && !(flags & ATTR_DMI))
+			xfs_ichgtime(ip, timeflags);
+		return 0;
+	}
 
 	/*
 	 * If disk quotas is on, we make sure that the dquots do exist on disk,
@@ -307,12 +337,11 @@
 	 * For the other attributes, we acquire the inode lock and
 	 * first do an error checking pass.
 	 */
-	tp = NULL;
 	lock_flags = XFS_ILOCK_EXCL;
 	if (flags & ATTR_NOLOCK)
 		need_iolock = 0;
 	if (!(mask & XFS_AT_SIZE)) {
-		if ((mask != (XFS_AT_CTIME|XFS_AT_ATIME|XFS_AT_MTIME)) ||
+		if ((mask & ~(XFS_AT_CTIME|XFS_AT_ATIME|XFS_AT_MTIME)) != 0 ||
 		    (mp->m_flags & XFS_MOUNT_WSYNC)) {
 			tp = xfs_trans_alloc(mp, XFS_TRANS_SETATTR_NOT_SIZE);
 			commit_flags = 0;
@@ -451,24 +480,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;
-		} else if (!VN_ISREG(vp)) {
-			code = XFS_ERROR(EINVAL);
-			goto error_return;
-		}
 		/*
 		 * Make sure that the dquots are attached to the inode.
 		 */
@@ -481,8 +492,7 @@
 	 */
 	if (mask & (XFS_AT_ATIME|XFS_AT_MTIME)) {
 		if (!file_owner) {
-			if ((flags & ATTR_UTIME) &&
-			    !capable(CAP_FOWNER)) {
+			if (!capable(CAP_FOWNER)) {
 				code = XFS_ERROR(EPERM);
 				goto error_return;
 			}
@@ -760,7 +770,7 @@
 			timeflags &= ~XFS_ICHGTIME_MOD;
 			timeflags |= XFS_ICHGTIME_CHG;
 		}
-		if (tp && (flags & ATTR_UTIME))
+		if (tp)
 			xfs_trans_log_inode (tp, ip, XFS_ILOG_CORE);
 	}