From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp2130.oracle.com ([156.151.31.86]:39636 "EHLO userp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728077AbeLMQaV (ORCPT ); Thu, 13 Dec 2018 11:30:21 -0500 Date: Thu, 13 Dec 2018 08:30:08 -0800 From: "Darrick J. Wong" Subject: Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device Message-ID: <20181213163008.GC24487@magnolia> References: <20181210161121.GC8356@bfoster> <20181210165020.GT24487@magnolia> <20181210174627.GD8356@bfoster> <20181210214115.GC6311@dastard> <20181211122701.GA2819@bfoster> <20181213035352.GF6311@dastard> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181213035352.GF6311@dastard> Sender: linux-xfs-owner@vger.kernel.org List-ID: List-Id: xfs To: Dave Chinner Cc: Nick Bowler , Brian Foster , linux-xfs@vger.kernel.org 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 wrote: > > > On Tue, Dec 11, 2018 at 02:04:48AM -0500, Nick Bowler wrote: > > >> Hi Dave, > > >> > > >> On 2018-12-10, Dave Chinner 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@fromorbit.com