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 <darrick.w...@oracle.com> > 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(&mp->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 <h...@lst.de> > > > > Seconded, > > Reviewed-by: Darrick J. Wong <darrick.w...@oracle.com> > > Thanks! > > Arnd _______________________________________________ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038