ovl: fix timestamp limits
diff mbox series

Message ID 20191111073000.2957-1-amir73il@gmail.com
State New
Headers show
Series
  • ovl: fix timestamp limits
Related show

Commit Message

Amir Goldstein Nov. 11, 2019, 7:30 a.m. UTC
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(+)

Comments

Deepa Dinamani Nov. 11, 2019, 10:31 p.m. UTC | #1
Thanks for the fix.

Acked-by: Deepa Dinamani <deepa.kernel@gmail.com>
Miklos Szeredi Nov. 12, 2019, 3:48 p.m. UTC | #2
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
Amir Goldstein Nov. 12, 2019, 4:06 p.m. UTC | #3
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.
Deepa Dinamani Nov. 12, 2019, 4:45 p.m. UTC | #4
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
Amir Goldstein Nov. 12, 2019, 4:49 p.m. UTC | #5
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.
Deepa Dinamani Nov. 12, 2019, 4:56 p.m. UTC | #6
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
Miklos Szeredi Nov. 13, 2019, 10:26 a.m. UTC | #7
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
Miklos Szeredi Nov. 13, 2019, 11:26 a.m. UTC | #8
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

Patch
diff mbox series

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