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

[Arnd Bergmann <arnd@arndb.de>]

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.

> > --- 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.

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.

	Arnd