On Thu, Mar 03, 2016 at 12:07:42AM +0100, Arnd Bergmann wrote:
On Thursday 03 March 2016 09:19:05 Dave Chinner wrote:
On Wed, Mar 02, 2016 at 07:51:34AM -0800, Deepa Dinamani wrote:
MAX_INVALID_VFS_TIME and MIN_INVALID_VFS_TIME are initialized to S64_MIN and S64_MAX respectively so that even if one of the fields is uninitialized, it can be detected by using the condition max_time < min_time.
I can't work out what MIN/MAX_INVALID_VFS_TIME is supposed to mean when I see it in the code. does it mean time that lies between MIN_INVALID_VFS_TIME > time > MAX_INVALID_VFS_TIME is invalid (unlikely, but that's the obvious reading :)?
Or that time < MIN_INVALID_VFS_TIME is invalid? Or is it valid? I can't tell...
Normally limits are specified by "min valid" and "max valid" defines, which are pretty clear in their meaning. Like:
Hmm, I had meant to comment on this in my private discussion.
I agree this needs some more explanation in the source code, the idea is that once we have modified all file systems to correctly set the actual limits, we can then detect any newly added file system that forgets to set them, so we don't get accidental incorrect limits.
But if a superblock supports full 64 bit time resolution (i.e. MIN_VFS_TIME to MAX_VFS_TIME) then you can't tell the difference and will warn inappropriately.
--- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -927,6 +927,12 @@ static inline struct file *get_file(struct file *f) #define MAX_LFS_FILESIZE ((loff_t)0x7fffffffffffffffLL) #endif +#define MAX_VFS_TIME S64_MAX +#define MIN_VFS_TIME S64_MIN
These. Anything ouside these ranges is invalid.
As such, I think this is wrong for 32 bit systems as the min/max VFS times right now are S32_MAX/S32_MIN...
I think the file system should always set the actual on-disk format limits in the superblock, regardless of the word size of the CPU architecture.
Eventually, yes. But right now, 32 bit systems are limited by the platform architecture. Hence no matter what the filesystem on-disk format supports, these are going to hard limits until filesystems start modifying the defaults to indicate what their on-disk format supports.
i.e. 32 bit systems should default to 32 bit time limits until the filesystem developers come along and verify that conversion from their on-disk format to 64 bit time works correctly. Then they set their real limits which may (or may not) be >y2038 compatible, and this means that filesystems that have not been converted to >y2038 compatible do not need any modifications to behave correctly on 32 bit systems...
We already support 32-bit time_t in compat tasks on 64-bit architectures, and we will support 64-bit time_t in normal tasks on 32-bit architectures in the future, and there is no need to range-check the timestamps written to disk.
The one range-check that we probably want for current 32-bit tasks is in the vfs_fstatat/vfs_getattr code, so a kernel that internally handles post-2038 timestamps can limit them to S32_MAX instead of wrapping.
Those should return EOVERFLOW rather than an incorrect timestamp, like we do for other 64 bit fields stat returns that can't fit in 32 bit stat fields. That's a clear indication of a process that should be using stat64() instead.
Cheers,
Dave.