On Mon, Aug 5, 2019 at 6:30 AM Ben Hutchings <ben.hutchings@xxxxxxxxxxxxxxx> wrote: > > On Mon, 2019-07-29 at 18:49 -0700, Deepa Dinamani wrote: > > 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 <deepa.kernel@xxxxxxxxx> > > --- > > fs/utimes.c | 17 +++++++++++++---- > > 1 file changed, 13 insertions(+), 4 deletions(-) > > > > diff --git a/fs/utimes.c b/fs/utimes.c > > index 350c9c16ace1..4c1a2ce90bbc 100644 > > --- a/fs/utimes.c > > +++ b/fs/utimes.c > > @@ -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) > > This is testing the un-clamped value. > > > + newattrs.ia_atime.tv_nsec = 0; > > + else > > + newattrs.ia_atime.tv_nsec = times[0].tv_nsec; > > newattrs.ia_valid |= ATTR_ATIME_SET; > > } > > > > 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. > > > + newattrs.ia_mtime.tv_nsec = 0; > > + else > > + newattrs.ia_mtime.tv_nsec = times[1].tv_nsec; > > newattrs.ia_valid |= ATTR_MTIME_SET; > > } Darrick pointed out that maybe we could use timestamp_truncate() here to clamp. I think it is ok to truncate to the right granularity also here. setattr callbacks do it already. So the diff here looks like below: - 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_atime = timestamp_truncate(times[0], inode); Thanks, Deepa
