[RFC,1/5] xfs: [variant A] avoid time_t in user api
diff mbox series

Message ID 20191112120910.1977003-2-arnd@arndb.de
State Accepted
Headers show
Series
  • xfs: y2038 conversion
Related show

Commit Message

Arnd Bergmann Nov. 12, 2019, 12:09 p.m. UTC
The ioctl definitions for XFS_IOC_SWAPEXT, XFS_IOC_FSBULKSTAT and
XFS_IOC_FSBULKSTAT_SINGLE are part of libxfs and based on time_t.

The definition for time_t differs between current kernels and coming
32-bit libc variants that define it as 64-bit. For most ioctls, that
means the kernel has to be able to handle two different command codes
based on the different structure sizes.

The same solution could be applied for XFS_IOC_SWAPEXT, but it would
not work for XFS_IOC_FSBULKSTAT and XFS_IOC_FSBULKSTAT_SINGLE because
the structure with the time_t is passed through an indirect pointer,
and the command number itself is based on struct xfs_fsop_bulkreq,
which does not differ based on time_t.

This means any solution that can be applied requires a change of the
ABI definition in the xfs_fs.h header file, as well as doing the same
change in any user application that contains a copy of this header.

The usual solution would be to define a replacement structure and
use conditional compilation for the ioctl command codes to use
one or the other, such as

 #define XFS_IOC_FSBULKSTAT_OLD _IOWR('X', 101, struct xfs_fsop_bulkreq)
 #define XFS_IOC_FSBULKSTAT_NEW _IOWR('X', 129, struct xfs_fsop_bulkreq)
 #define XFS_IOC_FSBULKSTAT ((sizeof(time_t) == sizeof(__kernel_long_t)) ? \
			     XFS_IOC_FSBULKSTAT_OLD : XFS_IOC_FSBULKSTAT_NEW)

After this, the kernel would be able to implement both
XFS_IOC_FSBULKSTAT_OLD and XFS_IOC_FSBULKSTAT_NEW handlers on
32-bit architectures with the correct ABI for either definition
of time_t.

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
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

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.

If either of the two assumptions is invalid, more discussion is needed
for coming up with a way to fix as much of the affected user space
code as possible.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/xfs/libxfs/xfs_fs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Christoph Hellwig Nov. 12, 2019, 2:16 p.m. UTC | #1
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.

XFS_IOC_SWAPEXT does not have a direct replacement, but the timestamp
is only used to verify that the file did not change vs the previous
stat.  So not being able to represent > 2038 times is not a real
problem anyway.

At some point we should probably look into a file system independent
defrag ioctl anyway, at which point we can deprecate XFS_IOC_SWAPEXT.

> 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>
Darrick J. Wong Nov. 13, 2019, 5:06 a.m. UTC | #2
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?

> XFS_IOC_SWAPEXT does not have a direct replacement, but the timestamp
> is only used to verify that the file did not change vs the previous
> stat.  So not being able to represent > 2038 times is not a real
> problem anyway.

Won't it become a problem when the tv_sec comparison in xfs_swap_extents
is type-promoted to the larger type but lacks the upper bits?

I guess we could force the check to the lower 32 bits on the assumption
that those are the most likely to change due to a file write.

I kinda want to do a SWAPEXT v5, fwiw....

> At some point we should probably look into a file system independent
> defrag ioctl anyway, at which point we can deprecate XFS_IOC_SWAPEXT.
> 
> > 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>

--D
Arnd Bergmann Nov. 13, 2019, 1:42 p.m. UTC | #3
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)
+{
+       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@lst.de>
>
> Seconded,
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks!

     Arnd
Darrick J. Wong Nov. 13, 2019, 4:34 p.m. UTC | #4
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
Arnd Bergmann Nov. 13, 2019, 5:14 p.m. UTC | #5
On Wed, Nov 13, 2019 at 5:34 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> 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:
> > 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?

I'd say probably not.

This would be for a kernel that intentionally only supports y2038-safe
user space to ensure that we don't get any surprises later. In that
configuration, I think we're still better off not allowing broken ioctls
regardless of whether the file system is also broken. ;-)

> > +       if (IS_ENABLED(CONFIG_COMPAT_32BIT_TIME))
>
> Heh, I didn't know that existed.

It barely does. At the moment it is always enabled on all architectures
except for 32-bit riscv, but I submitted a patch to make it user selectable
last week.

      Arnd

Patch
diff mbox series

diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
index e9371a8e0e26..4c4330f6e653 100644
--- a/fs/xfs/libxfs/xfs_fs.h
+++ b/fs/xfs/libxfs/xfs_fs.h
@@ -324,7 +324,7 @@  typedef struct xfs_growfs_rt {
  * Structures returned from ioctl XFS_IOC_FSBULKSTAT & XFS_IOC_FSBULKSTAT_SINGLE
  */
 typedef struct xfs_bstime {
-	time_t		tv_sec;		/* seconds		*/
+	__kernel_long_t tv_sec;		/* seconds		*/
 	__s32		tv_nsec;	/* and nanoseconds	*/
 } xfs_bstime_t;