vfs: Add support for timestamp limits

fs/fat/misc.c +0 -0

index 1e08bd54c5fb..53bb7c6bf993 100644
@@ -307,8 +307,9 @@ int fat_truncate_time(struct inode *inode, struct timespec64 *now, int flags)
 		inode->i_atime = (struct timespec64){ seconds, 0 };

Quoted 11 lines · Deepa Dinamani

fs/fat/misc.c +1 -2

index 1e08bd54c5fb..53bb7c6bf993 100644
@@ -307,8 +307,9 @@ int fat_truncate_time(struct inode *inode, struct timespec64 *now, int flags)
              inode->i_atime = (struct timespec64){ seconds, 0 };
      }
      if (flags & S_CTIME) {
-             if (sbi->options.isvfat)
-                     inode->i_ctime = timespec64_trunc(*now, 10000000);
+             if (sbi->options.isvfat) {

fs/fat/misc.c int fat_truncate_time(struct inode *inode, struct timespec64 *now, int flags) · line 315

Quoted 5 lines

fs/fat/misc.c +2 -0

+                     inode->i_ctime = timestamp_truncate(*now, inode);
+             }
              else
                      inode->i_ctime = fat_timespec64_trunc_2secs(*now);
      }

Looks like broken. It changed to sb->s_time_gran from 10000000, and changed coding style.

Quoted 14 lines · OGAWA Hirofumi

Quoted 11 lines · Deepa Dinamani

fs/fat/misc.c +1 -2

index 1e08bd54c5fb..53bb7c6bf993 100644
@@ -307,8 +307,9 @@ int fat_truncate_time(struct inode *inode, struct timespec64 *now, int flags)
             inode->i_atime = (struct timespec64){ seconds, 0 };
     }
     if (flags & S_CTIME) {
-             if (sbi->options.isvfat)
-                     inode->i_ctime = timespec64_trunc(*now, 10000000);
+             if (sbi->options.isvfat) {

Quoted 8 lines

fs/fat/misc.c int fat_truncate_time(struct inode *inode, struct timespec64 *now, int flags) · line 315

Quoted 5 lines

fs/fat/misc.c +2 -0

+                     inode->i_ctime = timestamp_truncate(*now, inode);
+             }
             else
                     inode->i_ctime = fat_timespec64_trunc_2secs(*now);
     }

Looks like broken. It changed to sb->s_time_gran from 10000000, and changed coding style.

This is using a new api: timestamp_truncate(). granularity is gotten by inode->sb->s_time_gran. See Patch [2/20]:
https://lkml.org/lkml/2019/7/29/1853

So this is not broken if fat is filling in the right granularity in the sb.

-Deepa

Hi Deepa,

Quoted 27 lines · Deepa Dinamani

Quoted 22 lines · OGAWA Hirofumi

Quoted 6 lines · Deepa Dinamani

fs/fat/misc.c +0 -0

index 1e08bd54c5fb..53bb7c6bf993 100644
@@ -307,8 +307,9 @@ int fat_truncate_time(struct inode *inode, struct timespec64 *now, int flags)
             inode->i_atime = (struct timespec64){ seconds, 0 };

fs/fat/misc.c int fat_truncate_time(struct inode *inode, struct timespec64 *now, int flags) · line 315

Quoted 10 lines

fs/fat/misc.c +3 -2

     }
     if (flags & S_CTIME) {
-             if (sbi->options.isvfat)
-                     inode->i_ctime = timespec64_trunc(*now, 10000000);
+             if (sbi->options.isvfat) {
+                     inode->i_ctime = timestamp_truncate(*now, inode);
+             }
             else
                     inode->i_ctime = fat_timespec64_trunc_2secs(*now);
     }

Looks like broken. It changed to sb->s_time_gran from 10000000, and changed coding style.

Quoted 5 lines

This is using a new api: timestamp_truncate(). granularity is gotten by inode->sb->s_time_gran. See Patch [2/20]:
https://lkml.org/lkml/2019/7/29/1853

So this is not broken if fat is filling in the right granularity in the sb.

It is broken for FAT because FAT has different granularities for different timestamps so it cannot put the correct value in the sb as that only allows one granularity. Your patch is totally broken for fat as it would be immediately obvious if you spent a few minutes looking at the code...

The warning reuses the uptime max of 30 years used by the setitimeofday().

Note that the warning is only added for new filesystem mounts through the mount syscall. Automounts do not have the same warning.

Signed-off-by Deepa Dinamani <[email protected]>

---
 fs/namespace.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

fs/namespace.c +0 -0

index b26778bdc236..5314fac8035e 100644

fs/namespace.c +3 -0

+		free_page((unsigned long)buf);
+	}
+
 	return error;
 }
 

Quoted 11 lines · Deepa Dinamani

The warning reuses the uptime max of 30 years used by the setitimeofday().

Note that the warning is only added for new filesystem mounts through the mount syscall. Automounts do not have the same warning.

Signed-off-by Deepa Dinamani <[email protected]>

---
 fs/namespace.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

