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;
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) {
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.
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
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
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.
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
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.
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
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.
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);
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
-
Arnd Bergmann -
Christoph Hellwig -
Darrick J. Wong