Re: [Y2038] [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time

2020-01-07 Thread Darrick J. Wong
On Tue, Jan 07, 2020 at 06:16:34AM -0800, Christoph Hellwig wrote:
> On Thu, Jan 02, 2020 at 10:07:49AM -0800, Darrick J. Wong wrote:
> > > Sorry I missed that comment earlier. I've had a fresh look now, but
> > > I think we still need to deprecate XFS_IOC_SWAPEXT and add a
> > > v5 version of it, since the comparison will fail as soon as the range
> > > of the inode timestamps is extended beyond 2038, otherwise the
> > > comparison will always be false, or require comparing the truncated
> > > time values which would add yet another representation.
> > 
> > I prefer we replace the old SWAPEXT with a new version to get rid of
> > struct xfs_bstat.  Though a SWAPEXT_V5 probably only needs to contain
> > the *stat fields that swapext actually needs to check that the file
> > hasn't been changed, which would be ino/gen/btime/ctime.
> > 
> > (Maybe I'd add an offset/length too...)
> 
> And most importantly we need to lift it to the VFS instead of all the
> crazy fs specific interfaces at the moment.

Yeah.  Fixing that (and maybe adding an ioctl to set the FS UUID online)
were on my list for 5.6 but clearly I have to defer everything until 5.7
because we've just run out of time.

Uh... I started looking into unifying the ext4 and xfs defrag ioctl, but
gagged when I realized that the ext4 ioctl also handles the data copy
inside the kernel.  I think that's the sort of behavior we should /not/
allow into the new ioctl, though that also means that the required
changes for ext4/e4defrag will be non-trivial.

The btrfs defrag ioctl also contains thresholding information and
optional knobs to enable compression, which makes me wonder if we should
focus narrowly on swapext being "swap these extents but only if the
source file hasn't changed" and not necessarily defrag?

...in which case I wonder, can people (ab)use this interface for atomic
file updates?  Create an O_TMPFILE, reflink the source file into the
temp file, make your updates to the tempfile, and then swapext the
donor's file data back into the source file, but only if the source file
hasn't changed?

--D
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038


Re: [Y2038] [PATCH v3 2/2] xfs: quota: move to time64_t interfaces

2020-01-02 Thread Darrick J. Wong
On Thu, Jan 02, 2020 at 09:40:46PM +0100, Arnd Bergmann wrote:
> As a preparation for removing the 32-bit time_t type and
> all associated interfaces, change xfs to use time64_t and
> ktime_get_real_seconds() for the quota housekeeping.
> 
> This avoids one difference between 32-bit and 64-bit kernels,
> raising the theoretical limit for the quota grace period
> to year 2106 on 32-bit instead of year 2038.
> 
> Note that common user space tools using the XFS quotactl
> interface instead of the generic one still use the y2038
> dates.
> 
> To fix quotas properly, both the on-disk format and user
> space still need to be changed.
>
> Signed-off-by: Arnd Bergmann 
> ---
> This has a small conflict against the series at
> https://www.spinics.net/lists/linux-xfs/msg35409.html
> ("xfs: widen timestamps to deal with y2038") which needs
> to be rebased on top of this.
> 
> All other changes to remove time_t and get_seconds()
> are now in linux-next, this is one of the last patches
> needed to remove their definitions for v5.6.
> 
> If the widened timestamps make it into v5.6, this patch
> can be dropped.

Not likely. :)

I'll give this series a spin,
Reviewed-by: Darrick J. Wong 

--D

> ---
>  fs/xfs/xfs_dquot.c   | 6 +++---
>  fs/xfs/xfs_qm.h  | 6 +++---
>  fs/xfs/xfs_quotaops.c| 6 +++---
>  fs/xfs/xfs_trans_dquot.c | 8 +---
>  4 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 2bff21ca9d78..9cfd3209f52b 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -137,7 +137,7 @@ xfs_qm_adjust_dqtimers(
>   (d->d_blk_hardlimit &&
>(be64_to_cpu(d->d_bcount) >
> be64_to_cpu(d->d_blk_hardlimit {
> - d->d_btimer = cpu_to_be32(get_seconds() +
> + d->d_btimer = cpu_to_be32(ktime_get_real_seconds() +
>   mp->m_quotainfo->qi_btimelimit);
>   } else {
>   d->d_bwarns = 0;
> @@ -160,7 +160,7 @@ xfs_qm_adjust_dqtimers(
>   (d->d_ino_hardlimit &&
>(be64_to_cpu(d->d_icount) >
> be64_to_cpu(d->d_ino_hardlimit {
> - d->d_itimer = cpu_to_be32(get_seconds() +
> + d->d_itimer = cpu_to_be32(ktime_get_real_seconds() +
>   mp->m_quotainfo->qi_itimelimit);
>   } else {
>   d->d_iwarns = 0;
> @@ -183,7 +183,7 @@ xfs_qm_adjust_dqtimers(
>   (d->d_rtb_hardlimit &&
>(be64_to_cpu(d->d_rtbcount) >
> be64_to_cpu(d->d_rtb_hardlimit {
> - d->d_rtbtimer = cpu_to_be32(get_seconds() +
> + d->d_rtbtimer = cpu_to_be32(ktime_get_real_seconds() +
>   mp->m_quotainfo->qi_rtbtimelimit);
>   } else {
>   d->d_rtbwarns = 0;
> diff --git a/fs/xfs/xfs_qm.h b/fs/xfs/xfs_qm.h
> index 7823af39008b..4e57edca8bce 100644
> --- a/fs/xfs/xfs_qm.h
> +++ b/fs/xfs/xfs_qm.h
> @@ -64,9 +64,9 @@ struct xfs_quotainfo {
>   struct xfs_inode*qi_pquotaip;   /* project quota inode */
>   struct list_lru  qi_lru;
>   int  qi_dquots;
> - time_t   qi_btimelimit;  /* limit for blks timer */
> - time_t   qi_itimelimit;  /* limit for inodes timer */
> - time_t   qi_rtbtimelimit;/* limit for rt blks timer */
> + time64_t qi_btimelimit;  /* limit for blks timer */
> + time64_t qi_itimelimit;  /* limit for inodes timer */
> + time64_t qi_rtbtimelimit;/* limit for rt blks timer */
>   xfs_qwarncnt_t   qi_bwarnlimit;  /* limit for blks warnings */
>   xfs_qwarncnt_t   qi_iwarnlimit;  /* limit for inodes warnings */
>   xfs_qwarncnt_t   qi_rtbwarnlimit;/* limit for rt blks warnings */
> diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c
> index c7de17deeae6..38669e827206 100644
> --- a/fs/xfs/xfs_quotaops.c
> +++ b/fs/xfs/xfs_quotaops.c
> @@ -37,9 +37,9 @@ xfs_qm_fill_state(
>   tstate->flags |= QCI_SYSFILE;
>   tstate->blocks = ip->i_d.di_nblocks;
>   tstate->nextents = ip->i_d.di_nextents;
> - tstate->spc_timelimit = q->qi_btimelimit;
> - tstate->ino_timelimit = q->qi_itimelimit;
> - tstate->rt_spc_timelimit = q->qi_rtbtimelimit;
> + tstate->spc_timelimit = (u32)q->qi_btimelimit;
> + tstate->in

Re: [Y2038] [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time

2020-01-02 Thread Darrick J. Wong
On Thu, Jan 02, 2020 at 10:16:21AM +0100, Arnd Bergmann wrote:
> On Tue, Dec 24, 2019 at 9:45 AM Christoph Hellwig  wrote:
> > On Wed, Dec 18, 2019 at 05:39:29PM +0100, Arnd Bergmann wrote:
> > > +/* disallow y2038-unsafe ioctls with CONFIG_COMPAT_32BIT_TIME=n */
> > > +static bool xfs_have_compat_bstat_time32(unsigned int cmd)
> > > +{
> > > + if (IS_ENABLED(CONFIG_COMPAT_32BIT_TIME))
> > > + return true;
> > > +
> > > + if (IS_ENABLED(CONFIG_64BIT) && !in_compat_syscall())
> > > + return true;
> > > +
> > > + if (cmd == XFS_IOC_FSBULKSTAT_SINGLE ||
> > > + cmd == XFS_IOC_FSBULKSTAT ||
> > > + cmd == XFS_IOC_SWAPEXT)
> > > + return false;
> > > +
> > > + return true;
> >
> > I think the check for the individual command belongs into the callers,
> > which laves us with:
> >
> > static inline bool have_time32(void)
> > {
> > return IS_ENABLED(CONFIG_COMPAT_32BIT_TIME) ||
> > (IS_ENABLED(CONFIG_64BIT) && !in_compat_syscall());
> > }
> >
> > and that looks like it should be in a generic helper somewhere.
> 
> Yes, makes sense.
> 
> I was going for something XFS specific here because XFS is unique in the
> kernel in completely deprecating a set of ioctl commands (replacing
> the old interface with a v5) rather than allowing the user space to be
> compiled with 64-bit time_t.
> 
> If we add a global helper for this, I'd be tempted to also stick a
> WARN_RATELIMIT() in there to give users a better indication of
> what broke after disabling CONFIG_COMPAT_32BIT_TIME.
> 
> The same warning would make sense in the system calls, but then
> we have to decide which combinations we want to allow being
> configured at runtime or compile-time.
> 
> a) unmodified behavior
> b) just warn but allow
> c) no warning but disallow
> d) warn and disallow
> 
> > >   if (XFS_FORCED_SHUTDOWN(mp))
> > >   return -EIO;
> > >
> > > @@ -1815,6 +1836,11 @@ xfs_ioc_swapext(
> > >   struct fd   f, tmp;
> > >   int error = 0;
> > >
> > > + if (!xfs_have_compat_bstat_time32(XFS_IOC_SWAPEXT)) {
> > > + error = -EINVAL;
> > > + goto out;
> > > + }
> >
> > And for this one we just have one cmd anyway.  But I actually still
> > disagree with the old_time check for this one entirely, as voiced on
> > one of the last iterations.  For swapext the time stamp really is
> > only used as a generation counter, so overflows are entirely harmless.
> 
> Sorry I missed that comment earlier. I've had a fresh look now, but
> I think we still need to deprecate XFS_IOC_SWAPEXT and add a
> v5 version of it, since the comparison will fail as soon as the range
> of the inode timestamps is extended beyond 2038, otherwise the
> comparison will always be false, or require comparing the truncated
> time values which would add yet another representation.

I prefer we replace the old SWAPEXT with a new version to get rid of
struct xfs_bstat.  Though a SWAPEXT_V5 probably only needs to contain
the *stat fields that swapext actually needs to check that the file
hasn't been changed, which would be ino/gen/btime/ctime.

(Maybe I'd add an offset/length too...)

--D

>Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038


Re: [Y2038] [PATCH v2 21/24] xfs: quota: move to time64_t interfaces

2019-12-17 Thread Darrick J. Wong
On Tue, Dec 17, 2019 at 04:02:47PM +0100, Arnd Bergmann wrote:
> On Mon, Dec 16, 2019 at 5:52 PM Arnd Bergmann  wrote:
> > On Fri, Dec 13, 2019 at 10:17 PM Darrick J. Wong  
> > wrote:
> >>
> >> Hmm, so one thing that I clean up on the way to bigtime is the total
> >> lack of clamping here.  If (for example) it's September 2105 and
> >> rtbtimelimit is set to 1 year, this will cause an integer overflow.  The
> >> quota timer will be set to 1970 and expire immediately, rather than what
> >> I'd consider the best effort of February 2106.
> 
> One more hing to note (I will add this to the changelog text) is that on

Ok, I'll look for it in the next revision you send out.

By the way, would you mind cc'ing the xfs list on all 24 patches?  They
probably aren't directly relevant to xfs, but it does make it a lot
easier for us to look at the other 21 patches and think "Oh, ok, so
there isn't some core infrastructure change that we're not seeing".

> 32-bit architectures, the limit here is y2038, while on 64-bit
> architectures it's y2106:

Yikes.  I probably just need to send the bigtime series and see what you
all think about the mess I created^W^W^Wway I dealt with all that.

> int xfs_trans_dqresv(...)
> {
>time_t  timer; /* signed 'long' */
>timer = be32_to_cpu(dqp->q_core.d_btimer);
>/* get_seconds() returns unsigned long */
>   if ((timer != 0 && get_seconds() > timer))
> return -EDQUOT;
> }
> 
> > I don't think clamping would be good here, that just replaces
> > one bug with another at the overflow time. If you would like to
> > have something better before this gets extended, I could try to
> > come up with a version that converts it to the nearest 64-bit
> > timestamp, similar to the way that time_before32() in the kernel
> > or the NTP protocol work.
> >
> > If you think it can get extended properly soon, I'd just leave the
> > patch as it is today in order to remove the get_seconds()
> > interface for v5.6.
> 
> I've tried this now, and but this feels wrong: it adds lots of complexity
> for corner cases and is still fragile, e.g. when the time is wrong
> during boot before ntp runs. See that patch below for reference.

Yeah, that is pretty weird to glue the upper 32 bits of the timestamp
onto the expiration timer...

--D

> I also see that quotatool on xfs always uses the old xfs quota
> interface, so it already overflows on the user space side. Fixing
> this properly seems to be a bigger effort than I was planning for
> (on an unpatched 64-bit kernel):
> 
> $ sudo quotatool -b-u  -t 220month  /mnt/tmp -r
> $ rm file ; fallocate -l 11M file
> $ sudo quotatool -d /mnt/tmp -u arnd
> 1000 /mnt/tmp 11264 10240 20480 570239975 2 0 00
> $ sudo quotatool -b-u  -t 222month  /mnt/tmp -r
> $ rm file ; fallocate -l 11M file
> $ sudo quotatool -d /mnt/tmp -u arnd
> 1000 /mnt/tmp 11264 10240 20480 18446744069990008316 2 0 00
> 
>Arnd
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 9cfd3209f52b..6c9128bb607b 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -98,6 +98,23 @@ xfs_qm_adjust_dqlimits(
> xfs_dquot_set_prealloc_limits(dq);
>  }
> 
> +static __be32 xfs_quota_timeout32(s64 limit)
> +{
> +   time64_t now = ktime_get_real_seconds();
> +   u32 timeout;
> +
> +   /* avoid overflows in out-of-range limits */
> +   if ((u64)limit > S32_MAX)
> +   limit = S32_MAX;
> +   timeout = now + limit;
> +
> +   /* avoid timeout of zero */
> +   if (lower_32_bits(timeout) == 0)
> +   return cpu_to_be32(1);
> +
> +   return cpu_to_be32(lower_32_bits(timeout));
> +}
> +
>  /*
>   * Check the limits and timers of a dquot and start or reset timers
>   * if necessary.
> @@ -137,7 +154,7 @@ xfs_qm_adjust_dqtimers(
> (d->d_blk_hardlimit &&
>  (be64_to_cpu(d->d_bcount) >
>   be64_to_cpu(d->d_blk_hardlimit {
> -   d->d_btimer = cpu_to_be32(ktime_get_real_seconds() +
> +   d->d_btimer = xfs_quota_timeout32(
> mp->m_quotainfo->qi_btimelimit);
> } else {
> d->d_bwarns = 0;
> @@ -160,7 +177,7 @@ xfs_qm_adjust_dqtimers(
> (d->d_ino_hardlimit &&
>  (be64_to_cpu(d->d_icount) >
>   be64_to_cpu(d->d_ino_hardlimit {
> -   d->d_itimer = cpu_

Re: [Y2038] [PATCH v2 20/24] xfs: disallow broken ioctls without compat-32-bit-time

2019-12-16 Thread Darrick J. Wong
On Mon, Dec 16, 2019 at 05:45:29PM +0100, Arnd Bergmann wrote:
> On Fri, Dec 13, 2019 at 10:05 PM Darrick J. Wong
>  wrote:
> >
> > On Fri, Dec 13, 2019 at 09:53:48PM +0100, Arnd Bergmann wrote:
> > > When building a kernel that disables support for 32-bit time_t
> > > system calls, it also makes sense to disable the old xfs_bstat
> > > ioctls completely, as they truncate the timestamps to 32-bit
> > > values.
> >
> > Note that current xfs doesn't support > 32-bit timestamps at all, so for
> > now the old bulkstat/swapext ioctls will never overflow.
> 
> Right, this patch originally came after my version of the 40-bit
> timestamps that I dropped from the series now.
> 
> I've added "... once the extended times are supported." above now.
> 
> > Granted, I melded everyone's suggestions into a more fully formed
> > 'bigtime' feature patchset that I'll dump out soon as part of my usual
> > end of year carpetbombing of the mailing list, so we likely still need
> > most of this patch anyway...
> 
> What is the timeline for that work now? I'm mainly interested in
> getting the removal of 'time_t/timeval/timespec' and 'get_seconds()'
> from the kernel done for v5.6, but it would be good to also have
> this patch and the extended timestamps in the same version
> just so we can claim that "all known y2038 issues" are addressed
> in that release (I'm sure we will run into bugs we don't know yet).

Personally, I think you should push this whenever it's ready.  Are you
aiming to send all 24 patches as a treewide pull request directly to
Linus, or would you rather the 2-3 xfs patches go through the xfs tree?

The y2038 format changes are going to take a while to push through
review.  If somehow it all gets through review for 5.6 I can always
apply both and fix the merge damage, but more likely y2038 timestamps is
a  5.8 EXPERIMENTAL thing.

Or later, given that Dave and I both have years worth of unreviewed
patch backlog. :(

> > > @@ -617,6 +618,23 @@ xfs_fsinumbers_fmt(
> > >   return xfs_ibulk_advance(breq, sizeof(struct xfs_inogrp));
> > >  }
> > >
> > > +/* disallow y2038-unsafe ioctls with CONFIG_COMPAT_32BIT_TIME=n */
> > > +static bool xfs_have_compat_bstat_time32(unsigned int cmd)
> >
> > The v5 bulkstat ioctls follow an entirely separate path through
> > xfs_ioctl.c, so I think you don't need the @cmd parameter.
> 
> The check is there to not forbid XFS_IOC_FSINUMBERS at
> the moment, since that is not affected.

Aha.

> > > @@ -1815,6 +1836,11 @@ xfs_ioc_swapext(
> > >   struct fd   f, tmp;
> > >   int error = 0;
> > >
> > > + if (xfs_have_compat_bstat_time32(XFS_IOC_SWAPEXT)) {
> >
> > if (!xfs_have...()) ?
> 
> Right, fixed now.



--D

>Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038


Re: [Y2038] [PATCH v2 19/24] xfs: rename compat_time_t to old_time32_t

2019-12-13 Thread Darrick J. Wong
On Fri, Dec 13, 2019 at 09:53:47PM +0100, Arnd Bergmann wrote:
> The compat_time_t type has been removed everywhere else,
> as most users rely on old_time32_t for both native and
> compat mode handling of 32-bit time_t.
> 
> Remove the last one in xfs.
> 
> Signed-off-by: Arnd Bergmann 

Looks fine to me, assuming that compat_time_t -> old_time32_t.
Reviewed-by: Darrick J. Wong 

--D

> ---
>  fs/xfs/xfs_ioctl32.c | 2 +-
>  fs/xfs/xfs_ioctl32.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> index c4c4f09113d3..a49bd80b2c3b 100644
> --- a/fs/xfs/xfs_ioctl32.c
> +++ b/fs/xfs/xfs_ioctl32.c
> @@ -107,7 +107,7 @@ xfs_ioctl32_bstime_copyin(
>   xfs_bstime_t*bstime,
>   compat_xfs_bstime_t __user *bstime32)
>  {
> - compat_time_t   sec32;  /* tv_sec differs on 64 vs. 32 */
> + old_time32_tsec32;  /* tv_sec differs on 64 vs. 32 */
>  
>   if (get_user(sec32, >tv_sec)  ||
>   get_user(bstime->tv_nsec,   >tv_nsec))
> diff --git a/fs/xfs/xfs_ioctl32.h b/fs/xfs/xfs_ioctl32.h
> index 8c7743cd490e..053de7d894cd 100644
> --- a/fs/xfs/xfs_ioctl32.h
> +++ b/fs/xfs/xfs_ioctl32.h
> @@ -32,7 +32,7 @@
>  #endif
>  
>  typedef struct compat_xfs_bstime {
> - compat_time_t   tv_sec; /* seconds  */
> + old_time32_ttv_sec; /* seconds  */
>   __s32   tv_nsec;/* and nanoseconds  */
>  } compat_xfs_bstime_t;
>  
> -- 
> 2.20.0
> 
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038


Re: [Y2038] [PATCH v2 21/24] xfs: quota: move to time64_t interfaces

2019-12-13 Thread Darrick J. Wong
On Fri, Dec 13, 2019 at 09:53:49PM +0100, Arnd Bergmann wrote:
> As a preparation for removing the 32-bit time_t type and
> all associated interfaces, change xfs to use time64_t and
> ktime_get_real_seconds() for the quota housekeeping.
> 
> Signed-off-by: Arnd Bergmann 

Looks mostly reasonable to me...

The bigtime series refactors the triplicated timer handling and whatnot,
but I don't think it would be difficult to rebase that series assuming
this lands first (which it probably will, I expect a new incompat ondisk
feature to take a /long/ time to get through review.)

> ---
>  fs/xfs/xfs_dquot.c   | 6 +++---
>  fs/xfs/xfs_qm.h  | 6 +++---
>  fs/xfs/xfs_quotaops.c| 6 +++---
>  fs/xfs/xfs_trans_dquot.c | 8 +---
>  4 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 2bff21ca9d78..9cfd3209f52b 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -137,7 +137,7 @@ xfs_qm_adjust_dqtimers(
>   (d->d_blk_hardlimit &&
>(be64_to_cpu(d->d_bcount) >
> be64_to_cpu(d->d_blk_hardlimit {
> - d->d_btimer = cpu_to_be32(get_seconds() +
> + d->d_btimer = cpu_to_be32(ktime_get_real_seconds() +
>   mp->m_quotainfo->qi_btimelimit);
>   } else {
>   d->d_bwarns = 0;
> @@ -160,7 +160,7 @@ xfs_qm_adjust_dqtimers(
>   (d->d_ino_hardlimit &&
>(be64_to_cpu(d->d_icount) >
> be64_to_cpu(d->d_ino_hardlimit {
> - d->d_itimer = cpu_to_be32(get_seconds() +
> + d->d_itimer = cpu_to_be32(ktime_get_real_seconds() +
>   mp->m_quotainfo->qi_itimelimit);
>   } else {
>   d->d_iwarns = 0;
> @@ -183,7 +183,7 @@ xfs_qm_adjust_dqtimers(
>   (d->d_rtb_hardlimit &&
>(be64_to_cpu(d->d_rtbcount) >
> be64_to_cpu(d->d_rtb_hardlimit {
> - d->d_rtbtimer = cpu_to_be32(get_seconds() +
> + d->d_rtbtimer = cpu_to_be32(ktime_get_real_seconds() +
>   mp->m_quotainfo->qi_rtbtimelimit);

Hmm, so one thing that I clean up on the way to bigtime is the total
lack of clamping here.  If (for example) it's September 2105 and
rtbtimelimit is set to 1 year, this will cause an integer overflow.  The
quota timer will be set to 1970 and expire immediately, rather than what
I'd consider the best effort of February 2106.

(I'll grant you the current code also behaves like this...)

Reviewed-by: Darrick J. Wong 

--D

>   } else {
>   d->d_rtbwarns = 0;
> diff --git a/fs/xfs/xfs_qm.h b/fs/xfs/xfs_qm.h
> index 7823af39008b..4e57edca8bce 100644
> --- a/fs/xfs/xfs_qm.h
> +++ b/fs/xfs/xfs_qm.h
> @@ -64,9 +64,9 @@ struct xfs_quotainfo {
>   struct xfs_inode*qi_pquotaip;   /* project quota inode */
>   struct list_lru  qi_lru;
>   int  qi_dquots;
> - time_t   qi_btimelimit;  /* limit for blks timer */
> - time_t   qi_itimelimit;  /* limit for inodes timer */
> - time_t   qi_rtbtimelimit;/* limit for rt blks timer */
> + time64_t qi_btimelimit;  /* limit for blks timer */
> + time64_t qi_itimelimit;  /* limit for inodes timer */
> + time64_t qi_rtbtimelimit;/* limit for rt blks timer */
>   xfs_qwarncnt_t   qi_bwarnlimit;  /* limit for blks warnings */
>   xfs_qwarncnt_t   qi_iwarnlimit;  /* limit for inodes warnings */
>   xfs_qwarncnt_t   qi_rtbwarnlimit;/* limit for rt blks warnings */
> diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c
> index c7de17deeae6..38669e827206 100644
> --- a/fs/xfs/xfs_quotaops.c
> +++ b/fs/xfs/xfs_quotaops.c
> @@ -37,9 +37,9 @@ xfs_qm_fill_state(
>   tstate->flags |= QCI_SYSFILE;
>   tstate->blocks = ip->i_d.di_nblocks;
>   tstate->nextents = ip->i_d.di_nextents;
> - tstate->spc_timelimit = q->qi_btimelimit;
> - tstate->ino_timelimit = q->qi_itimelimit;
> - tstate->rt_spc_timelimit = q->qi_rtbtimelimit;
> + tstate->spc_timelimit = (u32)q->qi_btimelimit;
> + tstate->ino_timelimit = (u32)q->qi_itimelimit;
> + tstate->rt_spc_timelimit = (u32)q->qi_rtbtimelimit;
>   tstate->spc_warnlimit = q->qi_bwarnlimit;
>   tstate->ino_warnlimit = q->qi_iwarnlimit;
>   tstate->rt_spc_warnlimit = 

Re: [Y2038] [PATCH v2 20/24] xfs: disallow broken ioctls without compat-32-bit-time

2019-12-13 Thread Darrick J. Wong
On Fri, Dec 13, 2019 at 09:53:48PM +0100, Arnd Bergmann wrote:
> When building a kernel that disables support for 32-bit time_t
> system calls, it also makes sense to disable the old xfs_bstat
> ioctls completely, as they truncate the timestamps to 32-bit
> values.

Note that current xfs doesn't support > 32-bit timestamps at all, so for
now the old bulkstat/swapext ioctls will never overflow.

Granted, I melded everyone's suggestions into a more fully formed
'bigtime' feature patchset that I'll dump out soon as part of my usual
end of year carpetbombing of the mailing list, so we likely still need
most of this patch anyway...

> Any application using these needs to be updated to use the v5
> interfaces.
> 
> Signed-off-by: Arnd Bergmann 
> ---
>  fs/xfs/xfs_ioctl.c | 26 ++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 7b35d62ede9f..a4a4eed8879c 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -36,6 +36,7 @@
>  #include "xfs_reflink.h"
>  #include "xfs_ioctl.h"
>  
> +#include 
>  #include 
>  #include 
>  
> @@ -617,6 +618,23 @@ xfs_fsinumbers_fmt(
>   return xfs_ibulk_advance(breq, sizeof(struct xfs_inogrp));
>  }
>  
> +/* disallow y2038-unsafe ioctls with CONFIG_COMPAT_32BIT_TIME=n */
> +static bool xfs_have_compat_bstat_time32(unsigned int cmd)

The v5 bulkstat ioctls follow an entirely separate path through
xfs_ioctl.c, so I think you don't need the @cmd parameter.

> +{
> + if (IS_ENABLED(CONFIG_COMPAT_32BIT_TIME))
> + return true;
> +
> + if (IS_ENABLED(CONFIG_64BIT) && !in_compat_syscall())
> + return true;
> +
> + if (cmd == XFS_IOC_FSBULKSTAT_SINGLE ||
> + cmd == XFS_IOC_FSBULKSTAT ||
> + cmd == XFS_IOC_SWAPEXT)
> + return false;
> +
> + return true;
> +}
> +
>  STATIC int
>  xfs_ioc_fsbulkstat(
>   xfs_mount_t *mp,
> @@ -637,6 +655,9 @@ xfs_ioc_fsbulkstat(
>   if (!capable(CAP_SYS_ADMIN))
>   return -EPERM;
>  
> + if (!xfs_have_compat_bstat_time32(cmd))
> + return -EINVAL;
> +
>   if (XFS_FORCED_SHUTDOWN(mp))
>   return -EIO;
>  
> @@ -1815,6 +1836,11 @@ xfs_ioc_swapext(
>   struct fd   f, tmp;
>   int error = 0;
>  
> + if (xfs_have_compat_bstat_time32(XFS_IOC_SWAPEXT)) {

if (!xfs_have...()) ?

--D

> + error = -EINVAL;
> + goto out;
> + }
> +
>   /* Pull information for the target fd */
>   f = fdget((int)sxp->sx_fdtarget);
>   if (!f.file) {
> -- 
> 2.20.0
> 
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038


Re: [Y2038] [RFC 1/5] xfs: [variant A] avoid time_t in user api

2019-11-13 Thread Darrick J. Wong
On Wed, Nov 13, 2019 at 02:42:24PM +0100, Arnd Bergmann wrote:
> On Wed, Nov 13, 2019 at 6:08 AM Darrick J. Wong  
> wrote:
> > On Tue, Nov 12, 2019 at 03:16:00PM +0100, Christoph Hellwig wrote:
> > > On Tue, Nov 12, 2019 at 01:09:06PM +0100, Arnd Bergmann wrote:
> > > > However, as long as two observations are true, a much simpler solution
> > > > can be used:
> > > >
> > > > 1. xfsprogs is the only user space project that has a copy of this 
> > > > header
> > >
> > > We can't guarantee that.
> > >
> > > > 2. xfsprogs already has a replacement for all three affected ioctl 
> > > > commands,
> > > >based on the xfs_bulkstat structure to pass 64-bit timestamps
> > > >regardless of the architecture
> > >
> > > XFS_IOC_BULKSTAT replaces XFS_IOC_FSBULKSTAT directly, and can replace
> > > XFS_IOC_FSBULKSTAT_SINGLE indirectly, so that is easy.  Most users
> > > actually use the new one now through libfrog, although I found a user
> > > of the direct ioctl in the xfs_io tool, which could easily be fixed as
> > > well.
> >
> > Agreed, XFS_IOC_BULKSTAT is the replacement for the two FSBULKSTAT
> > variants.  The only question in my mind for the old ioctls is whether we
> > should return EOVERFLOW if the timestamp would overflow?  Or just
> > truncate the results?
> 
> I think neither of these would be particularly helpful, the result is that 
> users
> see no change in behavior until it's actually too late and the timestamps have
> overrun.
> 
> If we take variant A and just fix the ABI to 32-bit time_t, it is important
> that all user space stop using these ioctls and moves to the v5 interfaces
> instead (including SWAPEXT I guess).
> 
> Something along the lines of this change would work:
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index d50135760622..87318486c96e 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -830,6 +830,23 @@ xfs_fsinumbers_fmt(
> return xfs_ibulk_advance(breq, sizeof(struct xfs_inogrp));
>  }
> 
> +/* disallow y2038-unsafe ioctls with CONFIG_COMPAT_32BIT_TIME=n */
> +static bool xfs_have_compat_bstat_time32(unsigned int cmd)
> +{

Wouldn't we want a test here like:

if (!xfs_sb_version_hasbigtimestamps(>m_sb))
return true;

Since date overflow isn't a problem for existing xfs with 32-bit
timestamps, right?

> +   if (IS_ENABLED(CONFIG_COMPAT_32BIT_TIME))

Heh, I didn't know that existed.

> +   return true;
> +
> +   if (IS_ENABLED(CONFIG_64BIT) && !in_compat_syscall())
> +   return true;
> +   if (cmd == XFS_IOC_FSBULKSTAT_SINGLE ||
> +   cmd == XFS_IOC_FSBULKSTAT ||
> +   cmd == XFS_IOC_SWAPEXT)
> +   return false;
> +
> +   return true;
> +}
> +
>  STATIC int
>  xfs_ioc_fsbulkstat(
> xfs_mount_t *mp,
> @@ -850,6 +867,9 @@ xfs_ioc_fsbulkstat(
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
> 
> +   if (!xfs_have_compat_bstat_time32())
> +   return -EINVAL;
> +
> if (XFS_FORCED_SHUTDOWN(mp))
> return -EIO;
> 
> @@ -2051,6 +2071,11 @@ xfs_ioc_swapext(
> struct fd   f, tmp;
> int error = 0;
> 
> +   if (xfs_have_compat_bstat_time32(XFS_IOC_SWAPEXT)) {
> +   error = -EINVAL
> +   got out;
> +   }
> +
> /* Pull information for the target fd */
> f = fdget((int)sxp->sx_fdtarget);
> if (!f.file) {
> 
> This way, at least users that intentionally turn off CONFIG_COMPAT_32BIT_TIME
> run into the broken application right away, which forces them to upgrade or 
> fix
> the code to use the v5 ioctl.

Sounds reasonable.

--D

> > > > Based on those assumptions, changing xfs_bstime to use __kernel_long_t
> > > > instead of time_t in both the kernel and in xfsprogs preserves the 
> > > > current
> > > > ABI for any libc definition of time_t and solves the problem of passing
> > > > 64-bit timestamps to 32-bit user space.
> > >
> > > As said above their are not entirely true, but I still think this patch
> > > is the right thing to do, if only to get the time_t out of the ABI..
> > >
> > > Reviewed-by: Christoph Hellwig 
> >
> > Seconded,
> > Reviewed-by: Darrick J. Wong 
> 
> Thanks!
> 
>  Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038


Re: [Y2038] [RFC 1/5] xfs: [variant A] avoid time_t in user api

2019-11-12 Thread Darrick J. Wong
On Tue, Nov 12, 2019 at 03:16:00PM +0100, Christoph Hellwig wrote:
> On Tue, Nov 12, 2019 at 01:09:06PM +0100, Arnd Bergmann wrote:
> > However, as long as two observations are true, a much simpler solution
> > can be used:
> > 
> > 1. xfsprogs is the only user space project that has a copy of this header
> 
> We can't guarantee that.
> 
> > 2. xfsprogs already has a replacement for all three affected ioctl commands,
> >based on the xfs_bulkstat structure to pass 64-bit timestamps
> >regardless of the architecture
> 
> XFS_IOC_BULKSTAT replaces XFS_IOC_FSBULKSTAT directly, and can replace
> XFS_IOC_FSBULKSTAT_SINGLE indirectly, so that is easy.  Most users
> actually use the new one now through libfrog, although I found a user
> of the direct ioctl in the xfs_io tool, which could easily be fixed as
> well.

Agreed, XFS_IOC_BULKSTAT is the replacement for the two FSBULKSTAT
variants.  The only question in my mind for the old ioctls is whether we
should return EOVERFLOW if the timestamp would overflow?  Or just
truncate the results?

> XFS_IOC_SWAPEXT does not have a direct replacement, but the timestamp
> is only used to verify that the file did not change vs the previous
> stat.  So not being able to represent > 2038 times is not a real
> problem anyway.

Won't it become a problem when the tv_sec comparison in xfs_swap_extents
is type-promoted to the larger type but lacks the upper bits?

I guess we could force the check to the lower 32 bits on the assumption
that those are the most likely to change due to a file write.

I kinda want to do a SWAPEXT v5, fwiw

> At some point we should probably look into a file system independent
> defrag ioctl anyway, at which point we can deprecate XFS_IOC_SWAPEXT.
> 
> > Based on those assumptions, changing xfs_bstime to use __kernel_long_t
> > instead of time_t in both the kernel and in xfsprogs preserves the current
> > ABI for any libc definition of time_t and solves the problem of passing
> > 64-bit timestamps to 32-bit user space.
> 
> As said above their are not entirely true, but I still think this patch
> is the right thing to do, if only to get the time_t out of the ABI..
> 
> Reviewed-by: Christoph Hellwig 

Seconded,
Reviewed-by: Darrick J. Wong 

--D

___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038


Re: [Y2038] [PATCH 06/20] fs: Fill in max and min timestamps in superblock

2019-07-31 Thread Darrick J. Wong
 = MAX_LFS_FILESIZE;
> + sb->s_time_min = 0;
> + sb->s_time_max = U32_MAX;
>   sb->s_flags |= SB_RDONLY;
>   sb->s_op = _super_ops;
>  
> diff --git a/fs/ufs/super.c b/fs/ufs/super.c
> index 4ed0dca52ec8..1da0be667409 100644
> --- a/fs/ufs/super.c
> +++ b/fs/ufs/super.c
> @@ -843,6 +843,10 @@ static int ufs_fill_super(struct super_block *sb, void 
> *data, int silent)
>  
>   sb->s_maxbytes = MAX_LFS_FILESIZE;
>  
> + sb->s_time_gran = NSEC_PER_SEC;
> + sb->s_time_min = S32_MIN;
> + sb->s_time_max = S32_MAX;
> +
>   switch (sbi->s_mount_opt & UFS_MOUNT_UFSTYPE) {
>   case UFS_MOUNT_UFSTYPE_44BSD:
>   UFSD("ufstype=44bsd\n");
> @@ -861,6 +865,9 @@ static int ufs_fill_super(struct super_block *sb, void 
> *data, int silent)
>   uspi->s_fshift = 9;
>   uspi->s_sbsize = super_block_size = 1536;
>   uspi->s_sbbase =  0;
> + sb->s_time_gran = 1;
> + sb->s_time_min = S64_MIN;
> + sb->s_time_max = S64_MAX;
>   flags |= UFS_TYPE_UFS2 | UFS_DE_44BSD | UFS_UID_44BSD | 
> UFS_ST_44BSD | UFS_CG_44BSD;
>   break;
>   
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index a14d11d78bd8..1a0daf46bae8 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1685,6 +1685,8 @@ xfs_fs_fill_super(
>   sb->s_maxbytes = xfs_max_file_offset(sb->s_blocksize_bits);
>   sb->s_max_links = XFS_MAXLINK;
>   sb->s_time_gran = 1;
> + sb->s_time_min = S32_MIN;
> + sb->s_time_max = S32_MAX;

For the XFS part,

Reviewed-by: Darrick J. Wong 

--D

>   set_posix_acl_flag(sb);
>  
>   /* version 5 superblocks support inode version counters. */
> -- 
> 2.17.1
> 
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038


Re: [Y2038] [PATCH 09/20] ext4: Initialize timestamps limits

2019-07-31 Thread Darrick J. Wong
On Mon, Jul 29, 2019 at 06:49:13PM -0700, Deepa Dinamani wrote:
> ext4 has different overflow limits for max filesystem
> timestamps based on the extra bytes available.
> 
> The timestamp limits are calculated according to the
> encoding table in
> a4dad1ae24f85i(ext4: Fix handling of extended tv_sec):
> 
> * extra  msb of adjust for signed
> * epoch  32-bit 32-bit tv_sec to
> * bits   timedecoded 64-bit tv_sec  64-bit tv_sec  valid time range
> * 0 01-0x8000..-0x0001  0x0   1901-12-13..1969-12-31
> * 0 000x0..0x07fff  0x0   1970-01-01..2038-01-19
> * 0 110x08000..0x0  0x1   2038-01-19..2106-02-07
> * 0 100x1..0x17fff  0x1   2106-02-07..2174-02-25
> * 1 010x18000..0x1  0x2   2174-02-25..2242-03-16
> * 1 000x2..0x27fff  0x2   2242-03-16..2310-04-04
> * 1 110x28000..0x2  0x3   2310-04-04..2378-04-22
> * 1 100x3..0x37fff  0x3   2378-04-22..2446-05-10

My recollection of ext4 has gotten rusty, so this could be a bogus
question:

Say you have a filesystem with s_inode_size > 128 where not all of the
ondisk inodes have been upgraded to i_extra_isize > 0 and therefore
don't support nanoseconds or times beyond 2038.  I think this happens on
ext3 filesystems that reserved extra space for inode attrs that are
subsequently converted to ext4?

In any case, that means that you have some inodes that support 34-bit
tv_sec and some inodes that only support 32-bit tv_sec.  For the inodes
with 32-bit tv_sec, I think all that happens is that a large timestamp
will be truncated further, right?

And no mount time warning because at least /some/ of the inodes are
ready to go for the next 30 years?

--D

> Note that the time limits are not correct for deletion times.
> 
> Signed-off-by: Deepa Dinamani 
> Reviewed-by: Andreas Dilger 
> Cc: ty...@mit.edu
> Cc: adilger.ker...@dilger.ca
> Cc: linux-e...@vger.kernel.org
> ---
>  fs/ext4/ext4.h  |  4 
>  fs/ext4/super.c | 17 +++--
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 1cb67859e051..3f13cf12ae7f 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1631,6 +1631,10 @@ static inline void ext4_clear_state_flags(struct 
> ext4_inode_info *ei)
>  
>  #define EXT4_GOOD_OLD_INODE_SIZE 128
>  
> +#define EXT4_EXTRA_TIMESTAMP_MAX (((s64)1 << 34) - 1  + S32_MIN)
> +#define EXT4_NON_EXTRA_TIMESTAMP_MAX S32_MAX
> +#define EXT4_TIMESTAMP_MIN   S32_MIN
> +
>  /*
>   * Feature set definitions
>   */
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 4079605d437a..3ea2d60f33aa 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4035,8 +4035,21 @@ static int ext4_fill_super(struct super_block *sb, 
> void *data, int silent)
>  sbi->s_inode_size);
>   goto failed_mount;
>   }
> - if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE)
> - sb->s_time_gran = 1 << (EXT4_EPOCH_BITS - 2);
> + /*
> +  * i_atime_extra is the last extra field available for 
> [acm]times in
> +  * struct ext4_inode. Checking for that field should suffice to 
> ensure
> +  * we have extra space for all three.
> +  */
> + if (sbi->s_inode_size >= offsetof(struct ext4_inode, 
> i_atime_extra) +
> + sizeof(((struct ext4_inode *)0)->i_atime_extra)) {
> + sb->s_time_gran = 1;
> + sb->s_time_max = EXT4_EXTRA_TIMESTAMP_MAX;
> + } else {
> + sb->s_time_gran = NSEC_PER_SEC;
> + sb->s_time_max = EXT4_NON_EXTRA_TIMESTAMP_MAX;
> + }
> +
> + sb->s_time_min = EXT4_TIMESTAMP_MIN;
>   }
>  
>   sbi->s_desc_size = le16_to_cpu(es->s_desc_size);
> -- 
> 2.17.1
> 
___
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

2019-07-31 Thread Darrick J. Wong
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


Re: [Y2038] [RFC 0/6] vfs: Add timestamp range check support

2016-11-03 Thread Darrick J. Wong
On Thu, Nov 03, 2016 at 09:48:27AM +1100, Dave Chinner wrote:
> On Wed, Nov 02, 2016 at 08:04:50AM -0700, Deepa Dinamani wrote:
> > The series is aimed at adding timestamp checking and policy
> > related to it to vfs.
> > 
> > The series was developed with discussions and guidance from
> > Arnd Bergmann.
> > 
> > The original idea for the series was the discussion:
> > https://lkml.org/lkml/2014/5/30/551
> > 
> > Patches 5 and 6 can be merged only after vfs is transitioned
> > to use 64 bit timestamps as noted in the respective commit
> > texts.
> > 
> > The series only includes adding range limits to filesystems:
> > ext4 and afs as examples to keep the series simple.
> > Every filesystem will be updated to add these limits.
> 
> We're going to need regression tests for this to ensure that it
> works properly and that we don't inadvertantly break it in future.
> Can you write some xfstests that exercise this functionality and
> validate that the mount behaviour, clamping and range limiting is
> working as intended?

Seconded. :)

I guess the only way to tell if a mountpoint can do 64 bit times is to
try it and see what happens?  Unless you enable the procfs thing that
prints to dmesg.  Evidently turning on the knob won't cause complaints
if there's already a mounted fs that didn't have 64-bit time support.
I'd go look at the testcases to corroborate this, but I don't know
that there are any?

(It was a big help to write a big pile of tests for adding reflink to
XFS.  It helped us find some btrfs reflink bugs too.)

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> da...@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038