fix truncate inode time modification breakage

It appears that I've broken inode time modifications on tmpfs/ext2. While ftruncate always updates these attributes, truncate must not unless size is changed. I hadn't actually understood that until Christoph told me.

Confusion is increased because other filesystems get this wrong. Those without ->setattr or ->truncate get it wrong by default. Others appear to have problems too.

I haven't gone through many yet, but is there any reason not to just do it in the vfs?

Quoted 11 lines · Nick Piggin

It appears that I've broken inode time modifications on tmpfs/ext2. While ftruncate always updates these attributes, truncate must not unless size is changed. I hadn't actually understood that until Christoph told me.

Confusion is increased because other filesystems get this wrong. Those without ->setattr or ->truncate get it wrong by default. Others appear to have problems too.

I haven't gone through many yet, but is there any reason not to just do it in the vfs?

Doing it in the VFS is fine with me, we still have the the file pointer in struct iatta to indicate a ftruncate / open O_TRUNC if any filesystem really cares. But I think you need to audit all instances if they care about this. And while you're at it also remove the code handling this and the comments about it in XFS.

mm/shmem.c done2: · line 771

Quoted 4 lines

mm/shmem.c +2 -2

-	if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
-		loff_t newsize = attr->ia_size;
+	if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE) &&
+						newsize != inode->i_size) {

Btw, the S_ISREG is superflous - we only ever set ATTR_SIZE for regular files from the upper layer code.

Quoted 13 lines · Christoph Hellwig

Quoted 11 lines · Nick Piggin

It appears that I've broken inode time modifications on tmpfs/ext2. While ftruncate always updates these attributes, truncate must not unless size is changed. I hadn't actually understood that until Christoph told me.

Confusion is increased because other filesystems get this wrong. Those without ->setattr or ->truncate get it wrong by default. Others appear to have problems too.

I haven't gone through many yet, but is there any reason not to just do it in the vfs?

Quoted 6 lines

Doing it in the VFS is fine with me, we still have the the file pointer in struct iatta to indicate a ftruncate / open O_TRUNC if any filesystem really cares. But I think you need to audit all instances if they care about this. And while you're at it also remove the code handling this and the comments about it in XFS.

Yes I was starting to look at that. Just want to see if I'm on the right track.

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +2 -2

-	if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
-		loff_t newsize = attr->ia_size;
+	if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE) &&
+						newsize != inode->i_size) {

Btw, the S_ISREG is superflous - we only ever set ATTR_SIZE for regular files from the upper layer code.

OK I'll get rid of it in the same patch.

Well I looked through filesystems and I cannot seem to find anywhere they can use ATTR_SIZE|ATTR_[MC]TIME to do anything strange that would let them distinguish truncate from ftruncate and O_TRUNC.

I couldn't quite understand what FUSE was trying to do, or whether I improved it.

xfs in the 0 length file shortcut, isn't this missing suid kill case? (ftruncate mandatory mtime update seems like it wasn't working right there either before this patch).

Which means XFS behaviour for truncate while not strictly against Posix is at least unexpected. I'll fix it up.

Quoted 2 lines · Christoph Hellwig

Which means XFS behaviour for truncate while not strictly against Posix is at least unexpected. I'll fix it up.

Actually I'd prefer if you could throw this into your do_truncate patch so that filesystems don't get the clear suid request if the file size doesn't change.

That way I can just change XFS
to do everything requested but the actual size update for that case. In fact just clearing out ATTR_SIZE in that case in do_truncate would be nice.

Which brings up the question: are we guaranteed to have stable and uptodate i_size in do_truncate? I think normally we'd need a ->getattr first to stabilize it.

Quoted 3 lines · Nick Piggin

xfs in the 0 length file shortcut, isn't this missing suid kill case? (ftruncate mandatory mtime update seems like it wasn't working right there either before this patch).

The wording from Posix for truncate is:

"Upon successful completion, if the file size is changed, this function
shall mark for update the st_ctime and st_mtime fields of the file, and the S_ISUID and S_ISGID bits of the file mode may be cleared."

and for truncate:

"Upon successful completion, if fildes refers to a regular file, the
ftruncate() function shall mark for update the st_ctime and st_mtime fields of the file and the S_ISUID and S_ISGID bits of the file mode may be cleared."

Which translates to me that the suid/sgid clearing behaviour is exactly the same as the c/mtime update and it can be skipped for truncate only if there is no size change. Except that the wording is rather nasty as the suid/sgid bits are only accepted into Posix as an ugly headed stepchild and may not actually be present on some systems.

Interesting specific filesystem questions: (not comprehensive)

ecryptfs improve mtime/ctime handling for truncate of lower inode

fuse, should be setting ctime on truncate?

