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.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)
> +{
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 <hch@xxxxxx>
> >
> > Seconded,
> > Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> Thanks!
>
> Arnd
