Re: [PATCH] vfs: Add support to check max and min inode times

[Dave Chinner <david@fromorbit.com>]

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@fromorbit.com