On Wed, Nov 13, 2019 at 6:08 AM Darrick J. Wong <darrick.wong@xxxxxxxxxx> 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)
+{
+ 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,
@@ -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.
> > > 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 <hch@xxxxxx>
>
> Seconded,
> Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
Thanks!
Arnd
