linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] xfs: rename compat_time_t to old_time32_t
@ 2019-12-18 16:39 Arnd Bergmann
  2019-12-18 16:39 ` [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time Arnd Bergmann
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Arnd Bergmann @ 2019-12-18 16:39 UTC (permalink / raw)
  To: Darrick J . Wong, linux-xfs
  Cc: y2038, Arnd Bergmann, Brian Foster, Eric Sandeen,
	Allison Collins, Nick Bowler, linux-kernel

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.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 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_t		sec32;	/* tv_sec differs on 64 vs. 32 */
 
 	if (get_user(sec32,		&bstime32->tv_sec)	||
 	    get_user(bstime->tv_nsec,	&bstime32->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_t	tv_sec;		/* seconds		*/
 	__s32		tv_nsec;	/* and nanoseconds	*/
 } compat_xfs_bstime_t;
 
-- 
2.20.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time
  2019-12-18 16:39 [PATCH 1/3] xfs: rename compat_time_t to old_time32_t Arnd Bergmann
@ 2019-12-18 16:39 ` Arnd Bergmann
  2019-12-24  8:45   ` Christoph Hellwig
  2019-12-18 16:44 ` [PATCH 3/3] xfs: quota: move to time64_t interfaces Arnd Bergmann
  2019-12-24  8:39 ` [PATCH 1/3] xfs: rename compat_time_t to old_time32_t Christoph Hellwig
  2 siblings, 1 reply; 12+ messages in thread
From: Arnd Bergmann @ 2019-12-18 16:39 UTC (permalink / raw)
  To: Darrick J . Wong, linux-xfs
  Cc: y2038, Arnd Bergmann, Brian Foster, Dave Chinner,
	Allison Collins, Jan Kara, Eric Sandeen, linux-kernel

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 once the extended times are supported.

Any application using these needs to be updated to use the v5
interfaces.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 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..d43582e933a0 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 <linux/compat.h>
 #include <linux/mount.h>
 #include <linux/namei.h>
 
@@ -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)
+{
+	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)) {
+		error = -EINVAL;
+		goto out;
+	}
+
 	/* Pull information for the target fd */
 	f = fdget((int)sxp->sx_fdtarget);
 	if (!f.file) {
-- 
2.20.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/3] xfs: quota: move to time64_t interfaces
  2019-12-18 16:39 [PATCH 1/3] xfs: rename compat_time_t to old_time32_t Arnd Bergmann
  2019-12-18 16:39 ` [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time Arnd Bergmann
@ 2019-12-18 16:44 ` Arnd Bergmann
  2019-12-24  8:39 ` [PATCH 1/3] xfs: rename compat_time_t to old_time32_t Christoph Hellwig
  2 siblings, 0 replies; 12+ messages in thread
From: Arnd Bergmann @ 2019-12-18 16:44 UTC (permalink / raw)
  To: Darrick J . Wong, linux-xfs
  Cc: y2038, Arnd Bergmann, Brian Foster, Carlos Maiolino,
	Pavel Reichl, Eric Sandeen, Tetsuo Handa, Dave Chinner,
	Allison Collins, Jan Kara, linux-kernel

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 <arnd@arndb.de>
---
 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->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 = q->qi_rtbwarnlimit;
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index a6fe2d8dc40f..d1b9869bc5fa 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -580,7 +580,7 @@ xfs_trans_dqresv(
 {
 	xfs_qcnt_t		hardlimit;
 	xfs_qcnt_t		softlimit;
-	time_t			timer;
+	time64_t		timer;
 	xfs_qwarncnt_t		warns;
 	xfs_qwarncnt_t		warnlimit;
 	xfs_qcnt_t		total_count;
@@ -635,7 +635,8 @@ xfs_trans_dqresv(
 				goto error_return;
 			}
 			if (softlimit && total_count > softlimit) {
-				if ((timer != 0 && get_seconds() > timer) ||
+				if ((timer != 0 &&
+				     ktime_get_real_seconds() > timer) ||
 				    (warns != 0 && warns >= warnlimit)) {
 					xfs_quota_warn(mp, dqp,
 						       QUOTA_NL_BSOFTLONGWARN);
@@ -662,7 +663,8 @@ xfs_trans_dqresv(
 				goto error_return;
 			}
 			if (softlimit && total_count > softlimit) {
-				if  ((timer != 0 && get_seconds() > timer) ||
+				if  ((timer != 0 &&
+				      ktime_get_real_seconds() > timer) ||
 				     (warns != 0 && warns >= warnlimit)) {
 					xfs_quota_warn(mp, dqp,
 						       QUOTA_NL_ISOFTLONGWARN);
-- 
2.20.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] xfs: rename compat_time_t to old_time32_t
  2019-12-18 16:39 [PATCH 1/3] xfs: rename compat_time_t to old_time32_t Arnd Bergmann
  2019-12-18 16:39 ` [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time Arnd Bergmann
  2019-12-18 16:44 ` [PATCH 3/3] xfs: quota: move to time64_t interfaces Arnd Bergmann
@ 2019-12-24  8:39 ` Christoph Hellwig
  2 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2019-12-24  8:39 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Darrick J . Wong, linux-xfs, y2038, Brian Foster, Eric Sandeen,
	Allison Collins, Nick Bowler, linux-kernel

On Wed, Dec 18, 2019 at 05:39:28PM +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.
> 
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time
  2019-12-18 16:39 ` [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time Arnd Bergmann
@ 2019-12-24  8:45   ` Christoph Hellwig
  2020-01-02  9:16     ` Arnd Bergmann
  0 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2019-12-24  8:45 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Darrick J . Wong, linux-xfs, y2038, Brian Foster, Dave Chinner,
	Allison Collins, Jan Kara, Eric Sandeen, linux-kernel

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.


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

Here we can simply check for cmd != XFS_IOC_FSINUMBERS before the call.

>  	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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time
  2019-12-24  8:45   ` Christoph Hellwig
@ 2020-01-02  9:16     ` Arnd Bergmann
  2020-01-02 18:07       ` Darrick J. Wong
  2020-01-02 20:34       ` Arnd Bergmann
  0 siblings, 2 replies; 12+ messages in thread
From: Arnd Bergmann @ 2020-01-02  9:16 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Darrick J . Wong, linux-xfs, y2038 Mailman List, Brian Foster,
	Dave Chinner, Allison Collins, Jan Kara, Eric Sandeen,
	linux-kernel

On Tue, Dec 24, 2019 at 9:45 AM Christoph Hellwig <hch@infradead.org> 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.

       Arnd

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time
  2020-01-02  9:16     ` Arnd Bergmann
@ 2020-01-02 18:07       ` Darrick J. Wong
  2020-01-07 14:16         ` Christoph Hellwig
  2020-01-02 20:34       ` Arnd Bergmann
  1 sibling, 1 reply; 12+ messages in thread
From: Darrick J. Wong @ 2020-01-02 18:07 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Christoph Hellwig, linux-xfs, y2038 Mailman List, Brian Foster,
	Dave Chinner, Allison Collins, Jan Kara, Eric Sandeen,
	linux-kernel

On Thu, Jan 02, 2020 at 10:16:21AM +0100, Arnd Bergmann wrote:
> On Tue, Dec 24, 2019 at 9:45 AM Christoph Hellwig <hch@infradead.org> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time
  2020-01-02  9:16     ` Arnd Bergmann
  2020-01-02 18:07       ` Darrick J. Wong
@ 2020-01-02 20:34       ` Arnd Bergmann
  2020-01-07 14:15         ` Christoph Hellwig
  1 sibling, 1 reply; 12+ messages in thread
From: Arnd Bergmann @ 2020-01-02 20:34 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Darrick J . Wong, linux-xfs, y2038 Mailman List, Brian Foster,
	Dave Chinner, Allison Collins, Jan Kara, Eric Sandeen,
	linux-kernel

On Thu, Jan 2, 2020 at 10:16 AM Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Dec 24, 2019 at 9:45 AM Christoph Hellwig <hch@infradead.org> 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.

I tried adding the helper now but ran into a stupid problem: the best
place to put it would be linux/time32.h, but then I have to include
linux/compat.h from there, which in turn pulls in tons of other
headers in any file using linux/time.h.

I considered making it a macro instead, but that's also really ugly.

I now think we should just defer this change until after v5.6, once I
have separated linux/time.h from linux/time32.h.
In the meantime I'll resend the other two patches that I know we
need in v5.6 in order to get there, so Darrick can apply them to his
tree.

      Arnd

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time
  2020-01-02 20:34       ` Arnd Bergmann
@ 2020-01-07 14:15         ` Christoph Hellwig
  0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2020-01-07 14:15 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Christoph Hellwig, Darrick J . Wong, linux-xfs,
	y2038 Mailman List, Brian Foster, Dave Chinner, Allison Collins,
	Jan Kara, Eric Sandeen, linux-kernel

On Thu, Jan 02, 2020 at 09:34:48PM +0100, Arnd Bergmann wrote:
> I tried adding the helper now but ran into a stupid problem: the best
> place to put it would be linux/time32.h, but then I have to include
> linux/compat.h from there, which in turn pulls in tons of other
> headers in any file using linux/time.h.
> 
> I considered making it a macro instead, but that's also really ugly.
> 
> I now think we should just defer this change until after v5.6, once I
> have separated linux/time.h from linux/time32.h.
> In the meantime I'll resend the other two patches that I know we
> need in v5.6 in order to get there, so Darrick can apply them to his
> tree.

Sounds good.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time
  2020-01-02 18:07       ` Darrick J. Wong
@ 2020-01-07 14:16         ` Christoph Hellwig
  2020-01-07 18:16           ` Darrick J. Wong
  0 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2020-01-07 14:16 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Arnd Bergmann, Christoph Hellwig, linux-xfs, y2038 Mailman List,
	Brian Foster, Dave Chinner, Allison Collins, Jan Kara,
	Eric Sandeen, linux-kernel

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time
  2020-01-07 14:16         ` Christoph Hellwig
@ 2020-01-07 18:16           ` Darrick J. Wong
  2020-01-08  8:57             ` Christoph Hellwig
  0 siblings, 1 reply; 12+ messages in thread
From: Darrick J. Wong @ 2020-01-07 18:16 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Arnd Bergmann, linux-xfs, y2038 Mailman List, Brian Foster,
	Dave Chinner, Allison Collins, Jan Kara, Eric Sandeen,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time
  2020-01-07 18:16           ` Darrick J. Wong
@ 2020-01-08  8:57             ` Christoph Hellwig
  0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2020-01-08  8:57 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Christoph Hellwig, Arnd Bergmann, linux-xfs, y2038 Mailman List,
	Brian Foster, Dave Chinner, Allison Collins, Jan Kara,
	Eric Sandeen, linux-kernel

On Tue, Jan 07, 2020 at 10:16:14AM -0800, Darrick J. Wong wrote:
> 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.

Well, we should eventually end up with a common defrag tool (e.g. in
util-linux).  We might as well start of with the xfs_fsr codebase
for that or whatever suits us best.

> 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?

That sounds like the most useful common API.

> ...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?

Sure.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-01-08  8:57 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-18 16:39 [PATCH 1/3] xfs: rename compat_time_t to old_time32_t Arnd Bergmann
2019-12-18 16:39 ` [PATCH 2/3] xfs: disallow broken ioctls without compat-32-bit-time Arnd Bergmann
2019-12-24  8:45   ` Christoph Hellwig
2020-01-02  9:16     ` Arnd Bergmann
2020-01-02 18:07       ` Darrick J. Wong
2020-01-07 14:16         ` Christoph Hellwig
2020-01-07 18:16           ` Darrick J. Wong
2020-01-08  8:57             ` Christoph Hellwig
2020-01-02 20:34       ` Arnd Bergmann
2020-01-07 14:15         ` Christoph Hellwig
2019-12-18 16:44 ` [PATCH 3/3] xfs: quota: move to time64_t interfaces Arnd Bergmann
2019-12-24  8:39 ` [PATCH 1/3] xfs: rename compat_time_t to old_time32_t Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).