Re: [PATCH v2 20/24] xfs: disallow broken ioctls without compat-32-bit-time

[Darrick J. Wong]

On Mon, Dec 16, 2019 at 05:45:29PM +0100, Arnd Bergmann wrote:
> On Fri, Dec 13, 2019 at 10:05 PM Darrick J. Wong
> <darrick.wong@xxxxxxxxxx> wrote:
> >
> > On Fri, Dec 13, 2019 at 09:53:48PM +0100, Arnd Bergmann wrote:
> > > 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.
> >
> > Note that current xfs doesn't support > 32-bit timestamps at all, so for
> > now the old bulkstat/swapext ioctls will never overflow.
> 
> Right, this patch originally came after my version of the 40-bit
> timestamps that I dropped from the series now.
> 
> I've added "... once the extended times are supported." above now.
> 
> > Granted, I melded everyone's suggestions into a more fully formed
> > 'bigtime' feature patchset that I'll dump out soon as part of my usual
> > end of year carpetbombing of the mailing list, so we likely still need
> > most of this patch anyway...
> 
> What is the timeline for that work now? I'm mainly interested in
> getting the removal of 'time_t/timeval/timespec' and 'get_seconds()'
> from the kernel done for v5.6, but it would be good to also have
> this patch and the extended timestamps in the same version
> just so we can claim that "all known y2038 issues" are addressed
> in that release (I'm sure we will run into bugs we don't know yet).

Personally, I think you should push this whenever it's ready.  Are you
aiming to send all 24 patches as a treewide pull request directly to
Linus, or would you rather the 2-3 xfs patches go through the xfs tree?

The y2038 format changes are going to take a while to push through
review.  If somehow it all gets through review for 5.6 I can always
apply both and fix the merge damage, but more likely y2038 timestamps is
a <cough> 5.8 EXPERIMENTAL thing.

Or later, given that Dave and I both have years worth of unreviewed
patch backlog. :(

> > > @@ -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)
> >
> > The v5 bulkstat ioctls follow an entirely separate path through
> > xfs_ioctl.c, so I think you don't need the @cmd parameter.
> 
> The check is there to not forbid XFS_IOC_FSINUMBERS at
> the moment, since that is not affected.

Aha.

> > > @@ -1815,6 +1836,11 @@ xfs_ioc_swapext(
> > >       struct fd       f, tmp;
> > >       int             error = 0;
> > >
> > > +     if (xfs_have_compat_bstat_time32(XFS_IOC_SWAPEXT)) {
> >
> > if (!xfs_have...()) ?
> 
> Right, fixed now.

<nod>

--D

>        Arnd