[PATCH] ovl: fix timestamp limits

[PATCH] ovl: fix timestamp limits

[Amir Goldstein]

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(+)

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);
-- 
2.17.1

Re: [PATCH] ovl: fix timestamp limits

[Deepa Dinamani]

Thanks for the fix.

Acked-by: Deepa Dinamani <deepa.kernel@gmail.com>

Re: [PATCH] ovl: fix timestamp limits

[Miklos Szeredi]

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

Re: [PATCH] ovl: fix timestamp limits

[Amir Goldstein]

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.

Re: [PATCH] ovl: fix timestamp limits

[Deepa Dinamani]

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

Re: [PATCH] ovl: fix timestamp limits

[Amir Goldstein]

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.

Re: [PATCH] ovl: fix timestamp limits

[Deepa Dinamani]

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

Re: [PATCH] ovl: fix timestamp limits

[Miklos Szeredi]

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

Re: [PATCH] ovl: fix timestamp limits

[Miklos Szeredi]

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