| Message ID | 20191218163954.296726-2-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] xfs: rename compat_time_t to old_time32_t | expand |
On Wed, Dec 18, 2019 at 05:39:29PM +0100, Arnd Bergmann wrote: > +/* 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; I think the check for the individual command belongs into the callers, which laves us with: static inline bool have_time32(void) { return IS_ENABLED(CONFIG_COMPAT_32BIT_TIME) || (IS_ENABLED(CONFIG_64BIT) && !in_compat_syscall()); } and that looks like it should be in a generic helper somewhere. > STATIC int > xfs_ioc_fsbulkstat( > xfs_mount_t *mp, > @@ -637,6 +655,9 @@ xfs_ioc_fsbulkstat( > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > > + if (!xfs_have_compat_bstat_time32(cmd)) > + return -EINVAL; Here we can simply check for cmd != XFS_IOC_FSINUMBERS before the call. > if (XFS_FORCED_SHUTDOWN(mp)) > return -EIO; > > @@ -1815,6 +1836,11 @@ xfs_ioc_swapext( > struct fd f, tmp; > int error = 0; > > + if (!xfs_have_compat_bstat_time32(XFS_IOC_SWAPEXT)) { > + error = -EINVAL; > + goto out; > + } And for this one we just have one cmd anyway. But I actually still disagree with the old_time check for this one entirely, as voiced on one of the last iterations. For swapext the time stamp really is only used as a generation counter, so overflows are entirely harmless.
On Tue, Dec 24, 2019 at 9:45 AM Christoph Hellwig <hch@infradead.org> wrote: > On Wed, Dec 18, 2019 at 05:39:29PM +0100, Arnd Bergmann wrote: > > +/* 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; > > I think the check for the individual command belongs into the callers, > which laves us with: > > static inline bool have_time32(void) > { > return IS_ENABLED(CONFIG_COMPAT_32BIT_TIME) || > (IS_ENABLED(CONFIG_64BIT) && !in_compat_syscall()); > } > > and that looks like it should be in a generic helper somewhere. Yes, makes sense. I was going for something XFS specific here because XFS is unique in the kernel in completely deprecating a set of ioctl commands (replacing the old interface with a v5) rather than allowing the user space to be compiled with 64-bit time_t. If we add a global helper for this, I'd be tempted to also stick a WARN_RATELIMIT() in there to give users a better indication of what broke after disabling CONFIG_COMPAT_32BIT_TIME. The same warning would make sense in the system calls, but then we have to decide which combinations we want to allow being configured at runtime or compile-time. a) unmodified behavior b) just warn but allow c) no warning but disallow d) warn and disallow > > if (XFS_FORCED_SHUTDOWN(mp)) > > return -EIO; > > > > @@ -1815,6 +1836,11 @@ xfs_ioc_swapext( > > struct fd f, tmp; > > int error = 0; > > > > + if (!xfs_have_compat_bstat_time32(XFS_IOC_SWAPEXT)) { > > + error = -EINVAL; > > + goto out; > > + } > > And for this one we just have one cmd anyway. But I actually still > disagree with the old_time check for this one entirely, as voiced on > one of the last iterations. For swapext the time stamp really is > only used as a generation counter, so overflows are entirely harmless. Sorry I missed that comment earlier. I've had a fresh look now, but I think we still need to deprecate XFS_IOC_SWAPEXT and add a v5 version of it, since the comparison will fail as soon as the range of the inode timestamps is extended beyond 2038, otherwise the comparison will always be false, or require comparing the truncated time values which would add yet another representation. Arnd
On Thu, Jan 02, 2020 at 10:16:21AM +0100, Arnd Bergmann wrote: > On Tue, Dec 24, 2019 at 9:45 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Wed, Dec 18, 2019 at 05:39:29PM +0100, Arnd Bergmann wrote: > > > +/* 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; > > > > I think the check for the individual command belongs into the callers, > > which laves us with: > > > > static inline bool have_time32(void) > > { > > return IS_ENABLED(CONFIG_COMPAT_32BIT_TIME) || > > (IS_ENABLED(CONFIG_64BIT) && !in_compat_syscall()); > > } > > > > and that looks like it should be in a generic helper somewhere. > > Yes, makes sense. > > I was going for something XFS specific here because XFS is unique in the > kernel in completely deprecating a set of ioctl commands (replacing > the old interface with a v5) rather than allowing the user space to be > compiled with 64-bit time_t. > > If we add a global helper for this, I'd be tempted to also stick a > WARN_RATELIMIT() in there to give users a better indication of > what broke after disabling CONFIG_COMPAT_32BIT_TIME. > > The same warning would make sense in the system calls, but then > we have to decide which combinations we want to allow being > configured at runtime or compile-time. > > a) unmodified behavior > b) just warn but allow > c) no warning but disallow > d) warn and disallow > > > > if (XFS_FORCED_SHUTDOWN(mp)) > > > return -EIO; > > > > > > @@ -1815,6 +1836,11 @@ xfs_ioc_swapext( > > > struct fd f, tmp; > > > int error = 0; > > > > > > + if (!xfs_have_compat_bstat_time32(XFS_IOC_SWAPEXT)) { > > > + error = -EINVAL; > > > + goto out; > > > + } > > > > And for this one we just have one cmd anyway. But I actually still > > disagree with the old_time check for this one entirely, as voiced on > > one of the last iterations. For swapext the time stamp really is > > only used as a generation counter, so overflows are entirely harmless. > > Sorry I missed that comment earlier. I've had a fresh look now, but > I think we still need to deprecate XFS_IOC_SWAPEXT and add a > v5 version of it, since the comparison will fail as soon as the range > of the inode timestamps is extended beyond 2038, otherwise the > comparison will always be false, or require comparing the truncated > time values which would add yet another representation. I prefer we replace the old SWAPEXT with a new version to get rid of struct xfs_bstat. Though a SWAPEXT_V5 probably only needs to contain the *stat fields that swapext actually needs to check that the file hasn't been changed, which would be ino/gen/btime/ctime. (Maybe I'd add an offset/length too...) --D > Arnd
On Thu, Jan 2, 2020 at 10:16 AM Arnd Bergmann <arnd@arndb.de> wrote: > On Tue, Dec 24, 2019 at 9:45 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Wed, Dec 18, 2019 at 05:39:29PM +0100, Arnd Bergmann wrote: > > > +/* 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; > > > > I think the check for the individual command belongs into the callers, > > which laves us with: > > > > static inline bool have_time32(void) > > { > > return IS_ENABLED(CONFIG_COMPAT_32BIT_TIME) || > > (IS_ENABLED(CONFIG_64BIT) && !in_compat_syscall()); > > } > > > > and that looks like it should be in a generic helper somewhere. > > Yes, makes sense. > > I was going for something XFS specific here because XFS is unique in the > kernel in completely deprecating a set of ioctl commands (replacing > the old interface with a v5) rather than allowing the user space to be > compiled with 64-bit time_t. I tried adding the helper now but ran into a stupid problem: the best place to put it would be linux/time32.h, but then I have to include linux/compat.h from there, which in turn pulls in tons of other headers in any file using linux/time.h. I considered making it a macro instead, but that's also really ugly. I now think we should just defer this change until after v5.6, once I have separated linux/time.h from linux/time32.h. In the meantime I'll resend the other two patches that I know we need in v5.6 in order to get there, so Darrick can apply them to his tree. Arnd
On Thu, Jan 02, 2020 at 09:34:48PM +0100, Arnd Bergmann wrote: > I tried adding the helper now but ran into a stupid problem: the best > place to put it would be linux/time32.h, but then I have to include > linux/compat.h from there, which in turn pulls in tons of other > headers in any file using linux/time.h. > > I considered making it a macro instead, but that's also really ugly. > > I now think we should just defer this change until after v5.6, once I > have separated linux/time.h from linux/time32.h. > In the meantime I'll resend the other two patches that I know we > need in v5.6 in order to get there, so Darrick can apply them to his > tree. Sounds good.
On Thu, Jan 02, 2020 at 10:07:49AM -0800, Darrick J. Wong wrote: > > Sorry I missed that comment earlier. I've had a fresh look now, but > > I think we still need to deprecate XFS_IOC_SWAPEXT and add a > > v5 version of it, since the comparison will fail as soon as the range > > of the inode timestamps is extended beyond 2038, otherwise the > > comparison will always be false, or require comparing the truncated > > time values which would add yet another representation. > > I prefer we replace the old SWAPEXT with a new version to get rid of > struct xfs_bstat. Though a SWAPEXT_V5 probably only needs to contain > the *stat fields that swapext actually needs to check that the file > hasn't been changed, which would be ino/gen/btime/ctime. > > (Maybe I'd add an offset/length too...) And most importantly we need to lift it to the VFS instead of all the crazy fs specific interfaces at the moment.
On Tue, Jan 07, 2020 at 06:16:34AM -0800, Christoph Hellwig wrote: > On Thu, Jan 02, 2020 at 10:07:49AM -0800, Darrick J. Wong wrote: > > > Sorry I missed that comment earlier. I've had a fresh look now, but > > > I think we still need to deprecate XFS_IOC_SWAPEXT and add a > > > v5 version of it, since the comparison will fail as soon as the range > > > of the inode timestamps is extended beyond 2038, otherwise the > > > comparison will always be false, or require comparing the truncated > > > time values which would add yet another representation. > > > > I prefer we replace the old SWAPEXT with a new version to get rid of > > struct xfs_bstat. Though a SWAPEXT_V5 probably only needs to contain > > the *stat fields that swapext actually needs to check that the file > > hasn't been changed, which would be ino/gen/btime/ctime. > > > > (Maybe I'd add an offset/length too...) > > And most importantly we need to lift it to the VFS instead of all the > crazy fs specific interfaces at the moment. Yeah. Fixing that (and maybe adding an ioctl to set the FS UUID online) were on my list for 5.6 but clearly I have to defer everything until 5.7 because we've just run out of time. Uh... I started looking into unifying the ext4 and xfs defrag ioctl, but gagged when I realized that the ext4 ioctl also handles the data copy inside the kernel. I think that's the sort of behavior we should /not/ allow into the new ioctl, though that also means that the required changes for ext4/e4defrag will be non-trivial. The btrfs defrag ioctl also contains thresholding information and optional knobs to enable compression, which makes me wonder if we should focus narrowly on swapext being "swap these extents but only if the source file hasn't changed" and not necessarily defrag? ...in which case I wonder, can people (ab)use this interface for atomic file updates? Create an O_TMPFILE, reflink the source file into the temp file, make your updates to the tempfile, and then swapext the donor's file data back into the source file, but only if the source file hasn't changed? --D
On Tue, Jan 07, 2020 at 10:16:14AM -0800, Darrick J. Wong wrote: > Yeah. Fixing that (and maybe adding an ioctl to set the FS UUID online) > were on my list for 5.6 but clearly I have to defer everything until 5.7 > because we've just run out of time. > > Uh... I started looking into unifying the ext4 and xfs defrag ioctl, but > gagged when I realized that the ext4 ioctl also handles the data copy > inside the kernel. I think that's the sort of behavior we should /not/ > allow into the new ioctl, though that also means that the required > changes for ext4/e4defrag will be non-trivial. Well, we should eventually end up with a common defrag tool (e.g. in util-linux). We might as well start of with the xfs_fsr codebase for that or whatever suits us best. > The btrfs defrag ioctl also contains thresholding information and > optional knobs to enable compression, which makes me wonder if we should > focus narrowly on swapext being "swap these extents but only if the > source file hasn't changed" and not necessarily defrag? That sounds like the most useful common API. > ...in which case I wonder, can people (ab)use this interface for atomic > file updates? Create an O_TMPFILE, reflink the source file into the > temp file, make your updates to the tempfile, and then swapext the > donor's file data back into the source file, but only if the source file > hasn't changed? Sure.
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 7b35d62ede9f..d43582e933a0 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -36,6 +36,7 @@ #include "xfs_reflink.h" #include "xfs_ioctl.h" +#include <linux/compat.h> #include <linux/mount.h> #include <linux/namei.h> @@ -617,6 +618,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, @@ -637,6 +655,9 @@ xfs_ioc_fsbulkstat( if (!capable(CAP_SYS_ADMIN)) return -EPERM; + if (!xfs_have_compat_bstat_time32(cmd)) + return -EINVAL; + if (XFS_FORCED_SHUTDOWN(mp)) return -EIO; @@ -1815,6 +1836,11 @@ xfs_ioc_swapext( struct fd f, tmp; int error = 0; + if (!xfs_have_compat_bstat_time32(XFS_IOC_SWAPEXT)) { + error = -EINVAL; + goto out; + } + /* Pull information for the target fd */ f = fdget((int)sxp->sx_fdtarget); if (!f.file) {
When building a kernel that disables support for 32-bit time_t system calls, it also makes sense to disable the old xfs_bstat ioctls completely, as they truncate the timestamps to 32-bit values once the extended times are supported. Any application using these needs to be updated to use the v5 interfaces. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- fs/xfs/xfs_ioctl.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)