gfs2, mtime and ctime should only be set in truncate if the ATTR_?TIME are set.

hostfs, don't ignore ATTR_SIZE. ftruncate/truncate have interesting different semantics for timestamp updates, so don't use fd != -1 for these (could use utimes for this, but it causes an atomicity/error handling issue).

hpfs could use cont_expand to do expanding truncates?

ncpfs, nfs smbfs, cifs seems to perform inode size checks (inode_newsize_ok) after truncating the file on the server??

ntfs does not do inode size checks properly.

logfs logfs_truncate without doing inode_change_ok, inode_newsize_ok etc can fail inode_setattr after successful truncate (truncate(2) would return failure and yet the file would be truncated).

procfs why not return -EPERM rather than setting size?

reiserfs should do all setattr checks early as possible (inode_change_ok, inode_newsize_ok).

ubifs should not update access times unconditionally when ATTR_SIZE is set

ufs simple_setsize still destroys the pagecache and causes concurrent reads to go EOF past updated i_size. Restoring old i_size is too late I think. Should do those checks first (in fairness lots of filesystems have bit problems with error handling, but ufs attempts a little bit and gets it wrong).

xfs in the 0 length file shortcut, isn't this missing suid kill case? (ftruncate mandatory mtime update seems like it wasn't working right there either before this patch).

nfsd should not change attributes in the case of truncated file with no size change?

Index: linux-2.6/fs/fuse/dir.c
===================================================================

fs/fuse/dir.c +0 -4

@@ -1161,20 +1161,6 @@ static int fuse_dir_fsync(struct file *f
 	return fuse_fsync_common(file, datasync, 1);
 }
 
