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