On Thu, Dec 13, 2018 at 02:53:52PM +1100, Dave Chinner wrote:
> On Tue, Dec 11, 2018 at 11:56:33PM -0500, Nick Bowler wrote:
> > On 12/11/18, Brian Foster <bfoster@xxxxxxxxxx> wrote:
> > > On Tue, Dec 11, 2018 at 02:04:48AM -0500, Nick Bowler wrote:
> > >> Hi Dave,
> > >>
> > >> On 2018-12-10, Dave Chinner <david@xxxxxxxxxxxxx> 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".
You(r distro) can enable X32 in the x86_64 kernel even if you never use
it, so this as proposed would break XFS on amd64. I'd rather just have
something like this in xfs_file_ioctl, and gated on is_x32_syscall().
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
