Re: [RFC 1/5] xfs: [variant A] avoid time_t in user api

["Darrick J. Wong" <darrick.wong@oracle.com>]

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@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 <hch@lst.de>
> >
> > Seconded,
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Thanks!
> 
>      Arnd