-static bool update_mtime(unsigned ivalid)
-{
-	/* Always update if mtime is explicitly set  */
-	if (ivalid & ATTR_MTIME_SET)

Quoted 12 lines · Nick Piggin

Index: linux-2.6/fs/fuse/dir.c
===================================================================

fs/fuse/dir.c +0 -4

@@ -1161,20 +1161,6 @@ static int fuse_dir_fsync(struct file *f
 	return fuse_fsync_common(file, datasync, 1);
 }
 
-static bool update_mtime(unsigned ivalid)
-{
-	/* Always update if mtime is explicitly set  */
-	if (ivalid & ATTR_MTIME_SET)

fs/fuse/dir.c static int fuse_dir_fsync(struct file *f · line 1177

Quoted 10 lines

fs/fuse/dir.c +0 -10

-		return true;
-
-	/* If it's an open(O_TRUNC) or an ftruncate(), don't update */
-	if ((ivalid & ATTR_SIZE) && (ivalid & (ATTR_OPEN | ATTR_FILE)))
-		return false;
-
-	/* In all other cases update */
-	return true;
-}
-

Fuse philosophy is: each operation itself has to update times on files if necessary. So it basically moves the responsibility to update [amc]time from the VFS into the filesystem.

This means the only place fuse is interested in ATTR_ATIME or ATTR_MTIME is for the utime* syscalls.

It also means that fuse always ignores ATTR_CTIME which is never set explicitly.

So I believe the current fuse code is correct.

Thanks,
Miklos
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 7 lines

Fuse philosophy is: each operation itself has to update times on files if necessary. So it basically moves the responsibility to update [amc]time from the VFS into the filesystem.

This means the only place fuse is interested in ATTR_ATIME or ATTR_MTIME is for the utime* syscalls.

Quoted 5 lines

It also means that fuse always ignores ATTR_CTIME which is never set explicitly.

So I believe the current fuse code is correct.

It might be correct, but there were reports it has problems with NFS export.

Why let the filesystems be broken? Why not do the common stuff common? (In VFS)

Quoted 7 lines · Boaz Harrosh

Quoted 7 lines

Fuse philosophy is: each operation itself has to update times on files if necessary. So it basically moves the responsibility to update [amc]time from the VFS into the filesystem.

This means the only place fuse is interested in ATTR_ATIME or ATTR_MTIME is for the utime* syscalls.

Quoted 7 lines

Quoted 5 lines

It also means that fuse always ignores ATTR_CTIME which is never set explicitly.

So I believe the current fuse code is correct.

It might be correct, but there were reports it has problems with NFS export.

Do you have details? I can't remember any report related to time modification and NFS export.

Quoted 2 lines

Why let the filesystems be broken? Why not do the common stuff common? (In VFS)

For consistency and simplicity. The fuse API looks much like the syscall API, which means that for a truncate() call on a fuse file the truncate() method will be called in the filesystem, not a truncate() + utimes(). And so on...

Thanks,
Miklos

Quoted 12 lines · Nick Piggin

Index: linux-2.6/fs/fuse/dir.c
===================================================================

fs/fuse/dir.c +0 -4

@@ -1161,20 +1161,6 @@ static int fuse_dir_fsync(struct file *f
 	return fuse_fsync_common(file, datasync, 1);
 }
 
-static bool update_mtime(unsigned ivalid)
-{
-	/* Always update if mtime is explicitly set  */
-	if (ivalid & ATTR_MTIME_SET)

fs/fuse/dir.c static int fuse_dir_fsync(struct file *f · line 1177

Quoted 10 lines

fs/fuse/dir.c +0 -10

-		return true;
-
-	/* If it's an open(O_TRUNC) or an ftruncate(), don't update */
-	if ((ivalid & ATTR_SIZE) && (ivalid & (ATTR_OPEN | ATTR_FILE)))
-		return false;
-
-	/* In all other cases update */
-	return true;
-}
-

Fuse philosophy is: each operation itself has to update times on files if necessary. So it basically moves the responsibility to update [amc]time from the VFS into the filesystem.

This means the only place fuse is interested in ATTR_ATIME or ATTR_MTIME is for the utime* syscalls.

It also means that fuse always ignores ATTR_CTIME which is never set explicitly.

So I believe the current fuse code is correct.

Quoted 6 lines

Fuse philosophy is: each operation itself has to update times on files if necessary. So it basically moves the responsibility to update [amc]time from the VFS into the filesystem.

This means the only place fuse is interested in ATTR_ATIME or ATTR_MTIME is for the utime* syscalls.

OK, makes sense. I wonder why you do ATTR_ATIME changes, though, rather than just getting those too back from the filesystem?

Quoted 4 lines

It also means that fuse always ignores ATTR_CTIME which is never set explicitly.

So I believe the current fuse code is correct.

After my patch to pass ATTR_MTIME|ATTR_CTIME from truncate(2), it is not (because above won't return false, ie. it will change the mtime for truncate).

Why not avoid all mtime updates except MTIME_SET?

Quoted 4 lines · Nick Piggin

Quoted 4 lines

Fuse philosophy is: each operation itself has to update times on files if necessary. So it basically moves the responsibility to update [amc]time from the VFS into the filesystem.

Quoted 5 lines

Quoted 2 lines

This means the only place fuse is interested in ATTR_ATIME or ATTR_MTIME is for the utime* syscalls.

OK, makes sense. I wonder why you do ATTR_ATIME changes, though, rather than just getting those too back from the filesystem?

It does that, through setting S_NOATIME and invalidating attributes after each op.

Quoted 5 lines

Quoted 4 lines

It also means that fuse always ignores ATTR_CTIME which is never set explicitly.

So I believe the current fuse code is correct.

Quoted 5 lines

After my patch to pass ATTR_MTIME|ATTR_CTIME from truncate(2), it is not (because above won't return false, ie. it will change the mtime for truncate).

Why not avoid all mtime updates except MTIME_SET?

Because utimes(NULL) will supply only ATTR_ATIME | ATTR_MTIME.

I guess fuse should do something like this:

	if (valid ^ (ATTR_MTIME | ATTR_CTIME | ATTR_SIZE) == 0)
		valid = ATTR_SIZE;

You'll have to be careful, truncate will pass other things down like mode to get rid of suid.

If you just wanted to ignore mtime changes on truncate, then masking it off would be the way to go I think.

if (valid & ATTR_SIZE)
valid &= ~ATTR_SIZE;

Would you also want to do the same thing with suid kill bits from truncate, then? Mask off ATTR_MODE and just read it back from the server too?

(In fact I think it would be cleanest if truncate/ftruncate was a separate operation from setattr on all levels, but that's a different story.)

Quoted 2 lines · Nick Piggin

You'll have to be careful, truncate will pass other things down like mode to get rid of suid.

Oh, right.

Quoted 6 lines

If you just wanted to ignore mtime changes on truncate, then masking it off would be the way to go I think.

if (valid & ATTR_SIZE)
valid &= ~ATTR_SIZE;

Quoted 3 lines

Would you also want to do the same thing with suid kill bits from truncate, then? Mask off ATTR_MODE and just read it back from the server too?

That would be logical, but no, it didn't used to do that, and now it can't start doing it for fear of creating a security hole.

Also suid removal happens very rarely compared to mtime update, so a rare additional chmod request (which might be superfluous for some filesystems) in addition to write and truncate is I think acceptable.

(In fact I think it would be cleanest if truncate/ftruncate was a separate operation from setattr on all levels, but that's a different story.)

So for now something like

	if (valid & ATTR_SIZE)
		valid &= ~(ATTR_MTIME | ATTR_CTIME);

would work?

Quoted 3 lines

(In fact I think it would be cleanest if truncate/ftruncate was a separate operation from setattr on all levels, but that's a different story.)

Well it's possible. It is a combination of inode and address space operation really. Because you really want to update mtime/ctime and suid bits iff the truncate succeeds.

We did consider a new API for it, but it didn't seem to be an obvious improvement. A filesystem can easily branch into another function for ATTR_SIZE immediately on setattr entry.

Quoted 2 lines

So for now something like

Quoted 4 lines

	if (valid & ATTR_SIZE)
		valid &= ~(ATTR_MTIME | ATTR_CTIME);

would work?

That should do the trick, yes. But I think CTIME would just confuse things seeing as you ignore it everywhere else.

It appears that I've broken inode time modifications on tmpfs/ext2. While ftruncate always updates these attributes, truncate must not unless size is changed. I hadn't actually understood that until Christoph told me.

Confusion is increased because other filesystems get this wrong. Those without ->setattr or ->truncate get it wrong by default. Others appear to have problems too.

I haven't gone through many yet, but is there any reason not to just do it in the vfs?

---
 fs/ext2/inode.c       |    1 -
 fs/open.c             |    3 +++
 fs/ramfs/file-nommu.c |    5 -----
 mm/shmem.c            |    5 +++--
 4 files changed, 6 insertions(+), 8 deletions(-)

Index: linux-2.6/fs/ext2/inode.c
===================================================================

fs/ext2/inode.c +0 -0

 	if (inode_needs_sync(inode)) {
 		sync_mapping_buffers(inode->i_mapping);
 		ext2_sync_inode (inode);

Index: linux-2.6/fs/open.c
===================================================================

fs/open.c +3 -0

@@ -55,6 +55,9 @@ int do_truncate(struct dentry *dentry, l
 		newattrs.ia_valid |= ret | ATTR_FORCE;
 
 	mutex_lock(&dentry->d_inode->i_mutex);
+	/* Unlike ftruncate, truncate only updates times when size changes */
+	if (length != dentry->d_inode->i_size)
+		newattrs.ia_valid |= ATTR_MTIME|ATTR_CTIME;
 	ret = notify_change(dentry, &newattrs);
 	mutex_unlock(&dentry->d_inode->i_mutex);
 	return ret;

Index: linux-2.6/mm/shmem.c
===================================================================

mm/shmem.c +3 -2

@@ -764,10 +764,11 @@ done2:
 static int shmem_notify_change(struct dentry *dentry, struct iattr *attr)
 {
 	struct inode *inode = dentry->d_inode;
+	loff_t newsize = attr->ia_size;
 	int error;
 
-	if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
-		loff_t newsize = attr->ia_size;
+	if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE) &&
+						newsize != inode->i_size) {
 		struct page *page = NULL;
 
 		if (newsize < inode->i_size) {

Index: linux-2.6/fs/ramfs/file-nommu.c
===================================================================

fs/ramfs/file-nommu.c +0 -6

@@ -175,11 +175,6 @@ static int ramfs_nommu_setattr(struct de
 			ret = ramfs_nommu_resize(inode, ia->ia_size, size);
 			if (ret < 0 || ia->ia_valid == ATTR_SIZE)
 				goto out;
-		} else {
-			/* we skipped the truncate but must still update
-			 * timestamps
-			 */
-			ia->ia_valid |= ATTR_MTIME|ATTR_CTIME;
 		}
 	}
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 27 lines · Nick Piggin

It appears that I've broken inode time modifications on tmpfs/ext2. While ftruncate always updates these attributes, truncate must not unless size is changed. I hadn't actually understood that until Christoph told me.

Confusion is increased because other filesystems get this wrong. Those without ->setattr or ->truncate get it wrong by default. Others appear to have problems too.

I haven't gone through many yet, but is there any reason not to just do it in the vfs?

---
 fs/ext2/inode.c       |    1 -
 fs/open.c             |    3 +++
 fs/ramfs/file-nommu.c |    5 -----
 mm/shmem.c            |    5 +++--
 4 files changed, 6 insertions(+), 8 deletions(-)

Index: linux-2.6/fs/ext2/inode.c
===================================================================

fs/ext2/inode.c +0 -0

@@ -1203,7 +1203,6 @@ int ext2_setsize(struct inode *inode, lo
 
 	__ext2_truncate_blocks(inode, newsize);
 

fs/ext2/inode.c int ext2_setsize(struct inode *inode, lo · line 1206

Quoted 1 line

fs/ext2/inode.c +0 -1

-	inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;

OK I was just investigating this. Please forgive my noviceness here:

So i_mtime is modifications time. i_ctime is creation time?

if I do ftrunc() (like dd skip=x) don't I want modification time changed but creation time unchanged?