Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device

[Dave Chinner <david@fromorbit.com>]

On Tue, Dec 11, 2018 at 11:56:33PM -0500, Nick Bowler wrote:
> On 12/11/18, Brian Foster <bfoster@redhat.com> wrote:
> > On Tue, Dec 11, 2018 at 02:04:48AM -0500, Nick Bowler wrote:
> >> Hi Dave,
> >>
> >> On 2018-12-10, Dave Chinner <david@fromorbit.com> wrote:
> >> > On Mon, Dec 10, 2018 at 03:54:47PM -0500, Nick Bowler wrote:
> >> >> I can have a go at fixing the FSGEOMETRY ioctl too (and submit it
> >> >> properly) if this approach seems reasonable.  Possibly other things
> >> >> may be broken too but I haven't hit any other issues yet in my XFS
> >> >> adventure.
> >> >
> >> > We really need to audit all the compat ioctls for this same
> >> > problem and fix all of them in one go, not just slap a bandaid on
> >> > the messenger and ignore the rest....
> >>
> >> OK then.  This patch should cover all of them.  However, I wouldn't know
> >> where to start with verification of a change like this, since I don't
> >> know what these ioctls actually do, but xfs_growfs does seem to work for
> >> me now on a test filesystem with this applied.
> >>
> >
> > Given that the structure size essentially changes the command value, I'm
> > kind of curious why we have this ifdeffery in the first place. That
> > aside, the patch seems reasonable to me at a glance (though some brief
> > comments around the ifdefs would be nice).
> 
> OK, xfstests has revealed some trouble with the three "bulkstat" ioctls,
> since while the xfs_bulkstat structure itself is fine, one of its members
> is used as a pointer to various structures which are not fine.  This
> wasn't too hard to fix though.

IIRC, there's bigger problems than you realise here - the bulkstat
structure has embedded timestamps in them and on x32 struct timeval
doesn't match either ia32 or x86-64. i.e. on ia32, struct timeval is
8 bytes, on x86-64 it is 16 bytes, and in x32 it is 12 bytes.

IOWs, using the x86-64 handlers for bulkstat is wrong, as is using
the compat handlers. That's one of the reasons why x32 is such a
Charlie Foxtrot when it comes to compat handlers - we basically have
to audit ioctl structures one by one with pahole to determine which
arch version they *may* be compatible with.

And then there is testing that we get identical output from all
three versions for each ioctl.

Right now, I'd much prefer we simply put this at the start of
xfs_fs_fill_super():

#ifdef CONFIG_X86_X32
	xfs_warn("XFS not supported on x32 architectures")
	return -ENOSYS;
#endif

Or, alternatively, tag it as EXPERIMENTAL and "use at your own
risk".

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com