fs/namespace.c static int do_new_mount_fc(struct fs_context *fc, struct path *mountpoint, · line 2743

Quoted 10 lines

fs/namespace.c +2 -0

index b26778bdc236..5314fac8035e 100644
@@ -2739,6 +2739,17 @@ static int do_new_mount_fc(struct fs_context *fc, struct path *mountpoint,
      error = do_add_mount(real_mount(mnt), mountpoint, mnt_flags);
      if (error < 0)
              mntput(mnt);
+
+     if (!error && sb->s_time_max &&

I don't know why you are testing sb->s_time_max here - it should always be non-zero since alloc_super() sets it to TIME64_MAX.

This doesn't seem like a helpful way to log the time. Maybe use time64_to_tm() to convert to "broken down" time and then print it with "%ptR"... but that wants struct rtc_time. If you apply the attached patch, however, you should then be able to print struct tm with "%ptT".

The warning reuses the uptime max of 30 years used by the setitimeofday().

Note that the warning is only added for new filesystem mounts through the mount syscall. Automounts do not have the same warning.

Quoted 5 lines · Deepa Dinamani

The warning reuses the uptime max of 30 years used by the setitimeofday().

Note that the warning is only added for new filesystem mounts through the mount syscall. Automounts do not have the same warning.

[...]

Another thing - perhaps this warning should be suppressed for read-only mounts?

Quoted 10 lines · Ben Hutchings

Quoted 5 lines · Deepa Dinamani

The warning reuses the uptime max of 30 years used by the setitimeofday().

Note that the warning is only added for new filesystem mounts through the mount syscall. Automounts do not have the same warning.

[...]

Another thing - perhaps this warning should be suppressed for read-only mounts?

Many filesystems support read only mounts only. We do fill in right granularities and limits for these filesystems as well. In keeping with the trend, I have added the warning accordingly. I don't think I have a preference either way. But, not warning for the red only mounts adds another if case. If you have a strong preference, I could add it in.

Quoted 13 lines · Deepa Dinamani

Quoted 10 lines · Ben Hutchings

Quoted 5 lines · Deepa Dinamani

The warning reuses the uptime max of 30 years used by the setitimeofday().

Note that the warning is only added for new filesystem mounts through the mount syscall. Automounts do not have the same warning.

[...]

Another thing - perhaps this warning should be suppressed for read-only mounts?

Quoted 6 lines

Many filesystems support read only mounts only. We do fill in right granularities and limits for these filesystems as well. In keeping with the trend, I have added the warning accordingly. I don't think I have a preference either way. But, not warning for the red only mounts adds another if case. If you have a strong preference, I could add it in.

It seems to me that the warning is needed if there is a possibility of data loss (incorrect timestamps, potentially leading to incorrect decisions about which files are newer). This can happen only when a filesystem is mounted read-write, or when a filesystem image is created.

I think that warning for read-only mounts would be an annoyance to users retrieving files from old filesystems.

Quoted 27 lines · Ben Hutchings

Quoted 13 lines · Deepa Dinamani

Quoted 10 lines · Ben Hutchings

Quoted 5 lines · Deepa Dinamani

The warning reuses the uptime max of 30 years used by the setitimeofday().

Note that the warning is only added for new filesystem mounts through the mount syscall. Automounts do not have the same warning.

[...]

Another thing - perhaps this warning should be suppressed for read-only mounts?

Quoted 6 lines

Many filesystems support read only mounts only. We do fill in right granularities and limits for these filesystems as well. In keeping with the trend, I have added the warning accordingly. I don't think I have a preference either way. But, not warning for the red only mounts adds another if case. If you have a strong preference, I could add it in.

It seems to me that the warning is needed if there is a possibility of data loss (incorrect timestamps, potentially leading to incorrect decisions about which files are newer). This can happen only when a filesystem is mounted read-write, or when a filesystem image is created.

Quoted 2 lines

I think that warning for read-only mounts would be an annoyance to users retrieving files from old filesystems.

I agree, the warning is not helpful for read-only mounts. An earlier plan was to completely disallow writable mounts that might risk an overflow (in some configurations at least). The warning replaces that now, and I think it should also just warn for the cases that would otherwise have been dangerous.

Quoted 30 lines · Arnd Bergmann

Quoted 27 lines · Ben Hutchings

Quoted 13 lines · Deepa Dinamani

Quoted 10 lines · Ben Hutchings

Quoted 5 lines · Deepa Dinamani

The warning reuses the uptime max of 30 years used by the setitimeofday().

Note that the warning is only added for new filesystem mounts through the mount syscall. Automounts do not have the same warning.

[...]

Another thing - perhaps this warning should be suppressed for read-only mounts?

Quoted 6 lines

Many filesystems support read only mounts only. We do fill in right granularities and limits for these filesystems as well. In keeping with the trend, I have added the warning accordingly. I don't think I have a preference either way. But, not warning for the red only mounts adds another if case. If you have a strong preference, I could add it in.

It seems to me that the warning is needed if there is a possibility of data loss (incorrect timestamps, potentially leading to incorrect decisions about which files are newer). This can happen only when a filesystem is mounted read-write, or when a filesystem image is created.

Quoted 8 lines

Quoted 2 lines

I think that warning for read-only mounts would be an annoyance to users retrieving files from old filesystems.

I agree, the warning is not helpful for read-only mounts. An earlier plan was to completely disallow writable mounts that might risk an overflow (in some configurations at least). The warning replaces that now, and I think it should also just warn for the cases that would otherwise have been dangerous.

Ok, I will make the change to exclude new read only mounts. I will use __mnt_is_readonly() so that it also exculdes filesystems that are readonly also.
The diff looks like below:

(quoted diff) +1 -1

-       if (!error && sb->s_time_max &&
+       if (!error && !__mnt_is_readonly(mnt) &&
            (ktime_get_real_seconds() + TIME_UPTIME_SEC_MAX > sb->s_time_max)) {

Note that we can get rid of checking for non zero sb->s_time_max now.

Quoted 44 lines · Deepa Dinamani

Quoted 29 lines · Arnd Bergmann

Quoted 27 lines · Ben Hutchings

Quoted 13 lines · Deepa Dinamani

Quoted 10 lines · Ben Hutchings

Quoted 5 lines · Deepa Dinamani

The warning reuses the uptime max of 30 years used by the setitimeofday().

Note that the warning is only added for new filesystem mounts through the mount syscall. Automounts do not have the same warning.

[...]

Another thing - perhaps this warning should be suppressed for read-only mounts?

Quoted 6 lines

Many filesystems support read only mounts only. We do fill in right granularities and limits for these filesystems as well. In keeping with the trend, I have added the warning accordingly. I don't think I have a preference either way. But, not warning for the red only mounts adds another if case. If you have a strong preference, I could add it in.

It seems to me that the warning is needed if there is a possibility of data loss (incorrect timestamps, potentially leading to incorrect decisions about which files are newer). This can happen only when a filesystem is mounted read-write, or when a filesystem image is created.

Quoted 8 lines

Quoted 2 lines

I think that warning for read-only mounts would be an annoyance to users retrieving files from old filesystems.

I agree, the warning is not helpful for read-only mounts. An earlier plan was to completely disallow writable mounts that might risk an overflow (in some configurations at least). The warning replaces that now, and I think it should also just warn for the cases that would otherwise have been dangerous.

Ok, I will make the change to exclude new read only mounts. I will use __mnt_is_readonly() so that it also exculdes filesystems that are readonly also.
The diff looks like below:

Quoted 5 lines

(quoted diff) +1 -1

-       if (!error && sb->s_time_max &&
+       if (!error && !__mnt_is_readonly(mnt) &&
            (ktime_get_real_seconds() + TIME_UPTIME_SEC_MAX > sb->s_time_max)) {

Note that we can get rid of checking for non zero sb->s_time_max now.

One more thing, we will probably have to add a second warning for when the filesystem is re-mounted rw after the initial readonly mount.

POSIX is ambiguous on the behavior of timestamps for futimens, utimensat and utimes. Whether to return an error or silently clamp a timestamp beyond the range supported by the underlying filesystems is not clear.

POSIX.1 section for futimens, utimensat and utimes says:
(http://pubs.opengroup.org/onlinepubs/9699919799/functions/futimens.html)

The file's relevant timestamp shall be set to the greatest value supported by the file system that is not greater than the specified time.

If the tv_nsec field of a timespec structure has the special value UTIME_NOW, the file's relevant timestamp shall be set to the greatest value supported by the file system that is not greater than the current time.

[EINVAL]
A new file timestamp would be a value whose tv_sec component is not a value supported by the file system.

The patch chooses to clamp the timestamps according to the
filesystem timestamp ranges and does not return an error.
This is in line with the behavior of utime syscall also
since the POSIX page(http://pubs.opengroup.org/onlinepubs/009695399/functions/utime.html) for utime does not mention returning an error or clamping like above.

Same for utimes http://pubs.opengroup.org/onlinepubs/009695399/functions/utimes.html

Signed-off-by Deepa Dinamani <[email protected]>

---
 fs/utimes.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

fs/utimes.c +7 -2

index 350c9c16ace1..4c1a2ce90bbc 100644
@@ -21,6 +21,7 @@ static int utimes_common(const struct path *path, struct timespec64 *times)
 	int error;
 	struct iattr newattrs;
 	struct inode *inode = path->dentry->d_inode;
+	struct super_block *sb = inode->i_sb;
 	struct inode *delegated_inode = NULL;
 
 	error = mnt_want_write(path->mnt);

@@ -36,16 +37,24 @@ static int utimes_common(const struct path *path, struct timespec64 *times)
 		if (times[0].tv_nsec == UTIME_OMIT)
 			newattrs.ia_valid &= ~ATTR_ATIME;
 		else if (times[0].tv_nsec != UTIME_NOW) {
-			newattrs.ia_atime.tv_sec = times[0].tv_sec;
-			newattrs.ia_atime.tv_nsec = times[0].tv_nsec;
+			newattrs.ia_atime.tv_sec =
+				clamp(times[0].tv_sec, sb->s_time_min, sb->s_time_max);
+			if (times[0].tv_sec == sb->s_time_max || times[0].tv_sec == sb->s_time_min)
+				newattrs.ia_atime.tv_nsec = 0;
+			else
+				newattrs.ia_atime.tv_nsec = times[0].tv_nsec;
 			newattrs.ia_valid |= ATTR_ATIME_SET;

fs/utimes.c +3 -0

+				newattrs.ia_mtime.tv_nsec = 0;
+			else
+				newattrs.ia_mtime.tv_nsec = times[1].tv_nsec;
 			newattrs.ia_valid |= ATTR_MTIME_SET;
 		}
 		/*

Signature · 1 line

2.17.1

Quoted 59 lines · Deepa Dinamani

POSIX is ambiguous on the behavior of timestamps for futimens, utimensat and utimes. Whether to return an error or silently clamp a timestamp beyond the range supported by the underlying filesystems is not clear.

POSIX.1 section for futimens, utimensat and utimes says:
(http://pubs.opengroup.org/onlinepubs/9699919799/functions/futimens.html)

The file's relevant timestamp shall be set to the greatest value supported by the file system that is not greater than the specified time.

If the tv_nsec field of a timespec structure has the special value UTIME_NOW, the file's relevant timestamp shall be set to the greatest value supported by the file system that is not greater than the current time.

[EINVAL]
A new file timestamp would be a value whose tv_sec component is not a value supported by the file system.

The patch chooses to clamp the timestamps according to the
filesystem timestamp ranges and does not return an error.
This is in line with the behavior of utime syscall also
since the POSIX page(http://pubs.opengroup.org/onlinepubs/009695399/functions/utime.html) for utime does not mention returning an error or clamping like above.

Same for utimes http://pubs.opengroup.org/onlinepubs/009695399/functions/utimes.html

Signed-off-by Deepa Dinamani <[email protected]>

---
 fs/utimes.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

fs/utimes.c +7 -2

index 350c9c16ace1..4c1a2ce90bbc 100644
@@ -21,6 +21,7 @@ static int utimes_common(const struct path *path, struct timespec64 *times)
      int error;
      struct iattr newattrs;
      struct inode *inode = path->dentry->d_inode;
+     struct super_block *sb = inode->i_sb;
      struct inode *delegated_inode = NULL;
 
      error = mnt_want_write(path->mnt);

@@ -36,16 +37,24 @@ static int utimes_common(const struct path *path, struct timespec64 *times)
              if (times[0].tv_nsec == UTIME_OMIT)
                      newattrs.ia_valid &= ~ATTR_ATIME;
              else if (times[0].tv_nsec != UTIME_NOW) {
-                     newattrs.ia_atime.tv_sec = times[0].tv_sec;
-                     newattrs.ia_atime.tv_nsec = times[0].tv_nsec;
+                     newattrs.ia_atime.tv_sec =
+                             clamp(times[0].tv_sec, sb->s_time_min, sb->s_time_max);
+                     if (times[0].tv_sec == sb->s_time_max || times[0].tv_sec == sb->s_time_min)
+                             newattrs.ia_atime.tv_nsec = 0;
+                     else
+                             newattrs.ia_atime.tv_nsec = times[0].tv_nsec;
                      newattrs.ia_valid |= ATTR_ATIME_SET;

fs/utimes.c static int utimes_common(const struct path *path, struct timespec64 *times) · line 54

Quoted 10 lines

fs/utimes.c +3 -2

              }
 
              if (times[1].tv_nsec == UTIME_OMIT)
                      newattrs.ia_valid &= ~ATTR_MTIME;
              else if (times[1].tv_nsec != UTIME_NOW) {
-                     newattrs.ia_mtime.tv_sec = times[1].tv_sec;
-                     newattrs.ia_mtime.tv_nsec = times[1].tv_nsec;
+                     newattrs.ia_mtime.tv_sec =
+                             clamp(times[1].tv_sec, sb->s_time_min, sb->s_time_max);
+                     if (times[1].tv_sec >= sb->s_time_max || times[1].tv_sec == sb->s_time_min)

Line length.

Also, didn't you just introduce a function to clamp tv_sec and fix granularity? Why not just use it here? I think this is the third time I've seen this open-coded logic.

POSIX is ambiguous on the behavior of timestamps for futimens, utimensat and utimes. Whether to return an error or silently clamp a timestamp beyond the range supported by the underlying filesystems is not clear.

POSIX.1 section for futimens, utimensat and utimes says:
(http://pubs.opengroup.org/onlinepubs/9699919799/functions/futimens.html)

The file's relevant timestamp shall be set to the greatest value supported by the file system that is not greater than the specified time.

If the tv_nsec field of a timespec structure has the special value UTIME_NOW, the file's relevant timestamp shall be set to the greatest value supported by the file system that is not greater than the current time.

[EINVAL]
A new file timestamp would be a value whose tv_sec component is not a value supported by the file system.

The patch chooses to clamp the timestamps according to the
filesystem timestamp ranges and does not return an error.
This is in line with the behavior of utime syscall also
since the POSIX page(http://pubs.opengroup.org/onlinepubs/009695399/functions/utime.html) for utime does not mention returning an error or clamping like above.

Same for utimes http://pubs.opengroup.org/onlinepubs/009695399/functions/utimes.html

Signed-off-by Deepa Dinamani <[email protected]>

---
 fs/utimes.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

fs/utimes.c +1 -0

index 350c9c16ace1..4c1a2ce90bbc 100644
@@ -21,6 +21,7 @@ static int utimes_common(const struct path *path, struct timespec64 *times)
 	int error;
 	struct iattr newattrs;
 	struct inode *inode = path->dentry->d_inode;
+	struct super_block *sb = inode->i_sb;
 	struct inode *delegated_inode = NULL;
 
 	error = mnt_want_write(path->mnt);

fs/utimes.c +3 -0

+				newattrs.ia_atime.tv_nsec = 0;
+			else
+				newattrs.ia_atime.tv_nsec = times[0].tv_nsec;
 			newattrs.ia_valid |= ATTR_ATIME_SET;

fs/utimes.c +3 -0

+				newattrs.ia_mtime.tv_nsec = 0;
+			else
+				newattrs.ia_mtime.tv_nsec = times[1].tv_nsec;
 			newattrs.ia_valid |= ATTR_MTIME_SET;
 		}
 		/*

Quoted 47 lines · Deepa Dinamani

POSIX is ambiguous on the behavior of timestamps for futimens, utimensat and utimes. Whether to return an error or silently clamp a timestamp beyond the range supported by the underlying filesystems is not clear.

POSIX.1 section for futimens, utimensat and utimes says:
(http://pubs.opengroup.org/onlinepubs/9699919799/functions/futimens.html)

The file's relevant timestamp shall be set to the greatest value supported by the file system that is not greater than the specified time.

If the tv_nsec field of a timespec structure has the special value UTIME_NOW, the file's relevant timestamp shall be set to the greatest value supported by the file system that is not greater than the current time.

[EINVAL]
A new file timestamp would be a value whose tv_sec component is not a value supported by the file system.

The patch chooses to clamp the timestamps according to the
filesystem timestamp ranges and does not return an error.
This is in line with the behavior of utime syscall also
since the POSIX page(http://pubs.opengroup.org/onlinepubs/009695399/functions/utime.html) for utime does not mention returning an error or clamping like above.

Same for utimes http://pubs.opengroup.org/onlinepubs/009695399/functions/utimes.html

Signed-off-by Deepa Dinamani <[email protected]>

---
 fs/utimes.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

fs/utimes.c +1 -0

index 350c9c16ace1..4c1a2ce90bbc 100644
@@ -21,6 +21,7 @@ static int utimes_common(const struct path *path, struct timespec64 *times)
      int error;
      struct iattr newattrs;
      struct inode *inode = path->dentry->d_inode;
+     struct super_block *sb = inode->i_sb;
      struct inode *delegated_inode = NULL;
 
      error = mnt_want_write(path->mnt);

fs/utimes.c static int utimes_common(const struct path *path, struct timespec64 *times) · line 42

Quoted 8 lines

fs/utimes.c +3 -2

@@ -36,16 +37,24 @@ static int utimes_common(const struct path *path, struct timespec64 *times)
              if (times[0].tv_nsec == UTIME_OMIT)
                      newattrs.ia_valid &= ~ATTR_ATIME;
              else if (times[0].tv_nsec != UTIME_NOW) {
-                     newattrs.ia_atime.tv_sec = times[0].tv_sec;
-                     newattrs.ia_atime.tv_nsec = times[0].tv_nsec;
+                     newattrs.ia_atime.tv_sec =
+                             clamp(times[0].tv_sec, sb->s_time_min, sb->s_time_max);
+                     if (times[0].tv_sec == sb->s_time_max || times[0].tv_sec == sb->s_time_min)

This is testing the un-clamped value.

Quoted 4 lines

fs/utimes.c +3 -0

+                             newattrs.ia_atime.tv_nsec = 0;
+                     else
+                             newattrs.ia_atime.tv_nsec = times[0].tv_nsec;
                      newattrs.ia_valid |= ATTR_ATIME_SET;

fs/utimes.c static int utimes_common(const struct path *path, struct timespec64 *times) · line 54

Quoted 10 lines

fs/utimes.c +3 -2

              }
 
              if (times[1].tv_nsec == UTIME_OMIT)
                      newattrs.ia_valid &= ~ATTR_MTIME;
              else if (times[1].tv_nsec != UTIME_NOW) {
-                     newattrs.ia_mtime.tv_sec = times[1].tv_sec;
-                     newattrs.ia_mtime.tv_nsec = times[1].tv_nsec;
+                     newattrs.ia_mtime.tv_sec =
+                             clamp(times[1].tv_sec, sb->s_time_min, sb->s_time_max);
+                     if (times[1].tv_sec >= sb->s_time_max || times[1].tv_sec == sb->s_time_min)

Similarly here, for the minimum.

I suggest testing for clamping like this:

                        if (newattrs.ia_atime.tv_sec != times[0].tv_sec)
                                ...
                        if (newattrs.ia_mtime.tv_sec != times[1].tv_sec)
                                ...

Ben.

Quoted 5 lines

fs/utimes.c +3 -0

+                             newattrs.ia_mtime.tv_nsec = 0;
+                     else
+                             newattrs.ia_mtime.tv_nsec = times[1].tv_nsec;
                      newattrs.ia_valid |= ATTR_MTIME_SET;
              }

Fill in the appropriate limits to avoid inconsistencies in the vfs cached inode times when timestamps are outside the permitted range.

Even though some filesystems are read-only, fill in the timestamps to reflect the on-disk representation.

Signed-off-by Deepa Dinamani <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
 fs/befs/linuxvfs.c       | 2 ++
 fs/bfs/inode.c           | 2 ++
 fs/coda/inode.c          | 3 +++
 fs/cramfs/inode.c        | 2 ++
 fs/efs/super.c           | 2 ++
 fs/ext2/super.c          | 2 ++
 fs/freevxfs/vxfs_super.c | 2 ++
 fs/jffs2/fs.c            | 3 +++
 fs/jfs/super.c           | 2 ++
 fs/minix/inode.c         | 2 ++
 fs/qnx4/inode.c          | 2 ++
 fs/qnx6/inode.c          | 2 ++
 fs/reiserfs/super.c      | 3 +++
 fs/romfs/super.c         | 2 ++
 fs/squashfs/super.c      | 2 ++
 fs/ufs/super.c           | 7 +++++++
 fs/xfs/xfs_super.c       | 2 ++
 17 files changed, 42 insertions(+)

fs/befs/linuxvfs.c +2 -0

index 462d096ff3e9..64cdf4d8e424 100644
@@ -893,6 +893,8 @@ befs_fill_super(struct super_block *sb, void *data, int silent)
 	sb_set_blocksize(sb, (ulong) befs_sb->block_size);
 	sb->s_op = &befs_sops;
 	sb->s_export_op = &befs_export_operations;
+	sb->s_time_min = 0;
+	sb->s_time_max = 0xffffffffffffll;
 	root = befs_iget(sb, iaddr2blockno(sb, &(befs_sb->root_dir)));
 	if (IS_ERR(root)) {
 		ret = PTR_ERR(root);

fs/bfs/inode.c +2 -0

index 5e97bed073d7..f8ce1368218b 100644
@@ -324,6 +324,8 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent)
 		return -ENOMEM;
 	mutex_init(&info->bfs_lock);
 	s->s_fs_info = info;
+	s->s_time_min = 0;
+	s->s_time_max = U32_MAX;
 
 	sb_set_blocksize(s, BFS_BSIZE);
 

fs/coda/inode.c +3 -0

index 321f56e487cb..b1c70e2b9b1e 100644
@@ -188,6 +188,9 @@ static int coda_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_magic = CODA_SUPER_MAGIC;
 	sb->s_op = &coda_super_operations;
 	sb->s_d_op = &coda_dentry_operations;
+	sb->s_time_gran = 1;
+	sb->s_time_min = S64_MIN;
+	sb->s_time_max = S64_MAX;
 
 	error = super_setup_bdi(sb);
 	if (error)

fs/cramfs/inode.c +2 -0

index 9352487bd0fc..4d1d8b7761ed 100644
@@ -597,6 +597,8 @@ static int cramfs_finalize_super(struct super_block *sb,
 
 	/* Set it all up.. */
 	sb->s_flags |= SB_RDONLY;
+	sb->s_time_min = 0;
+	sb->s_time_max = 0;
 	sb->s_op = &cramfs_ops;
 	root = get_cramfs_inode(sb, cramfs_root, 0);
 	if (IS_ERR(root))

fs/efs/super.c +2 -0

index 867fc24dee20..4a6ebff2af76 100644
@@ -257,6 +257,8 @@ static int efs_fill_super(struct super_block *s, void *d, int silent)
 	if (!sb)
 		return -ENOMEM;
 	s->s_fs_info = sb;
+	s->s_time_min = 0;
+	s->s_time_max = U32_MAX;
 
 	s->s_magic		= EFS_SUPER_MAGIC;
 	if (!sb_set_blocksize(s, EFS_BLOCKSIZE)) {

fs/ext2/super.c +2 -0

index 44eb6e7eb492..baa36c6fb71e 100644
@@ -1002,6 +1002,8 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
 
 	sb->s_maxbytes = ext2_max_size(sb->s_blocksize_bits);
 	sb->s_max_links = EXT2_LINK_MAX;
+	sb->s_time_min = S32_MIN;
+	sb->s_time_max = S32_MAX;
 
 	if (le32_to_cpu(es->s_rev_level) == EXT2_GOOD_OLD_REV) {
 		sbi->s_inode_size = EXT2_GOOD_OLD_INODE_SIZE;

fs/freevxfs/vxfs_super.c +2 -0

index a89f68c3cbed..578a5062706e 100644
@@ -229,6 +229,8 @@ static int vxfs_fill_super(struct super_block *sbp, void *dp, int silent)
 
 	sbp->s_op = &vxfs_super_ops;
 	sbp->s_fs_info = infp;
+	sbp->s_time_min = 0;
+	sbp->s_time_max = U32_MAX;
 
 	if (!vxfs_try_sb_magic(sbp, silent, 1,
 			(__force __fs32)cpu_to_le32(VXFS_SUPER_MAGIC))) {

fs/jffs2/fs.c +3 -0

index 8a20ddd25f2d..d0b59d03a7a9 100644
@@ -590,6 +590,9 @@ int jffs2_do_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_blocksize = PAGE_SIZE;
 	sb->s_blocksize_bits = PAGE_SHIFT;
 	sb->s_magic = JFFS2_SUPER_MAGIC;
+	sb->s_time_min = 0;
+	sb->s_time_max = U32_MAX;
+
 	if (!sb_rdonly(sb))
 		jffs2_start_garbage_collect_thread(c);
 	return 0;

fs/jfs/super.c +2 -0

index f4e10cb9f734..b2dc4d1f9dcc 100644
@@ -503,6 +503,8 @@ static int jfs_fill_super(struct super_block *sb, void *data, int silent)
 
 	sb->s_fs_info = sbi;
 	sb->s_max_links = JFS_LINK_MAX;
+	sb->s_time_min = 0;
+	sb->s_time_max = U32_MAX;
 	sbi->sb = sb;
 	sbi->uid = INVALID_UID;
 	sbi->gid = INVALID_GID;

fs/minix/inode.c +2 -0

index f96073f25432..7cb5fd38eb14 100644
@@ -277,6 +277,8 @@ static int minix_fill_super(struct super_block *s, void *data, int silent)
 
 	/* set up enough so that it can read an inode */
 	s->s_op = &minix_sops;
+	s->s_time_min = 0;
+	s->s_time_max = U32_MAX;
 	root_inode = minix_iget(s, MINIX_ROOT_INO);
 	if (IS_ERR(root_inode)) {
 		ret = PTR_ERR(root_inode);

fs/qnx4/inode.c +2 -0

index 922d083bbc7c..e8da1cde87b9 100644
@@ -201,6 +201,8 @@ static int qnx4_fill_super(struct super_block *s, void *data, int silent)
 	s->s_op = &qnx4_sops;
 	s->s_magic = QNX4_SUPER_MAGIC;
 	s->s_flags |= SB_RDONLY;	/* Yup, read-only yet */
+	s->s_time_min = 0;
+	s->s_time_max = U32_MAX;
 
 	/* Check the superblock signature. Since the qnx4 code is
 	   dangerous, we should leave as quickly as possible

fs/qnx6/inode.c +2 -0

index 0f8b0ff1ba43..345db56c98fd 100644
@@ -429,6 +429,8 @@ static int qnx6_fill_super(struct super_block *s, void *data, int silent)
 	s->s_op = &qnx6_sops;
 	s->s_magic = QNX6_SUPER_MAGIC;
 	s->s_flags |= SB_RDONLY;        /* Yup, read-only yet */
+	s->s_time_min = 0;
+	s->s_time_max = U32_MAX;
 
 	/* ease the later tree level calculations */
 	sbi = QNX6_SB(s);

fs/reiserfs/super.c +3 -0

index ab028ea0e561..d69b4ac0ae2f 100644
@@ -1976,6 +1976,9 @@ static int reiserfs_fill_super(struct super_block *s, void *data, int silent)
 		goto error_unlocked;
 	}
 
+	s->s_time_min = 0;
+	s->s_time_max = U32_MAX;
+
 	rs = SB_DISK_SUPER_BLOCK(s);
 	/*
 	 * Let's do basic sanity check to verify that underlying device is not

fs/romfs/super.c +2 -0

index 7d580f7c3f1d..a42c0e3079dc 100644
@@ -478,6 +478,8 @@ static int romfs_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_maxbytes = 0xFFFFFFFF;
 	sb->s_magic = ROMFS_MAGIC;
 	sb->s_flags |= SB_RDONLY | SB_NOATIME;
+	sb->s_time_min = 0;
+	sb->s_time_max = 0;
 	sb->s_op = &romfs_super_ops;
 
 #ifdef CONFIG_ROMFS_ON_MTD

fs/squashfs/super.c +2 -0

index effa638d6d85..a9e9837617a9 100644
@@ -183,6 +183,8 @@ static int squashfs_fill_super(struct super_block *sb, void *data, int silent)
 		(u64) le64_to_cpu(sblk->id_table_start));
 
 	sb->s_maxbytes = MAX_LFS_FILESIZE;
+	sb->s_time_min = 0;
+	sb->s_time_max = U32_MAX;
 	sb->s_flags |= SB_RDONLY;
 	sb->s_op = &squashfs_super_ops;
 

fs/ufs/super.c +7 -0

index 4ed0dca52ec8..1da0be667409 100644
@@ -843,6 +843,10 @@ static int ufs_fill_super(struct super_block *sb, void *data, int silent)
 
 	sb->s_maxbytes = MAX_LFS_FILESIZE;
 
+	sb->s_time_gran = NSEC_PER_SEC;
+	sb->s_time_min = S32_MIN;
+	sb->s_time_max = S32_MAX;
+
 	switch (sbi->s_mount_opt & UFS_MOUNT_UFSTYPE) {
 	case UFS_MOUNT_UFSTYPE_44BSD:
 		UFSD("ufstype=44bsd\n");

@@ -861,6 +865,9 @@ static int ufs_fill_super(struct super_block *sb, void *data, int silent)
 		uspi->s_fshift = 9;
 		uspi->s_sbsize = super_block_size = 1536;
 		uspi->s_sbbase =  0;
+		sb->s_time_gran = 1;
+		sb->s_time_min = S64_MIN;
+		sb->s_time_max = S64_MAX;
 		flags |= UFS_TYPE_UFS2 | UFS_DE_44BSD | UFS_UID_44BSD | UFS_ST_44BSD | UFS_CG_44BSD;
 		break;
 

fs/xfs/xfs_super.c +0 -0

index a14d11d78bd8..1a0daf46bae8 100644

fs/xfs/xfs_super.c +0 -0

 	set_posix_acl_flag(sb);
 
 	/* version 5 superblocks support inode version counters. */

Signature · 1 line

2.17.1

ext4 has different overflow limits for max filesystem timestamps based on the extra bytes available.

The timestamp limits are calculated according to the encoding table in
a4dad1ae24f85i(ext4: Fix handling of extended tv_sec):

* extra  msb of                         adjust for signed
* epoch  32-bit                         32-bit tv_sec to
* bits   time    decoded 64-bit tv_sec  64-bit tv_sec      valid time range
* 0 0    1    -0x80000000..-0x00000001  0x000000000   1901-12-13..1969-12-31
* 0 0    0    0x000000000..0x07fffffff  0x000000000   1970-01-01..2038-01-19
* 0 1    1    0x080000000..0x0ffffffff  0x100000000   2038-01-19..2106-02-07
* 0 1    0    0x100000000..0x17fffffff  0x100000000   2106-02-07..2174-02-25
* 1 0    1    0x180000000..0x1ffffffff  0x200000000   2174-02-25..2242-03-16
* 1 0    0    0x200000000..0x27fffffff  0x200000000   2242-03-16..2310-04-04
* 1 1    1    0x280000000..0x2ffffffff  0x300000000   2310-04-04..2378-04-22
* 1 1    0    0x300000000..0x37fffffff  0x300000000   2378-04-22..2446-05-10

Note that the time limits are not correct for deletion times.

Signed-off-by Deepa Dinamani <[email protected]>

Reviewed-by Andreas Dilger <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
 fs/ext4/ext4.h  |  4 ++++
 fs/ext4/super.c | 17 +++++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

fs/ext4/ext4.h +4 -0

index 1cb67859e051..3f13cf12ae7f 100644
@@ -1631,6 +1631,10 @@ static inline void ext4_clear_state_flags(struct ext4_inode_info *ei)
 
 #define EXT4_GOOD_OLD_INODE_SIZE 128
 
+#define EXT4_EXTRA_TIMESTAMP_MAX	(((s64)1 << 34) - 1  + S32_MIN)
+#define EXT4_NON_EXTRA_TIMESTAMP_MAX	S32_MAX
+#define EXT4_TIMESTAMP_MIN		S32_MIN
+
 /*
  * Feature set definitions
  */

fs/ext4/super.c +15 -2

index 4079605d437a..3ea2d60f33aa 100644
@@ -4035,8 +4035,21 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 			       sbi->s_inode_size);
 			goto failed_mount;
 		}
-		if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE)
-			sb->s_time_gran = 1 << (EXT4_EPOCH_BITS - 2);
+		/*
+		 * i_atime_extra is the last extra field available for [acm]times in
+		 * struct ext4_inode. Checking for that field should suffice to ensure
+		 * we have extra space for all three.
+		 */
+		if (sbi->s_inode_size >= offsetof(struct ext4_inode, i_atime_extra) +
+			sizeof(((struct ext4_inode *)0)->i_atime_extra)) {
+			sb->s_time_gran = 1;
+			sb->s_time_max = EXT4_EXTRA_TIMESTAMP_MAX;
+		} else {
+			sb->s_time_gran = NSEC_PER_SEC;
+			sb->s_time_max = EXT4_NON_EXTRA_TIMESTAMP_MAX;
+		}
+
+		sb->s_time_min = EXT4_TIMESTAMP_MIN;
 	}
 
 	sbi->s_desc_size = le16_to_cpu(es->s_desc_size);

Signature · 1 line

2.17.1

Quoted 7 lines · Deepa Dinamani

ext4 has different overflow limits for max filesystem timestamps based on the extra bytes available.

The timestamp limits are calculated according to the encoding table in
a4dad1ae24f85i(ext4: Fix handling of extended tv_sec):

Quoted 11 lines

* extra  msb of                         adjust for signed
* epoch  32-bit                         32-bit tv_sec to
* bits   time    decoded 64-bit tv_sec  64-bit tv_sec      valid time range
* 0 0    1    -0x80000000..-0x00000001  0x000000000   1901-12-13..1969-12-31
* 0 0    0    0x000000000..0x07fffffff  0x000000000   1970-01-01..2038-01-19
* 0 1    1    0x080000000..0x0ffffffff  0x100000000   2038-01-19..2106-02-07
* 0 1    0    0x100000000..0x17fffffff  0x100000000   2106-02-07..2174-02-25
* 1 0    1    0x180000000..0x1ffffffff  0x200000000   2174-02-25..2242-03-16
* 1 0    0    0x200000000..0x27fffffff  0x200000000   2242-03-16..2310-04-04
* 1 1    1    0x280000000..0x2ffffffff  0x300000000   2310-04-04..2378-04-22
* 1 1    0    0x300000000..0x37fffffff  0x300000000   2378-04-22..2446-05-10

My recollection of ext4 has gotten rusty, so this could be a bogus question:

Say you have a filesystem with s_inode_size > 128 where not all of the ondisk inodes have been upgraded to i_extra_isize > 0 and therefore don't support nanoseconds or times beyond 2038. I think this happens on ext3 filesystems that reserved extra space for inode attrs that are subsequently converted to ext4?

In any case, that means that you have some inodes that support 34-bit tv_sec and some inodes that only support 32-bit tv_sec. For the inodes with 32-bit tv_sec, I think all that happens is that a large timestamp will be truncated further, right?

And no mount time warning because at least /some/ of the inodes are ready to go for the next 30 years?

Quoted 5 lines

Say you have a filesystem with s_inode_size > 128 where not all of the ondisk inodes have been upgraded to i_extra_isize > 0 and therefore don't support nanoseconds or times beyond 2038. I think this happens on ext3 filesystems that reserved extra space for inode attrs that are subsequently converted to ext4?

I'm confused about ext3 being converted to ext4. If the converted inodes have extra space, then ext4_iget() will start using the extra space when it modifies the on disk inode, won't it?i

Quoted 9 lines · Deepa Dinamani

Quoted 5 lines

Say you have a filesystem with s_inode_size > 128 where not all of the ondisk inodes have been upgraded to i_extra_isize > 0 and therefore don't support nanoseconds or times beyond 2038. I think this happens on ext3 filesystems that reserved extra space for inode attrs that are subsequently converted to ext4?

I'm confused about ext3 being converted to ext4. If the converted inodes have extra space, then ext4_iget() will start using the extra space when it modifies the on disk inode, won't it?i

It is possible that you can have an ext3 file system with (for example) 256 byte inodes, and all of the extra space was used for extended attributes, then ext4 won't have the extra space available. This is going toh be on an inode-by-inode basis, and if an extended attribute is motdified or deleted, the space would become available,t and then inode would start getting a higher resolution timestamp.

I really don't think it's worth worrying about that, though. It's highly unlikely ext3 file systems will be still be in service by the time it's needed in 2038. And if so, it's highly unlikely they would be converted to ext4.

Is it correct to assume that this kind of file would have to be created using the ext3.ko file system implementation that was removed in linux-4.3, but not usiing ext2.ko or ext4.ko (which would always set the extended timestamps even in "-t ext2" or "-t ext3" mode)?

If we check for s_min_extra_isize instead of s_inode_size to determine s_time_gran/s_time_max, we would warn at mount time as well as and consistently truncate all timestamps to full 32-bit seconds, regardless of whether there is actually space or not.

Alternatively, we could warn if s_min_extra_isize is too small, but use i_inode_size to determine s_time_gran/s_time_max anyway.

Quoted 5 lines · Arnd Bergmann

Is it correct to assume that this kind of file would have to be created using the ext3.ko file system implementation that was removed in linux-4.3, but not usiing ext2.ko or ext4.ko (which would always set the extended timestamps even in "-t ext2" or "-t ext3" mode)?

Correct. Some of the enterprise distro's were using ext4 to support "mount -t ext3" even before 4.3. There's a CONFIG option to enable using ext4 for ext2 or ext3 if they aren't enabled.

Quoted 9 lines

If we check for s_min_extra_isize instead of s_inode_size to determine s_time_gran/s_time_max, we would warn at mount time as well as and consistently truncate all timestamps to full 32-bit seconds, regardless of whether there is actually space or not.

Alternatively, we could warn if s_min_extra_isize is too small, but use i_inode_size to determine s_time_gran/s_time_max anyway.

Even with ext4, s_min_extra_isize doesn't guarantee that will be able to expand the inode. This can fail if (a) we aren't able to expand existing the transaction handle because there isn't enough space in the journal, or (b) there is already an external xattr block which is also full, so there is no space to evacuate an extended attribute out of the inode's extra space.

We could be more aggressive by trying to expand make room in the inode in ext4_iget (when we're reading in the inode, assuming the file system isn't mounted read/only), instead of in the middle of mark_inode_dirty(). That will eliminate failure mode (a) --- which is statistically rare --- but it won't eliminate failure mode (b).

Ultimately, the question is which is worse: having a timestamp be wrong, or randomly dropping an xattr from the inode to make room for the extended timestamp. We've come down on it being less harmful to have the timestamp be wrong.

But again, this is a pretty rare case. I'm not convinced it's worth stressing about, since it's going to require multiple things to go wrong before a timestamp will be bad.

I must have misunderstood what the field says. I expected that with s_min_extra_isize set beyond the nanosecond fields, there would be a guarantee that all inodes have at least as many extra bytes already allocated. What circumstances would lead to an i_extra_isize smaller than s_min_extra_isize?

Quoted 6 lines · Arnd Bergmann

I must have misunderstood what the field says. I expected that with s_min_extra_isize set beyond the nanosecond fields, there would be a guarantee that all inodes have at least as many extra bytes already allocated. What circumstances would lead to an i_extra_isize smaller than s_min_extra_isize?

When allocating new inodes, i_extra_isize is set to s_want_extra_isize. When modifying existing inodes, if i_extra_isize is less than s_min_extra_isize, then we will attempt to move out extended attribute(s) to the external xattr block. So the s_min_extra_isize field is not a guarantee, but rather an aspirationa goal. The idea is that at some point when we want to enable a new feature, which needs more extra inode space, we can adjust s_min_extra_size and s_want_extra_size, and the file system will migrate things to meet these constraints.

Since the extended timestamps were one of the first extra inode fields to be added, I strongly suggest that we not try to borrow trouble. Solving the general case problem is *hard*.

I see in the ext4 code that we always try to expand i_extra_size to s_want_extra_isize in ext4_mark_inode_dirty(), and that s_want_extra_isize is always at least s_min_extra_isize, so we constantly try to expand the inode to fit.

Did older versions of ext4 or ext3 ignore s_min_extra_isize when creating inodes despite EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE,
or is there another possibility I'm missing?

Quoted 5 lines · Arnd Bergmann

I see in the ext4 code that we always try to expand i_extra_size to s_want_extra_isize in ext4_mark_inode_dirty(), and that s_want_extra_isize is always at least s_min_extra_isize, so we constantly try to expand the inode to fit.

Yes, we *try*. But we may not succeed. There may actually be a problem here if the cause is due to there simply is no space in the external xattr block, so we might try and try every time we try to modify that inode, and it would be a performance mess. If it's due to there being no room in the current transaction, then it's highly likely it will succeed the next time.

Quoted 4 lines

Did older versions of ext4 or ext3 ignore s_min_extra_isize when creating inodes despite EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE,
or is there another possibility I'm missing?

s_min_extra_isize could get changed in order to make room for some new file system feature --- such as extended timestamps.

If you want to pretend that file systems never get upgraded, then life is much simpler. The general approach is that for less-sophisticated customers (e.g., most people running enterprise distros) file system upgrades are not a thing. But for sophisticated users, we do try to make thing work for people who are aware of the risks / caveats / rough edges. Google won't have been able to upgrade thousands and thousands of servers in data centers all over the world if we limited ourselves to Red Hat's support restrictions. Backup / reformat / restore really isn't a practical rollout strategy for many exabytes of file systems.

It sounds like your safety checks / warnings are mostly targeted at low-information customers, no?

Quoted 15 lines · Theodore Y. Ts'o

Quoted 5 lines · Arnd Bergmann

I see in the ext4 code that we always try to expand i_extra_size to s_want_extra_isize in ext4_mark_inode_dirty(), and that s_want_extra_isize is always at least s_min_extra_isize, so we constantly try to expand the inode to fit.

Yes, we *try*. But we may not succeed. There may actually be a problem here if the cause is due to there simply is no space in the external xattr block, so we might try and try every time we try to modify that inode, and it would be a performance mess. If it's due to there being no room in the current transaction, then it's highly likely it will succeed the next time.

Quoted 7 lines

Quoted 4 lines

Did older versions of ext4 or ext3 ignore s_min_extra_isize when creating inodes despite EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE,
or is there another possibility I'm missing?

s_min_extra_isize could get changed in order to make room for some new file system feature --- such as extended timestamps.

Ok, that explains it. I assumed s_min_extra_isize was meant to not be modifiable, and did not find a way to change it using the kernel or tune2fs, but now I can see that debugfs can set it.

Quoted 11 lines

If you want to pretend that file systems never get upgraded, then life is much simpler. The general approach is that for less-sophisticated customers (e.g., most people running enterprise distros) file system upgrades are not a thing. But for sophisticated users, we do try to make thing work for people who are aware of the risks / caveats / rough edges. Google won't have been able to upgrade thousands and thousands of servers in data centers all over the world if we limited ourselves to Red Hat's support restrictions. Backup / reformat / restore really isn't a practical rollout strategy for many exabytes of file systems.

Quoted 2 lines

It sounds like your safety checks / warnings are mostly targeted at low-information customers, no?

Yes, that seems like a reasonable compromise: just warn based on s_min_extra_isize, and assume that anyone who used debugfs to set s_min_extra_isize to a higher value from an ext3 file system during the migration to ext4 was aware of the risks already.

That leaves the question of what we should set the s_time_gran and s_time_max to on a superblock with s_min_extra_isize<16 and s_want_extra_isize>=16.

If we base it on s_min_extra_isize, we never try to set a timestamp later than 2038 and so will never fail, but anyone with a grandfathered s_min_extra_isize from ext3 won't be able to set extended timestamps on any files any more. Based on s_want_extra_isize we would keep the current behavior, but could add a custom warning in the ext4 code about the small s_min_extra_isize indicating a theoretical problem.

Rather than printing a warning at mount time (which may be confusing to users for a problem they may never see), it makes sense to only print such a warning in the vanishingly small case that someone actually tries to modify the inode timestamp but it doesn't fit, rather than on the theoretical case that may never happen.

fs/fat/inode.c +6 -0

+/* DOS dates from 1980/1/1 through 2107/12/31 */
+#define FAT_DATE_MIN (0<<9 | 1<<5 | 1)
+#define FAT_DATE_MAX (127<<9 | 12<<5 | 31)
+#define FAT_TIME_MAX (23<<11 | 59<<5 | 29)
+
 /*
  * A deserialized copy of the on-disk structure laid out in struct
  * fat_boot_sector.

@@ -1605,6 +1610,7 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
 	int debug;
 	long error;
 	char buf[50];
+	struct timespec64 ts;
 
 	/*
 	 * GFP_KERNEL is ok here, because while we do hold the

Quoted 14 lines · Deepa Dinamani

fs/fat/inode.c +6 -0

+/* DOS dates from 1980/1/1 through 2107/12/31 */
+#define FAT_DATE_MIN (0<<9 | 1<<5 | 1)
+#define FAT_DATE_MAX (127<<9 | 12<<5 | 31)
+#define FAT_TIME_MAX (23<<11 | 59<<5 | 29)
+
 /*
  * A deserialized copy of the on-disk structure laid out in struct
  * fat_boot_sector.

@@ -1605,6 +1610,7 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
      int debug;
      long error;
      char buf[50];
+     struct timespec64 ts;
 

fs/fat/inode.c int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat, · line 1712

Quoted 12 lines

fs/fat/inode.c +6 -0

      /*
       * GFP_KERNEL is ok here, because while we do hold the

@@ -1698,6 +1704,12 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
      sbi->free_clus_valid = 0;
      sbi->prev_free = FAT_START_ENT;
      sb->s_maxbytes = 0xffffffff;
+     fat_time_fat2unix(sbi, &ts, 0, cpu_to_le16(FAT_DATE_MIN), 0);
+     sb->s_time_min = ts.tv_sec;
+
+     fat_time_fat2unix(sbi, &ts, cpu_to_le16(FAT_TIME_MAX),
+                       cpu_to_le16(FAT_DATE_MAX), 0);
+     sb->s_time_max = ts.tv_sec;

At least, it is wrong to call fat_time_fat2unix() before setup parameters in sbi.

Quoted 2 lines

At least, it is wrong to call fat_time_fat2unix() before setup parameters in sbi.

All the parameters that fat_time_fat2unix() cares in sbi is accessed through

static inline int fat_tz_offset(struct msdos_sb_info *sbi)

{
    return (sbi->options.tz_set ?
           -sbi->options.time_offset :
           sys_tz.tz_minuteswest) * SECS_PER_MIN;
}

Both the sbi fields sbi->options.tz_set and sbi->options.time_offset are set by the call to parse_options(). And, parse_options() is called before the calls to fat_time_fat2unix().:

int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
void (*setup)(struct super_block *))

{
     <snip>

    error = parse_options(sb, data, isvfat, silent, &debug, &sbi->options);
    if (error)
        goto out_fail;

   <snip>

    sbi->prev_free = FAT_START_ENT;
    sb->s_maxbytes = 0xffffffff;
    fat_time_fat2unix(sbi, &ts, 0, cpu_to_le16(FAT_DATE_MIN), 0);
    sb->s_time_min = ts.tv_sec;

    fat_time_fat2unix(sbi, &ts, cpu_to_le16(FAT_TIME_MAX),
              cpu_to_le16(FAT_DATE_MAX), 0);
    sb->s_time_max = ts.tv_sec;

   <snip>
}

I do not see what the problem is.

Fill in the appropriate limits to avoid inconsistencies in the vfs cached inode times when timestamps are outside the permitted range.

Also fix timestamp calculation to avoid overflow while converting from days to seconds.

Signed-off-by Deepa Dinamani <[email protected]>

Cc [email protected]

Fill in the appropriate limits to avoid inconsistencies in the vfs cached inode times when timestamps are outside the permitted range.

Signed-off-by Deepa Dinamani <[email protected]>

Cc [email protected]

Cc [email protected]

---
 fs/omfs/inode.c | 4 ++++
 1 file changed, 4 insertions(+)

fs/omfs/inode.c +0 -0

index 08226a835ec3..b76ec6b88ded 100644

Also update the gran since pstore has microsecond granularity.

Signed-off-by Deepa Dinamani <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
 fs/pstore/inode.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/pstore/inode.c +3 -1

index 89a80b568a17..ee752f9fda57 100644
@@ -388,7 +388,9 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_blocksize_bits	= PAGE_SHIFT;
 	sb->s_magic		= PSTOREFS_MAGIC;
 	sb->s_op		= &pstore_ops;
-	sb->s_time_gran		= 1;
+	sb->s_time_gran         = NSEC_PER_USEC;
+	sb->s_time_min		= S64_MIN;
+	sb->s_time_max		= S64_MAX;
 
 	parse_options(data);
 

Signature · 1 line

2.17.1

Quoted 1 line · Deepa Dinamani

Also update the gran since pstore has microsecond granularity.

So, I'm fine with this, but technically the granularity depends on the backend storage... many have no actual time keeping, though. My point is, pstore's timestamps are really mostly a lie, but the most common backend (ramoops) is seconds-granularity.

So, I'm fine with this, but it's a lie but it's a lie that doesn't matter, so ...

Acked-by Kees Cook <[email protected]>

I'm open to suggestions to improve it...

Quoted 12 lines · Kees Cook

Quoted 1 line · Deepa Dinamani

Also update the gran since pstore has microsecond granularity.

So, I'm fine with this, but technically the granularity depends on the backend storage... many have no actual time keeping, though. My point is, pstore's timestamps are really mostly a lie, but the most common backend (ramoops) is seconds-granularity.

So, I'm fine with this, but it's a lie but it's a lie that doesn't matter, so ...

Quoted 3 lines

Acked-by Kees Cook <[email protected]>

I'm open to suggestions to improve it...

If we don't care about using sub-second granularity, then setting it to one second unconditionally here will make it always use that and report it correctly.

Quoted 14 lines · Arnd Bergmann

Quoted 12 lines · Kees Cook

Quoted 1 line · Deepa Dinamani

Also update the gran since pstore has microsecond granularity.

So, I'm fine with this, but technically the granularity depends on the backend storage... many have no actual time keeping, though. My point is, pstore's timestamps are really mostly a lie, but the most common backend (ramoops) is seconds-granularity.

So, I'm fine with this, but it's a lie but it's a lie that doesn't matter, so ...

Quoted 7 lines

Quoted 3 lines

Acked-by Kees Cook <[email protected]>

I'm open to suggestions to improve it...

If we don't care about using sub-second granularity, then setting it to one second unconditionally here will make it always use that and report it correctly.

Should this printf in ramoops_write_kmsg_hdr() also be fixed then?

        RAMOOPS_KERNMSG_HDR "%lld.%06lu-%c\n",
        (time64_t)record->time.tv_sec,
        record->time.tv_nsec / 1000,
        record->compressed ? 'C' : 'D');
    persistent_ram_write(prz, hdr, len);

ramoops_read_kmsg_hdr() doesn't read this as microseconds. Seems like a mismatch from above.

If we want to agree that we just want seconds granularity for pstore, we could replace the tv_nsec part to be all 0's if anybody else is depending on this format.
I could drop this patch from the series and post that patch seperately.

Quoted 26 lines · Deepa Dinamani

Quoted 14 lines · Arnd Bergmann

Quoted 12 lines · Kees Cook

Quoted 1 line · Deepa Dinamani

Also update the gran since pstore has microsecond granularity.

So, I'm fine with this, but technically the granularity depends on the backend storage... many have no actual time keeping, though. My point is, pstore's timestamps are really mostly a lie, but the most common backend (ramoops) is seconds-granularity.

So, I'm fine with this, but it's a lie but it's a lie that doesn't matter, so ...

Quoted 7 lines

Quoted 3 lines

Acked-by Kees Cook <[email protected]>

I'm open to suggestions to improve it...

If we don't care about using sub-second granularity, then setting it to one second unconditionally here will make it always use that and report it correctly.

Should this printf in ramoops_write_kmsg_hdr() also be fixed then?

Quoted 8 lines

        RAMOOPS_KERNMSG_HDR "%lld.%06lu-%c\n",
        (time64_t)record->time.tv_sec,
        record->time.tv_nsec / 1000,
        record->compressed ? 'C' : 'D');
    persistent_ram_write(prz, hdr, len);

ramoops_read_kmsg_hdr() doesn't read this as microseconds. Seems like a mismatch from above.

Good catch. This seems to go back to commit 3f8f80f0cfeb ("pstore/ram:
Read and write to the 'compressed' flag of pstore"), which introduced the nanosecond read. The write function however has always used microseconds, and that was kept when the implementation changed from timeval to timespec in commit 1e817fb62cd1 ("time: create __getnstimeofday for WARNless calls").

Quoted 4 lines

If we want to agree that we just want seconds granularity for pstore, we could replace the tv_nsec part to be all 0's if anybody else is depending on this format.
I could drop this patch from the series and post that patch seperately.

We should definitely fix it to not produce a bogus nanosecond value. Whether using full seconds or microsecond resolution is better here, I don't know. It seems that pstore records generally get created with a nanosecond nanosecond accurate timestamp from ktime_get_real_fast_ns() and then truncated to the resolution of the backend, rather than the normal jiffies-accurate inode timestamps that we have for regular file systems.

This might mean that we do want the highest possible resolution and not further truncate here, in case that information ends up being useful afterwards.