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. -- Dave Chinner david@xxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
