fs: Fix truncate never updates m/ctime

Commit Message

Jubin Zhong Nov. 15, 2021, 11 a.m. UTC

From: zhongjubin <[e-mail]>

Syscall truncate() never updates m/ctime even if the file size is
changed. However, this is incorrect according to man file:

  truncate (2):
  If  the  size  changed, then the st_ctime and st_mtime fields
  (respectively, time of last status change and time of last modification;
  see stat(2)) for the file are updated, and the set-user-ID and
  set-group-ID mode bits may be cleared.

Check file size before do_truncate() to fix this.

Signed-off-by: zhongjubin <[e-mail]>
---
 fs/open.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Christoph Hellwig Nov. 15, 2021, 5:58 p.m. UTC | #1

On Mon, Nov 15, 2021 at 07:00:18PM +0800, Jubin Zhong wrote:
> From: zhongjubin <[e-mail]>
> 
> Syscall truncate() never updates m/ctime even if the file size is
> changed. However, this is incorrect according to man file:
> 
>   truncate (2):
>   If  the  size  changed, then the st_ctime and st_mtime fields
>   (respectively, time of last status change and time of last modification;
>   see stat(2)) for the file are updated, and the set-user-ID and
>   set-group-ID mode bits may be cleared.
> 
> Check file size before do_truncate() to fix this.

Please try to actually reproduce your alleged "bug".  And maybe also
look at the actual setattr implementations.  Hint: The XFS one even
has extensive comments.

Jubin Zhong Nov. 16, 2021, 3:58 a.m. UTC | #2

> On Mon, Nov 15, 2021 at 07:00:18PM +0800, Jubin Zhong wrote:
>> From: zhongjubin <[e-mail]>
>> 
>> Syscall truncate() never updates m/ctime even if the file size is
>> changed. However, this is incorrect according to man file:
>> 
>>   truncate (2):
>>   If  the  size  changed, then the st_ctime and st_mtime fields
>>   (respectively, time of last status change and time of last modification;
>>   see stat(2)) for the file are updated, and the set-user-ID and
>>   set-group-ID mode bits may be cleared.
>> 
>> Check file size before do_truncate() to fix this.
>
> Please try to actually reproduce your alleged "bug".  And maybe also
> look at the actual setattr implementations.  Hint: The XFS one even
> has extensive comments.

Thanks for your advice. I found this problem on yaffs2 in the beginning,
ftruncate() always works fine but truncate() does not. Now I have done 
a few more tests and the following are the results:

Test Environmont:
	kernel: Linux Kernel v5.16
	hardware: QEMU emulator version 3.1.0
	arch: vexpress-v2p-ca9

Teset Results:
	filesystems     m/ctime updated by truncate?
	jffs2           fail
	yaffs2          fail
	ubifs           success
	ext2            success
	ext4            success
	tmpfs           success
	xfs             success

Test Steps:
	1. cd /path/to/mnt/point
	2. dd if=/dev/zero of=test bs=1M count=1
	3. stat test
	4. /bin/my_truncate -s 1024 test
	5. stat test
	6. compare m/ctime of step 5 with step 3

Program source:
	#include <stdio.h>
	#include <stdlib.h>
	#include <string.h>
	#include <unistd.h>
	#include <sys/types.h>
	int main(int argc, char **argv)
	{
		int ret;
		char file_name[128] = {0};
		
		if (argc < 4 || argv == NULL || argv[1] == NULL || argv[2] == NULL || argv[3] == NULL) {
			return -1;
		}
		
		if (strcmp(argv[1], "-s")) {
			return -1;
		}
		
		if (realpath(argv[3], file_name) == NULL) {
			printf("truncate: input file name %s err.\n", argv[3]);
			return -1;
		}
		
		off_t size = (off_t)strtol(argv[2], 0, 0);
		ret = truncate(file_name, size);
		if (ret) {
			printf("truncate return err %d\n", ret);
		}   
		return ret;
	}   

I work on embedded devices so concern about jffs2/yaffs2/ubifs the most. 
If there are any errors in my test program please let me know.

Thanks.

Christoph Hellwig Nov. 16, 2021, 5:22 a.m. UTC | #3

On Tue, Nov 16, 2021 at 11:58:10AM +0800, Jubin Zhong wrote:
> I work on embedded devices so concern about jffs2/yaffs2/ubifs the most. 
> If there are any errors in my test program please let me know.

It seems like you need to fix jffs2 to implement the proper semantics
in its ->setattr.

Jubin Zhong Nov. 16, 2021, 6:57 a.m. UTC | #4

> It seems like you need to fix jffs2 to implement the proper semantics in its ->setattr.

Yes I have thought of this solution. However, when I tried to
track this problem down, I found that ftruncate() had similar
problem and it was fixed by commit 6e656be89999 ("ftruncate 
does not always update m/ctime"):

	diff --git a/fs/open.c b/fs/open.c
	index 5fb16e5267dc..303f06d2a7b9 100644
	--- a/fs/open.c
	+++ b/fs/open.c
	@@ -322,7 +322,7 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
 
        error = locks_verify_truncate(inode, file, length);
        if (!error)
	-   	error = do_truncate(dentry, length, 0, file);
	+   	error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, file);
 	out_putf:
        fput(file);
	 out:

In my opinion, there are two advantages if we fix it in
vfs_truncate():

1. All filesystems can reuse the scheme without adapting
Separately, just like what we did for ftruncate().

2. In the case when old_size = new_size, we can avoid
calling do_truncate() and return without doing anything.

Hope that you can consider my suggestion, thanks.

Christoph Hellwig Nov. 16, 2021, 7:02 a.m. UTC | #5

> In my opinion, there are two advantages if we fix it in
> vfs_truncate():

Please actually read the comments in the xfs setattr implementation
I pointed you to first time.

Patch

diff --git a/fs/open.c b/fs/open.c
index f732fb94600c..02404b759c2e 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -106,8 +106,11 @@  long vfs_truncate(const struct path *path, loff_t length)
 		goto put_write_and_out;
 
 	error = security_path_truncate(path);
-	if (!error)
-		error = do_truncate(mnt_userns, path->dentry, length, 0, NULL);
+	if (error)
+		goto put_write_and_out;
+
+	if (i_size_read(inode) != length)
+		error = do_truncate(mnt_userns, path->dentry, length, ATTR_MTIME | ATTR_CTIME, NULL);
 
 put_write_and_out:
 	put_write_access(inode);