On 12/11/18, Brian Foster <bfoster@xxxxxxxxxx> wrote: > On Tue, Dec 11, 2018 at 02:04:48AM -0500, Nick Bowler wrote: >> 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. The ifdeffery will mean the "native" ioctls number only get accepted in the compat case on select architectures. In principle it could be removed and both "native" and "compat" ioctl numbers would be accepted all the time in the compat case (so the possible impact would mainly be on non-x86 and hopefully would be limited to code size and/or "some ioctl numbers gave errors before and now don't") I think it's a separate discussion for whether that's prudent or not. Current code has the ifdeffery. Also since it's a syntax error to have multiple case labels with the same value it'd be essential to validate that all supported architectures architectures end up with different values for each XFS_IOC_xyz and the corresponding XFS_IOC_xyz_32. > Note that verification doesn't just mean checking whether your > particular configuration works, but also that nothing has broken on > other potentially affected configurations. For example, I'd suggest that > this kind of change at least warrants an additional regression test of > 32-bit (x86) userspace on a 64-bit kernel that has CONFIG_X86_X32 > enabled so we can make sure that we haven't disrupted anything in the > compat_ioctl() path for the non-x32 case. Indeed. I will run xfstests in a vm (thanks for the instructions) and report before/after results for x32-on-x86_64 and i686-on-x86_64. Cheers, Nick
