Re: [Y2038] [PATCH 05/20] utimes: Clamp the timestamps before update
On Mon, Aug 5, 2019 at 6:30 AM Ben Hutchings
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
> > ---
> > 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
Re: [Y2038] [PATCH 05/20] utimes: Clamp the timestamps before update
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
> ---
> 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;
> }
> /*
--
Ben Hutchings, Software Developer Codethink Ltd
https://www.codethink.co.uk/ Dale House, 35 Dale Street
Manchester, M1 2HF, United Kingdom
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH 05/20] utimes: Clamp the timestamps before update
On Wed, Jul 31, 2019 at 8:15 AM Darrick J. Wong wrote:
>
> On Mon, Jul 29, 2019 at 06:49:09PM -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
> > ---
> > 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)
> > + 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)
>
> Line length.
>
> Also, didn't you just introduce a function to clamp tv_sec and fix
> granularity? Why not just use it here? I think this is the third time
> I've seen this open-coded logic.
Yes, we can use that now. Earlier we were not setting the tv_nsec to 0
in timestamp_truncate() which is why this was opencoded here.
I will make the change to include this.
Thanks,
Deepa
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH 05/20] utimes: Clamp the timestamps before update
On Mon, Jul 29, 2019 at 06:49:09PM -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
> ---
> 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)
> + 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)
Line length.
Also, didn't you just introduce a function to clamp tv_sec and fix
granularity? Why not just use it here? I think this is the third time
I've seen this open-coded logic.
--D
> + newattrs.ia_mtime.tv_nsec = 0;
> + else
> + newattrs.ia_mtime.tv_nsec = times[1].tv_nsec;
> newattrs.ia_valid |= ATTR_MTIME_SET;
> }
> /*
> --
> 2.17.1
>
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
[Y2038] [PATCH 05/20] utimes: Clamp the timestamps before update
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
---
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)
+ 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)
+ newattrs.ia_mtime.tv_nsec = 0;
+ else
+ newattrs.ia_mtime.tv_nsec = times[1].tv_nsec;
newattrs.ia_valid |= ATTR_MTIME_SET;
}
/*
--
2.17.1
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
