| Message ID | 20191111073000.2957-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ovl: fix timestamp limits | expand |
Thanks for the fix.
Acked-by: Deepa Dinamani <deepa.kernel@gmail.com>
On Mon, Nov 11, 2019 at 8:30 AM Amir Goldstein <amir73il@gmail.com> wrote: > > Overlayfs timestamp overflow limits should be inherrited from upper > filesystem. > > The current behavior, when overlayfs is over an underlying filesystem > that does not support post 2038 timestamps (e.g. xfs), is that overlayfs > overflows post 2038 timestamps instead of clamping them. How? Isn't the clamping supposed to happen in the underlying filesystem anyway? Thanks, Miklos
On Tue, Nov 12, 2019 at 5:48 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Mon, Nov 11, 2019 at 8:30 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > Overlayfs timestamp overflow limits should be inherrited from upper > > filesystem. > > > > The current behavior, when overlayfs is over an underlying filesystem > > that does not support post 2038 timestamps (e.g. xfs), is that overlayfs > > overflows post 2038 timestamps instead of clamping them. > > How? Isn't the clamping supposed to happen in the underlying filesystem anyway? > Not sure if it is supposed to be it doesn't. It happens in do_utimes() -> utimes_common() clamping seems to happen when user sets the times, so setting the overlay limits to those of upper fs seems correct anyway. Thanks, Amir.
On Tue, Nov 12, 2019 at 8:06 AM Amir Goldstein <amir73il@gmail.com> wrote: > > On Tue, Nov 12, 2019 at 5:48 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > On Mon, Nov 11, 2019 at 8:30 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > Overlayfs timestamp overflow limits should be inherrited from upper > > > filesystem. > > > > > > The current behavior, when overlayfs is over an underlying filesystem > > > that does not support post 2038 timestamps (e.g. xfs), is that overlayfs > > > overflows post 2038 timestamps instead of clamping them. > > > > How? Isn't the clamping supposed to happen in the underlying filesystem anyway? > > > > Not sure if it is supposed to be it doesn't. > It happens in do_utimes() -> utimes_common() Clamping also happens as part of current_time(). If this is called on an inode belonging to the upper fs, then the timestamps are clamped to those limits. -Deepa
On Tue, Nov 12, 2019 at 6:45 PM Deepa Dinamani <deepa.kernel@gmail.com> wrote: > > On Tue, Nov 12, 2019 at 8:06 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > On Tue, Nov 12, 2019 at 5:48 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > > > On Mon, Nov 11, 2019 at 8:30 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > > > Overlayfs timestamp overflow limits should be inherrited from upper > > > > filesystem. > > > > > > > > The current behavior, when overlayfs is over an underlying filesystem > > > > that does not support post 2038 timestamps (e.g. xfs), is that overlayfs > > > > overflows post 2038 timestamps instead of clamping them. > > > > > > How? Isn't the clamping supposed to happen in the underlying filesystem anyway? > > > > > > > Not sure if it is supposed to be it doesn't. > > It happens in do_utimes() -> utimes_common() > > Clamping also happens as part of current_time(). If this is called on > an inode belonging to the upper fs, then the timestamps are clamped to > those limits. > OK, but from utimes syscall they do not get clamped inside filesystem only in syscall itself. Right? Thanks, Amir.
On Tue, Nov 12, 2019 at 8:49 AM Amir Goldstein <amir73il@gmail.com> wrote: > > On Tue, Nov 12, 2019 at 6:45 PM Deepa Dinamani <deepa.kernel@gmail.com> wrote: > > > > On Tue, Nov 12, 2019 at 8:06 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > On Tue, Nov 12, 2019 at 5:48 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > > > > > On Mon, Nov 11, 2019 at 8:30 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > > > > > Overlayfs timestamp overflow limits should be inherrited from upper > > > > > filesystem. > > > > > > > > > > The current behavior, when overlayfs is over an underlying filesystem > > > > > that does not support post 2038 timestamps (e.g. xfs), is that overlayfs > > > > > overflows post 2038 timestamps instead of clamping them. > > > > > > > > How? Isn't the clamping supposed to happen in the underlying filesystem anyway? > > > > > > > > > > Not sure if it is supposed to be it doesn't. > > > It happens in do_utimes() -> utimes_common() > > > > Clamping also happens as part of current_time(). If this is called on > > an inode belonging to the upper fs, then the timestamps are clamped to > > those limits. > > > > OK, but from utimes syscall they do not get clamped inside filesystem > only in syscall itself. Right? Yes, that's right. -Deepa
On Tue, Nov 12, 2019 at 5:06 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Tue, Nov 12, 2019 at 5:48 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > On Mon, Nov 11, 2019 at 8:30 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > Overlayfs timestamp overflow limits should be inherrited from upper > > > filesystem. > > > > > > The current behavior, when overlayfs is over an underlying filesystem > > > that does not support post 2038 timestamps (e.g. xfs), is that overlayfs > > > overflows post 2038 timestamps instead of clamping them. > > > > How? Isn't the clamping supposed to happen in the underlying filesystem anyway? > > > > Not sure if it is supposed to be it doesn't. > It happens in do_utimes() -> utimes_common() Ah. How about moving the timestamp_truncate() inside notify_change()? > clamping seems to happen when user sets the times, > so setting the overlay limits to those of upper fs seems > correct anyway. It does seem correct, I just think moving the truncation into the right layer would make more sense. Thanks, Miklos
On Wed, Nov 13, 2019 at 11:26:13AM +0100, Miklos Szeredi wrote: > On Tue, Nov 12, 2019 at 5:06 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > On Tue, Nov 12, 2019 at 5:48 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > > > On Mon, Nov 11, 2019 at 8:30 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > > > Overlayfs timestamp overflow limits should be inherrited from upper > > > > filesystem. > > > > > > > > The current behavior, when overlayfs is over an underlying filesystem > > > > that does not support post 2038 timestamps (e.g. xfs), is that overlayfs > > > > overflows post 2038 timestamps instead of clamping them. > > > > > > How? Isn't the clamping supposed to happen in the underlying filesystem anyway? > > > > > > > Not sure if it is supposed to be it doesn't. > > It happens in do_utimes() -> utimes_common() > > Ah. How about moving the timestamp_truncate() inside notify_change()? Untested patch below. BTW overlayfs isn't the only one calling notify_change(). There's knfsd and ecryptfs, neither of which seems to clamp timestamps according on the underlying filesystem. Thanks, Miklos diff --git a/fs/attr.c b/fs/attr.c index df28035aa23e..e8de5e636e66 100644 --- a/fs/attr.c +++ b/fs/attr.c @@ -268,8 +268,13 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de attr->ia_ctime = now; if (!(ia_valid & ATTR_ATIME_SET)) attr->ia_atime = now; + else + attr->ia_atime = timestamp_truncate(attr->ia_atime, inode); if (!(ia_valid & ATTR_MTIME_SET)) attr->ia_mtime = now; + else + attr->ia_mtime = timestamp_truncate(attr->ia_mtime, inode); + if (ia_valid & ATTR_KILL_PRIV) { error = security_inode_need_killpriv(dentry); if (error < 0) diff --git a/fs/utimes.c b/fs/utimes.c index 1ba3f7883870..df483207da4e 100644 --- a/fs/utimes.c +++ b/fs/utimes.c @@ -35,17 +35,13 @@ static int utimes_common(const struct path *path, struct timespec64 *times) if (times) { if (times[0].tv_nsec == UTIME_OMIT) newattrs.ia_valid &= ~ATTR_ATIME; - else if (times[0].tv_nsec != UTIME_NOW) { - newattrs.ia_atime = timestamp_truncate(times[0], inode); + else if (times[0].tv_nsec != UTIME_NOW) 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 = timestamp_truncate(times[1], inode); + else if (times[1].tv_nsec != UTIME_NOW) newattrs.ia_valid |= ATTR_MTIME_SET; - } /* * Tell setattr_prepare(), that this is an explicit time * update, even if neither ATTR_ATIME_SET nor ATTR_MTIME_SET
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 5d4faab57ba0..44915874fccb 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -1621,6 +1621,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) sb->s_stack_depth = ofs->upper_mnt->mnt_sb->s_stack_depth; sb->s_time_gran = ofs->upper_mnt->mnt_sb->s_time_gran; + sb->s_time_min = ofs->upper_mnt->mnt_sb->s_time_min; + sb->s_time_max = ofs->upper_mnt->mnt_sb->s_time_max; } oe = ovl_get_lowerstack(sb, ofs);
Overlayfs timestamp overflow limits should be inherrited from upper filesystem. The current behavior, when overlayfs is over an underlying filesystem that does not support post 2038 timestamps (e.g. xfs), is that overlayfs overflows post 2038 timestamps instead of clamping them. This change fixes xfstest generic/402 (verify filesystem timestamps for supported ranges). Cc: Deepa Dinamani <deepa.kernel@gmail.com> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- (*) generic/402 currently does 'notrun' on overlayfs and need a small fix that I will post shortly fs/overlayfs/super.c | 2 ++ 1 file changed, 2 insertions(+)