* Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device @ 2018-12-10 4:29 Nick Bowler 2018-12-10 14:33 ` Brian Foster 0 siblings, 1 reply; 26+ messages in thread From: Nick Bowler @ 2018-12-10 4:29 UTC (permalink / raw) To: linux-xfs Hello, I'm a bit new to using XFS and I ran into some errors trying to enlarge a filesystem. This setup uses dmcrypt on top of md raid and I just reshaped the array to add additional storage. The underlying block device reflects the new size, but the filesystem hasn't been enlarged yet: # blockdev --report /dev/mapper/data RO RA SSZ BSZ StartSec Size Device rw 4096 512 4096 0 20001386921984 /dev/mapper/data # findmnt /dev/mapper/data TARGET SOURCE FSTYPE OPTIONS /mnt/data /dev/mapper/data xfs rw,relatime,attr2,inode64,sunit=1024,swidth=2048,noquota # df -h /mnt/data Filesystem Size Used Avail Use% Mounted on /dev/mapper/data 9.1T 8.5T 649G 94% /mnt/data So I read the manpage and it seems all I should need to do is run xfs_growfs on the mounted filesystem but... # xfs_growfs /mnt/data meta-data=/dev/mapper/data isize=512 agcount=32, agsize=76299136 blks = sectsz=4096 attr=2, projid32bit=1 = crc=1 finobt=1, sparse=1, rmapbt=0 = reflink=0 data = bsize=4096 blocks=2441572352, imaxpct=5 = sunit=128 swidth=256 blks naming =version 2 bsize=4096 ascii-ci=0, ftype=1 log =internal log bsize=4096 blocks=521728, version=2 = sectsz=4096 sunit=1 blks, lazy-count=1 realtime =none extsz=4096 blocks=0, rtextents=0 xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device xfs_growfs: XFS_IOC_FSGEOMETRY xfsctl failed: Inappropriate ioctl for device ... and the filesystem is not enlarged. Looking at strace output, the failing ioctls seem to be: openat(AT_FDCWD, "/mnt/data", O_RDONLY) = 3 [...] ioctl(3, _IOC(_IOC_WRITE, 0x58, 0x6e, 0x10), 0xffcc9a80) = -1 ENOTTY (Inappropriate ioctl for device) [...] ioctl(3, _IOC(_IOC_READ, 0x58, 0x64, 0x70), 0xffcc9ba0) = -1 ENOTTY (Inappropriate ioctl for device) Kernel version is 4.14.82 with xfsprogs 4.17.0, although I tried also with xfsprogs 4.19.0 and received the same errors. Am I missing something obvious here? What further steps should I take to help solve this? Thanks, Nick ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-10 4:29 Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device Nick Bowler @ 2018-12-10 14:33 ` Brian Foster 2018-12-10 15:39 ` Nick Bowler 0 siblings, 1 reply; 26+ messages in thread From: Brian Foster @ 2018-12-10 14:33 UTC (permalink / raw) To: Nick Bowler; +Cc: linux-xfs On Sun, Dec 09, 2018 at 11:29:04PM -0500, Nick Bowler wrote: > Hello, > > I'm a bit new to using XFS and I ran into some errors trying to enlarge > a filesystem. This setup uses dmcrypt on top of md raid and I just > reshaped the array to add additional storage. The underlying block > device reflects the new size, but the filesystem hasn't been enlarged > yet: > > # blockdev --report /dev/mapper/data > RO RA SSZ BSZ StartSec Size Device > rw 4096 512 4096 0 20001386921984 /dev/mapper/data > > # findmnt /dev/mapper/data > TARGET SOURCE FSTYPE OPTIONS > /mnt/data /dev/mapper/data xfs rw,relatime,attr2,inode64,sunit=1024,swidth=2048,noquota > > # df -h /mnt/data > Filesystem Size Used Avail Use% Mounted on > /dev/mapper/data 9.1T 8.5T 649G 94% /mnt/data > > So I read the manpage and it seems all I should need to do is run > xfs_growfs on the mounted filesystem but... > > # xfs_growfs /mnt/data > meta-data=/dev/mapper/data isize=512 agcount=32, agsize=76299136 blks > = sectsz=4096 attr=2, projid32bit=1 > = crc=1 finobt=1, sparse=1, rmapbt=0 > = reflink=0 > data = bsize=4096 blocks=2441572352, imaxpct=5 > = sunit=128 swidth=256 blks > naming =version 2 bsize=4096 ascii-ci=0, ftype=1 > log =internal log bsize=4096 blocks=521728, version=2 > = sectsz=4096 sunit=1 blks, lazy-count=1 > realtime =none extsz=4096 blocks=0, rtextents=0 > xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device > xfs_growfs: XFS_IOC_FSGEOMETRY xfsctl failed: Inappropriate ioctl for device > > ... and the filesystem is not enlarged. Looking at strace output, the > failing ioctls seem to be: > > openat(AT_FDCWD, "/mnt/data", O_RDONLY) = 3 > [...] > ioctl(3, _IOC(_IOC_WRITE, 0x58, 0x6e, 0x10), 0xffcc9a80) = -1 ENOTTY (Inappropriate ioctl for device) > [...] > ioctl(3, _IOC(_IOC_READ, 0x58, 0x64, 0x70), 0xffcc9ba0) = -1 ENOTTY (Inappropriate ioctl for device) > > Kernel version is 4.14.82 with xfsprogs 4.17.0, although I tried also > with xfsprogs 4.19.0 and received the same errors. > > Am I missing something obvious here? What further steps should I take > to help solve this? > The only thing that comes to mind while poking through the code is perhaps xfsprogs is sending the traditional XFS_IOC_GROWFSDATA command into the compat_ioctl() path somehow or another (assuming BROKEN_X86_ALIGNMENT is set). What arch is your kernel/xfsprogs? What does 'cat /sys/kernel/debug/trace/trace' show if you run 'trace-cmd start -e xfs:xfs_file*ioctl*' and then attempt the growfs? Brian > Thanks, > Nick ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-10 14:33 ` Brian Foster @ 2018-12-10 15:39 ` Nick Bowler 2018-12-10 16:11 ` Brian Foster 0 siblings, 1 reply; 26+ messages in thread From: Nick Bowler @ 2018-12-10 15:39 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs Hi Brian, On 12/10/18, Brian Foster <bfoster@redhat.com> wrote: > The only thing that comes to mind while poking through the code is > perhaps xfsprogs is sending the traditional XFS_IOC_GROWFSDATA command > into the compat_ioctl() path somehow or another (assuming > BROKEN_X86_ALIGNMENT is set). > > What arch is your kernel/xfsprogs? This system is running an amd64 kernel with x32 userspace (including xfsprogs). > What does 'cat /sys/kernel/debug/trace/trace' show if you run > 'trace-cmd start -e xfs:xfs_file*ioctl*' and then attempt the growfs? Looks like I don't have the required tracing enabled in my kernel configuration, but I can build a new one if needed. Is CONFIG_FTRACE sufficient for this? Thanks, Nick ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-10 15:39 ` Nick Bowler @ 2018-12-10 16:11 ` Brian Foster 2018-12-10 16:50 ` Darrick J. Wong 0 siblings, 1 reply; 26+ messages in thread From: Brian Foster @ 2018-12-10 16:11 UTC (permalink / raw) To: Nick Bowler; +Cc: linux-xfs On Mon, Dec 10, 2018 at 10:39:14AM -0500, Nick Bowler wrote: > Hi Brian, > > On 12/10/18, Brian Foster <bfoster@redhat.com> wrote: > > The only thing that comes to mind while poking through the code is > > perhaps xfsprogs is sending the traditional XFS_IOC_GROWFSDATA command > > into the compat_ioctl() path somehow or another (assuming > > BROKEN_X86_ALIGNMENT is set). > > > > What arch is your kernel/xfsprogs? > > This system is running an amd64 kernel with x32 userspace (including > xfsprogs). > Ok, so I think that means BROKEN_X86_ALIGNMENT should be set since XFS defines it as: #if defined(CONFIG_IA64) || defined(CONFIG_X86_64) #define BROKEN_X86_ALIGNMENT ... > > What does 'cat /sys/kernel/debug/trace/trace' show if you run > > 'trace-cmd start -e xfs:xfs_file*ioctl*' and then attempt the growfs? > > Looks like I don't have the required tracing enabled in my kernel > configuration, but I can build a new one if needed. Is CONFIG_FTRACE > sufficient for this? > Not sure. I think you need to have CONFIG_TRACING enabled, which may require FTRACE and/or some other options. Hmm, perhaps you'd be covered if you make sure you have CONFIG_DYNAMIC_FTRACE enabled. >From your strace output: ioctl(3, _IOC(_IOC_WRITE, 0x58, 0x6e, 0x10), 0xffcc9a80) = -1 ENOTTY (Inappropriate ioctl for device) 0x6e corresponds to the GROWFSDATA[_32] cmd and I think 0x10 is the size, which is 16 bytes as opposed to the 12 bytes expected for GROWFSDATA_32 for struct compat_xfs_growfs_data: typedef struct compat_xfs_growfs_data { __u64 newblocks; /* new data subvol size, fsblocks */ __u32 imaxpct; /* new inode space percentage limit */ } __attribute__((packed)) compat_xfs_growfs_data_t; On a 64-bit kernel, that packed attribute is the difference between expecting a padded 16 byte struct vs. a 12 byte version presumably from a 32-bit application. So if you are calling into the ->compat_ioctl() path I think the question is why is your xfsprogs sending the 16 byte structure? Brian > Thanks, > Nick ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-10 16:11 ` Brian Foster @ 2018-12-10 16:50 ` Darrick J. Wong 2018-12-10 16:55 ` Darrick J. Wong 2018-12-10 17:46 ` Brian Foster 0 siblings, 2 replies; 26+ messages in thread From: Darrick J. Wong @ 2018-12-10 16:50 UTC (permalink / raw) To: Brian Foster; +Cc: Nick Bowler, linux-xfs On Mon, Dec 10, 2018 at 11:11:22AM -0500, Brian Foster wrote: > On Mon, Dec 10, 2018 at 10:39:14AM -0500, Nick Bowler wrote: > > Hi Brian, > > > > On 12/10/18, Brian Foster <bfoster@redhat.com> wrote: > > > The only thing that comes to mind while poking through the code is > > > perhaps xfsprogs is sending the traditional XFS_IOC_GROWFSDATA command > > > into the compat_ioctl() path somehow or another (assuming > > > BROKEN_X86_ALIGNMENT is set). > > > > > > What arch is your kernel/xfsprogs? > > > > This system is running an amd64 kernel with x32 userspace (including > > xfsprogs). > > > > Ok, so I think that means BROKEN_X86_ALIGNMENT should be set since XFS > defines it as: > > #if defined(CONFIG_IA64) || defined(CONFIG_X86_64) > #define BROKEN_X86_ALIGNMENT > ... > > > > What does 'cat /sys/kernel/debug/trace/trace' show if you run > > > 'trace-cmd start -e xfs:xfs_file*ioctl*' and then attempt the growfs? > > > > Looks like I don't have the required tracing enabled in my kernel > > configuration, but I can build a new one if needed. Is CONFIG_FTRACE > > sufficient for this? > > > > Not sure. I think you need to have CONFIG_TRACING enabled, which may > require FTRACE and/or some other options. Hmm, perhaps you'd be covered > if you make sure you have CONFIG_DYNAMIC_FTRACE enabled. > > From your strace output: > > ioctl(3, _IOC(_IOC_WRITE, 0x58, 0x6e, 0x10), 0xffcc9a80) = -1 ENOTTY (Inappropriate ioctl for device) > > 0x6e corresponds to the GROWFSDATA[_32] cmd and I think 0x10 is the > size, which is 16 bytes as opposed to the 12 bytes expected for > GROWFSDATA_32 for struct compat_xfs_growfs_data: > > typedef struct compat_xfs_growfs_data { > __u64 newblocks; /* new data subvol size, fsblocks */ > __u32 imaxpct; /* new inode space percentage limit */ > } __attribute__((packed)) compat_xfs_growfs_data_t; > > On a 64-bit kernel, that packed attribute is the difference between > expecting a padded 16 byte struct vs. a 12 byte version presumably from > a 32-bit application. So if you are calling into the ->compat_ioctl() > path I think the question is why is your xfsprogs sending the 16 byte > structure? ...because the x32 ABI is weird in that pointers are 4 bytes like on x86, but the registers are 64 bits wide like on x64, and (except for pointers being 4 bytes wide) the structure alignment rules follow x64. Normally xfs structures are explicitly padded to 8-byte boundaries and pointers forced into u64 fields to avoid all of this compatibility headache, but this wasn't done with struct xfs_growfs_data, so it needs a compatibility shim for every ABI supported by Linux. As you can tell, we never really bothered to check in XFS. The creators of the x32 ABI even call out XFS ioctl32.c[1] specifically on their list of things that needed fixing, but they never got around to it. https://sites.google.com/site/x32abi/ $ cat b.c struct moo { unsigned long long a; unsigned int b; }; int bork(struct moo *m) { return m->b; } $ gcc -Wall -g -m64 -c -o b.o b.c # x86_64 $ gdb b.o Reading symbols from b.o...done. (gdb) p sizeof(struct moo) $1 = 12 (gdb) quit $ gcc -Wall -g -m32 -c -o b.o b.c # i386 $ gdb b.o Reading symbols from b.o...done. (gdb) p sizeof(struct moo) $1 = 12 (gdb) quit $ gcc -Wall -g -mx32 -c -o b.o b.c # x32 $ gdb b.o Reading symbols from b.o...done. (gdb) p sizeof(struct moo) $1 = 16 So I guess someone needs to fix the headers to detect x32 and point it at the x64 definitions ... or something. Personally, I thought x32 was basically dead at this point, but clearly not. :/ --D > Brian > > > Thanks, > > Nick ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-10 16:50 ` Darrick J. Wong @ 2018-12-10 16:55 ` Darrick J. Wong 2018-12-10 17:46 ` Brian Foster 1 sibling, 0 replies; 26+ messages in thread From: Darrick J. Wong @ 2018-12-10 16:55 UTC (permalink / raw) To: Brian Foster; +Cc: Nick Bowler, linux-xfs On Mon, Dec 10, 2018 at 08:50:20AM -0800, Darrick J. Wong wrote: > On Mon, Dec 10, 2018 at 11:11:22AM -0500, Brian Foster wrote: > > On Mon, Dec 10, 2018 at 10:39:14AM -0500, Nick Bowler wrote: > > > Hi Brian, > > > > > > On 12/10/18, Brian Foster <bfoster@redhat.com> wrote: > > > > The only thing that comes to mind while poking through the code is > > > > perhaps xfsprogs is sending the traditional XFS_IOC_GROWFSDATA command > > > > into the compat_ioctl() path somehow or another (assuming > > > > BROKEN_X86_ALIGNMENT is set). > > > > > > > > What arch is your kernel/xfsprogs? > > > > > > This system is running an amd64 kernel with x32 userspace (including > > > xfsprogs). > > > > > > > Ok, so I think that means BROKEN_X86_ALIGNMENT should be set since XFS > > defines it as: > > > > #if defined(CONFIG_IA64) || defined(CONFIG_X86_64) > > #define BROKEN_X86_ALIGNMENT > > ... > > > > > > What does 'cat /sys/kernel/debug/trace/trace' show if you run > > > > 'trace-cmd start -e xfs:xfs_file*ioctl*' and then attempt the growfs? > > > > > > Looks like I don't have the required tracing enabled in my kernel > > > configuration, but I can build a new one if needed. Is CONFIG_FTRACE > > > sufficient for this? > > > > > > > Not sure. I think you need to have CONFIG_TRACING enabled, which may > > require FTRACE and/or some other options. Hmm, perhaps you'd be covered > > if you make sure you have CONFIG_DYNAMIC_FTRACE enabled. > > > > From your strace output: > > > > ioctl(3, _IOC(_IOC_WRITE, 0x58, 0x6e, 0x10), 0xffcc9a80) = -1 ENOTTY (Inappropriate ioctl for device) > > > > 0x6e corresponds to the GROWFSDATA[_32] cmd and I think 0x10 is the > > size, which is 16 bytes as opposed to the 12 bytes expected for > > GROWFSDATA_32 for struct compat_xfs_growfs_data: > > > > typedef struct compat_xfs_growfs_data { > > __u64 newblocks; /* new data subvol size, fsblocks */ > > __u32 imaxpct; /* new inode space percentage limit */ > > } __attribute__((packed)) compat_xfs_growfs_data_t; > > > > On a 64-bit kernel, that packed attribute is the difference between > > expecting a padded 16 byte struct vs. a 12 byte version presumably from > > a 32-bit application. So if you are calling into the ->compat_ioctl() > > path I think the question is why is your xfsprogs sending the 16 byte > > structure? > > ...because the x32 ABI is weird in that pointers are 4 bytes like on > x86, but the registers are 64 bits wide like on x64, and (except for > pointers being 4 bytes wide) the structure alignment rules follow x64. > > Normally xfs structures are explicitly padded to 8-byte boundaries and > pointers forced into u64 fields to avoid all of this compatibility > headache, but this wasn't done with struct xfs_growfs_data, so it needs > a compatibility shim for every ABI supported by Linux. > > As you can tell, we never really bothered to check in XFS. The creators > of the x32 ABI even call out XFS ioctl32.c[1] specifically on their list > of things that needed fixing, but they never got around to it. > > https://sites.google.com/site/x32abi/ > > $ cat b.c > struct moo { > unsigned long long a; > unsigned int b; > }; > > int bork(struct moo *m) > { > return m->b; > } > $ gcc -Wall -g -m64 -c -o b.o b.c # x86_64 > $ gdb b.o > Reading symbols from b.o...done. > (gdb) p sizeof(struct moo) > $1 = 12 <sigh> paste error, that's supposed to be 16. --D > (gdb) quit > $ gcc -Wall -g -m32 -c -o b.o b.c # i386 > $ gdb b.o > Reading symbols from b.o...done. > (gdb) p sizeof(struct moo) > $1 = 12 > (gdb) quit > $ gcc -Wall -g -mx32 -c -o b.o b.c # x32 > $ gdb b.o > Reading symbols from b.o...done. > (gdb) p sizeof(struct moo) > $1 = 16 > > So I guess someone needs to fix the headers to detect x32 and point it > at the x64 definitions ... or something. Personally, I thought x32 was > basically dead at this point, but clearly not. :/ > > --D > > > Brian > > > > > Thanks, > > > Nick ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-10 16:50 ` Darrick J. Wong 2018-12-10 16:55 ` Darrick J. Wong @ 2018-12-10 17:46 ` Brian Foster 2018-12-10 20:54 ` Nick Bowler 1 sibling, 1 reply; 26+ messages in thread From: Brian Foster @ 2018-12-10 17:46 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Nick Bowler, linux-xfs On Mon, Dec 10, 2018 at 08:50:20AM -0800, Darrick J. Wong wrote: > On Mon, Dec 10, 2018 at 11:11:22AM -0500, Brian Foster wrote: > > On Mon, Dec 10, 2018 at 10:39:14AM -0500, Nick Bowler wrote: > > > Hi Brian, > > > > > > On 12/10/18, Brian Foster <bfoster@redhat.com> wrote: > > > > The only thing that comes to mind while poking through the code is > > > > perhaps xfsprogs is sending the traditional XFS_IOC_GROWFSDATA command > > > > into the compat_ioctl() path somehow or another (assuming > > > > BROKEN_X86_ALIGNMENT is set). > > > > > > > > What arch is your kernel/xfsprogs? > > > > > > This system is running an amd64 kernel with x32 userspace (including > > > xfsprogs). > > > > > > > Ok, so I think that means BROKEN_X86_ALIGNMENT should be set since XFS > > defines it as: > > > > #if defined(CONFIG_IA64) || defined(CONFIG_X86_64) > > #define BROKEN_X86_ALIGNMENT > > ... > > > > > > What does 'cat /sys/kernel/debug/trace/trace' show if you run > > > > 'trace-cmd start -e xfs:xfs_file*ioctl*' and then attempt the growfs? > > > > > > Looks like I don't have the required tracing enabled in my kernel > > > configuration, but I can build a new one if needed. Is CONFIG_FTRACE > > > sufficient for this? > > > > > > > Not sure. I think you need to have CONFIG_TRACING enabled, which may > > require FTRACE and/or some other options. Hmm, perhaps you'd be covered > > if you make sure you have CONFIG_DYNAMIC_FTRACE enabled. > > > > From your strace output: > > > > ioctl(3, _IOC(_IOC_WRITE, 0x58, 0x6e, 0x10), 0xffcc9a80) = -1 ENOTTY (Inappropriate ioctl for device) > > > > 0x6e corresponds to the GROWFSDATA[_32] cmd and I think 0x10 is the > > size, which is 16 bytes as opposed to the 12 bytes expected for > > GROWFSDATA_32 for struct compat_xfs_growfs_data: > > > > typedef struct compat_xfs_growfs_data { > > __u64 newblocks; /* new data subvol size, fsblocks */ > > __u32 imaxpct; /* new inode space percentage limit */ > > } __attribute__((packed)) compat_xfs_growfs_data_t; > > > > On a 64-bit kernel, that packed attribute is the difference between > > expecting a padded 16 byte struct vs. a 12 byte version presumably from > > a 32-bit application. So if you are calling into the ->compat_ioctl() > > path I think the question is why is your xfsprogs sending the 16 byte > > structure? > > ...because the x32 ABI is weird in that pointers are 4 bytes like on > x86, but the registers are 64 bits wide like on x64, and (except for > pointers being 4 bytes wide) the structure alignment rules follow x64. > Thanks, I wasn't aware of that. I just read x32 as x86. > Normally xfs structures are explicitly padded to 8-byte boundaries and > pointers forced into u64 fields to avoid all of this compatibility > headache, but this wasn't done with struct xfs_growfs_data, so it needs > a compatibility shim for every ABI supported by Linux. > > As you can tell, we never really bothered to check in XFS. The creators > of the x32 ABI even call out XFS ioctl32.c[1] specifically on their list > of things that needed fixing, but they never got around to it. > > https://sites.google.com/site/x32abi/ > ... > > So I guess someone needs to fix the headers to detect x32 and point it > at the x64 definitions ... or something. Personally, I thought x32 was > basically dead at this point, but clearly not. :/ > Yeah, it seems to me that fundamentally conflicts with the whole BROKEN_X86_ALIGNMENT thing we have now. IIUC, compat_ioctl() on an x86_64 kernel needs to account for x86 userspace via all of the associated _32 ioctl commands as it already does, but at the same time x32 means we could get any of the traditional x86_64 commands through that path as well. I guess the short answer in the meantime is that XFS apparently doesn't support this architecture. Brian > --D > > > Brian > > > > > Thanks, > > > Nick ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-10 17:46 ` Brian Foster @ 2018-12-10 20:54 ` Nick Bowler 2018-12-10 21:41 ` Dave Chinner 0 siblings, 1 reply; 26+ messages in thread From: Nick Bowler @ 2018-12-10 20:54 UTC (permalink / raw) To: Brian Foster; +Cc: Darrick J. Wong, linux-xfs On 2018-12-10, Brian Foster <bfoster@redhat.com> wrote: > On Mon, Dec 10, 2018 at 08:50:20AM -0800, Darrick J. Wong wrote: >> On Mon, Dec 10, 2018 at 11:11:22AM -0500, Brian Foster wrote: >> > 0x6e corresponds to the GROWFSDATA[_32] cmd and I think 0x10 is the >> > size, which is 16 bytes as opposed to the 12 bytes expected for >> > GROWFSDATA_32 for struct compat_xfs_growfs_data: >> > >> > typedef struct compat_xfs_growfs_data { >> > __u64 newblocks; /* new data subvol size, >> > fsblocks */ >> > __u32 imaxpct; /* new inode space percentage >> > limit */ >> > } __attribute__((packed)) compat_xfs_growfs_data_t; >> > >> > On a 64-bit kernel, that packed attribute is the difference between >> > expecting a padded 16 byte struct vs. a 12 byte version presumably from >> > a 32-bit application. So if you are calling into the ->compat_ioctl() >> > path I think the question is why is your xfsprogs sending the 16 byte >> > structure? >> >> ...because the x32 ABI is weird in that pointers are 4 bytes like on >> x86, but the registers are 64 bits wide like on x64, and (except for >> pointers being 4 bytes wide) the structure alignment rules follow x64. [...] > Yeah, it seems to me that fundamentally conflicts with the whole > BROKEN_X86_ALIGNMENT thing we have now. IIUC, compat_ioctl() on an > x86_64 kernel needs to account for x86 userspace via all of the > associated _32 ioctl commands as it already does, but at the same time > x32 means we could get any of the traditional x86_64 commands through > that path as well. In the specific case of this one ioctl on this one architecture since the only problem is unused padding at the end of the structure, the fix might be simple: just accept both ioctl numbers in the compat path. The packed compat struct layout looks like it should match what x32 userspace sends just fine. (I didn't realize x32 syscalls would go through compat_ioctl). i.e., perhaps we just do something like this? (TOTALLY UNTESTED) diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c index fba115f4103a..b5a02f36d568 100644 --- a/fs/xfs/xfs_ioctl32.c +++ b/fs/xfs/xfs_ioctl32.c @@ -581,6 +581,9 @@ xfs_file_compat_ioctl( } case XFS_IOC_FSGEOMETRY_V1_32: return xfs_compat_ioc_fsgeometry_v1(mp, arg); +#ifdef CONFIG_X86_X32 + case XFS_IOC_FSGROWFSDATA: +#endif case XFS_IOC_FSGROWFSDATA_32: { struct xfs_growfs_data in; 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. Cheers, Nick ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-10 20:54 ` Nick Bowler @ 2018-12-10 21:41 ` Dave Chinner 2018-12-11 7:04 ` Nick Bowler 0 siblings, 1 reply; 26+ messages in thread From: Dave Chinner @ 2018-12-10 21:41 UTC (permalink / raw) To: Nick Bowler; +Cc: Brian Foster, Darrick J. Wong, linux-xfs On Mon, Dec 10, 2018 at 03:54:47PM -0500, Nick Bowler wrote: > On 2018-12-10, Brian Foster <bfoster@redhat.com> wrote: > > On Mon, Dec 10, 2018 at 08:50:20AM -0800, Darrick J. Wong wrote: > >> On Mon, Dec 10, 2018 at 11:11:22AM -0500, Brian Foster wrote: > >> > 0x6e corresponds to the GROWFSDATA[_32] cmd and I think 0x10 is the > >> > size, which is 16 bytes as opposed to the 12 bytes expected for > >> > GROWFSDATA_32 for struct compat_xfs_growfs_data: > >> > > >> > typedef struct compat_xfs_growfs_data { > >> > __u64 newblocks; /* new data subvol size, > >> > fsblocks */ > >> > __u32 imaxpct; /* new inode space percentage > >> > limit */ > >> > } __attribute__((packed)) compat_xfs_growfs_data_t; > >> > > >> > On a 64-bit kernel, that packed attribute is the difference between > >> > expecting a padded 16 byte struct vs. a 12 byte version presumably from > >> > a 32-bit application. So if you are calling into the ->compat_ioctl() > >> > path I think the question is why is your xfsprogs sending the 16 byte > >> > structure? > >> > >> ...because the x32 ABI is weird in that pointers are 4 bytes like on > >> x86, but the registers are 64 bits wide like on x64, and (except for > >> pointers being 4 bytes wide) the structure alignment rules follow x64. > [...] > > Yeah, it seems to me that fundamentally conflicts with the whole > > BROKEN_X86_ALIGNMENT thing we have now. IIUC, compat_ioctl() on an > > x86_64 kernel needs to account for x86 userspace via all of the > > associated _32 ioctl commands as it already does, but at the same time > > x32 means we could get any of the traditional x86_64 commands through > > that path as well. > > In the specific case of this one ioctl on this one architecture since the > only problem is unused padding at the end of the structure, the fix might > be simple: just accept both ioctl numbers in the compat path. The packed > compat struct layout looks like it should match what x32 userspace sends > just fine. (I didn't realize x32 syscalls would go through compat_ioctl). > > i.e., perhaps we just do something like this? (TOTALLY UNTESTED) > > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c > index fba115f4103a..b5a02f36d568 100644 > --- a/fs/xfs/xfs_ioctl32.c > +++ b/fs/xfs/xfs_ioctl32.c > @@ -581,6 +581,9 @@ xfs_file_compat_ioctl( > } > case XFS_IOC_FSGEOMETRY_V1_32: > return xfs_compat_ioc_fsgeometry_v1(mp, arg); > +#ifdef CONFIG_X86_X32 > + case XFS_IOC_FSGROWFSDATA: > +#endif > case XFS_IOC_FSGROWFSDATA_32: { > struct xfs_growfs_data in; Ugly, but something like that may be our only option here. > > 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.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-10 21:41 ` Dave Chinner @ 2018-12-11 7:04 ` Nick Bowler 2018-12-11 12:27 ` Brian Foster 0 siblings, 1 reply; 26+ messages in thread From: Nick Bowler @ 2018-12-11 7:04 UTC (permalink / raw) To: Dave Chinner; +Cc: Brian Foster, Darrick J. Wong, linux-xfs 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. I didn't look at the set of ioctls marked 'No size or alignment issue on any arch' since those should presumably all be fine. I manually inspected the structures used by everything else. On all the cases under BROKEN_X86_ALIGNMENT, the structures differ due to 64-bit integer alignment reasons alone. In this case, x32 will match amd64 exactly, including the calculated ioctl number (which is different from the ia32 ioctl number since the structure sizes change), so we just need to emit BOTH sets of cases to handle this correctly. The odd one out is XFS_IOC_SWAPEXT. This is a rather big structure but it appears to consist only of integers of various sizes, with some time_t in there. That should still all exactly match between x32 and amd64, and the structure size is different from ia32 so we can add a case that calls the native version. All remaining ioctls use structures that consist exclusively of 32-bit integers and one or more pointers (or, recursively, structures containing only such members). That means x32 should match ia32 exactly for these, including the calculated ioctl number, so the compat versions should be fine as-is for these and no change is required. diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c index fba115f4103a..dd77f20f42ec 100644 --- a/fs/xfs/xfs_ioctl32.c +++ b/fs/xfs/xfs_ioctl32.c @@ -547,7 +547,7 @@ xfs_file_compat_ioctl( case FS_IOC_GETFSMAP: case XFS_IOC_SCRUB_METADATA: return xfs_file_ioctl(filp, cmd, p); -#ifndef BROKEN_X86_ALIGNMENT +#if !defined(BROKEN_X86_ALIGNMENT) || defined(CONFIG_X86_X32) /* These are handled fine if no alignment issues */ case XFS_IOC_ALLOCSP: case XFS_IOC_FREESP: @@ -561,8 +561,12 @@ xfs_file_compat_ioctl( case XFS_IOC_FSGROWFSDATA: case XFS_IOC_FSGROWFSRT: case XFS_IOC_ZERO_RANGE: +#ifdef CONFIG_X86_X32 + case XFS_IOC_SWAPEXT: +#endif return xfs_file_ioctl(filp, cmd, p); -#else +#endif +#if defined(BROKEN_X86_ALIGNMENT) case XFS_IOC_ALLOCSP_32: case XFS_IOC_FREESP_32: case XFS_IOC_ALLOCSP64_32: ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-11 7:04 ` Nick Bowler @ 2018-12-11 12:27 ` Brian Foster 2018-12-11 20:13 ` Nick Bowler 2018-12-12 4:56 ` Nick Bowler 0 siblings, 2 replies; 26+ messages in thread From: Brian Foster @ 2018-12-11 12:27 UTC (permalink / raw) To: Nick Bowler; +Cc: Dave Chinner, Darrick J. Wong, linux-xfs 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). 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. As for test mechanism, look at the fstests suite: https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git It contains a suite of tests that all fs devs run against most changes before they are posted/committed to the upstream kernel. Within that tool, see the xfstests-dev/ltp/fsstress and xfstests-dev/ltp/fsx test utilities. Each of those utils basically runs a stress test that consists of all supported operations against a target directory. For example, the set of operations noted by fsstress is: the valid operation names are: afsync allocsp aread attr_remove attr_set awrite bulkstat bulkstat1 chown clonerange copyrange creat deduperange dread dwrite fallocate fdatasync fiemap freesp fsync getattr getdents link mkdir mknod mread mwrite punch zero collapse insert read readlink readv rename resvsp rmdir setattr setxattr stat symlink sync truncate unlink unresvsp write writev ... which covers several of these ioctls (block allocation, bulkstat, etc.). The broader fstests suite has tests to cover things like growfs, etc., and will also run the aforementioned tools, but you can run them directly for quicker verification during development. So if you'd like to propose this change for upstream inclusion, I'd suggest to take advantage of those tools to test your change and then run the xfstests suite (re: the auto test group) against the targeted configuration along with one or more affected configurations. Brian > I didn't look at the set of ioctls marked 'No size or alignment issue on > any arch' since those should presumably all be fine. > > I manually inspected the structures used by everything else. On all the > cases under BROKEN_X86_ALIGNMENT, the structures differ due to 64-bit > integer alignment reasons alone. In this case, x32 will match amd64 > exactly, including the calculated ioctl number (which is different from > the ia32 ioctl number since the structure sizes change), so we just need > to emit BOTH sets of cases to handle this correctly. > > The odd one out is XFS_IOC_SWAPEXT. This is a rather big structure but it > appears to consist only of integers of various sizes, with some time_t in > there. That should still all exactly match between x32 and amd64, and the > structure size is different from ia32 so we can add a case that calls the > native version. > > All remaining ioctls use structures that consist exclusively of 32-bit > integers and one or more pointers (or, recursively, structures containing > only such members). That means x32 should match ia32 exactly for these, > including the calculated ioctl number, so the compat versions should be > fine as-is for these and no change is required. > > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c > index fba115f4103a..dd77f20f42ec 100644 > --- a/fs/xfs/xfs_ioctl32.c > +++ b/fs/xfs/xfs_ioctl32.c > @@ -547,7 +547,7 @@ xfs_file_compat_ioctl( > case FS_IOC_GETFSMAP: > case XFS_IOC_SCRUB_METADATA: > return xfs_file_ioctl(filp, cmd, p); > -#ifndef BROKEN_X86_ALIGNMENT > +#if !defined(BROKEN_X86_ALIGNMENT) || defined(CONFIG_X86_X32) > /* These are handled fine if no alignment issues */ > case XFS_IOC_ALLOCSP: > case XFS_IOC_FREESP: > @@ -561,8 +561,12 @@ xfs_file_compat_ioctl( > case XFS_IOC_FSGROWFSDATA: > case XFS_IOC_FSGROWFSRT: > case XFS_IOC_ZERO_RANGE: > +#ifdef CONFIG_X86_X32 > + case XFS_IOC_SWAPEXT: > +#endif > return xfs_file_ioctl(filp, cmd, p); > -#else > +#endif > +#if defined(BROKEN_X86_ALIGNMENT) > case XFS_IOC_ALLOCSP_32: > case XFS_IOC_FREESP_32: > case XFS_IOC_ALLOCSP64_32: ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-11 12:27 ` Brian Foster @ 2018-12-11 20:13 ` Nick Bowler 2018-12-11 20:20 ` Nick Bowler 2018-12-12 4:56 ` Nick Bowler 1 sibling, 1 reply; 26+ messages in thread From: Nick Bowler @ 2018-12-11 20:13 UTC (permalink / raw) To: Brian Foster; +Cc: Dave Chinner, Darrick J. Wong, linux-xfs On 12/11/18, Brian Foster <bfoster@redhat.com> wrote: > On Tue, Dec 11, 2018 at 02:04:48AM -0500, Nick Bowler wrote: >> 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. 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-11 20:13 ` Nick Bowler @ 2018-12-11 20:20 ` Nick Bowler 2018-12-12 13:09 ` Brian Foster 0 siblings, 1 reply; 26+ messages in thread From: Nick Bowler @ 2018-12-11 20:20 UTC (permalink / raw) To: Brian Foster; +Cc: Dave Chinner, Darrick J. Wong, linux-xfs On 12/11/18, Nick Bowler <nbowler@draconx.ca> wrote: > On 12/11/18, Brian Foster <bfoster@redhat.com> wrote: >> 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). [...] > 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. Right after I write this, I realize that it's almost certainly the case that architectures which _don't_ define BROKEN_X86_ALIGNMENT will have matching ioctl numbers between e.g., XFS_IOC_ALLOCSP and XFS_IOC_ALLOCSP_32. Thus the ifdeffery is essential for the above syntactic reason. Cheers, Nick ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-11 20:20 ` Nick Bowler @ 2018-12-12 13:09 ` Brian Foster 2018-12-13 0:21 ` Nick Bowler 0 siblings, 1 reply; 26+ messages in thread From: Brian Foster @ 2018-12-12 13:09 UTC (permalink / raw) To: Nick Bowler; +Cc: Dave Chinner, Darrick J. Wong, linux-xfs On Tue, Dec 11, 2018 at 03:20:23PM -0500, Nick Bowler wrote: > On 12/11/18, Nick Bowler <nbowler@draconx.ca> wrote: > > On 12/11/18, Brian Foster <bfoster@redhat.com> wrote: > >> 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). > [...] > > 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. > > Right after I write this, I realize that it's almost certainly > the case that architectures which _don't_ define BROKEN_X86_ALIGNMENT > will have matching ioctl numbers between e.g., XFS_IOC_ALLOCSP and > XFS_IOC_ALLOCSP_32. Thus the ifdeffery is essential for the above > syntactic reason. > So the two struct [compat_]xfs_flock64 variants are essentially the same with the exception of internal alignment padding that leaves a hole after the first 4 bytes in the 64-bit variant. So on x86_64, these are essentially two different (sized) structures and ioctl commands. On some non-broken alignment arch, the packing may be implied and thus these definitions evaluate down to the same ioctl command. If so, then having those separate but equivalent value commands in the same switch statement results in a compilation error. Am I following that correctly? If so, then it does seem we need an ifdef to exlude those definitions so long as we follow the one huge switch statement implementation. But is there anything that fundamentally prevents a multiple switch statement implementation to avoid such syntax errors? Note that in the end I don't care too much about whether we have an ifdef or not. I'm more interested in the most simple and elegant implementation and it just seemed that trying to dilute the existing ifdef may push us in the opposite direction (as opposed to something that for example called into the "native" ioctl and fell back to the _32 variants on failure before returning an error). There may be technical complications to that or using some form of the ifdef may just end up being the more simple approach after all. Brian > Cheers, > Nick ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-12 13:09 ` Brian Foster @ 2018-12-13 0:21 ` Nick Bowler 0 siblings, 0 replies; 26+ messages in thread From: Nick Bowler @ 2018-12-13 0:21 UTC (permalink / raw) To: Brian Foster; +Cc: Dave Chinner, Darrick J. Wong, linux-xfs On 2018-12-12, Brian Foster <bfoster@redhat.com> wrote: > On Tue, Dec 11, 2018 at 03:20:23PM -0500, Nick Bowler wrote: >> Right after I write this, I realize that it's almost certainly >> the case that architectures which _don't_ define BROKEN_X86_ALIGNMENT >> will have matching ioctl numbers between e.g., XFS_IOC_ALLOCSP and >> XFS_IOC_ALLOCSP_32. [...] > So the two struct [compat_]xfs_flock64 variants are essentially the same > with the exception of internal alignment padding that leaves a hole > after the first 4 bytes in the 64-bit variant. So on x86_64, these are > essentially two different (sized) structures and ioctl commands. On some > non-broken alignment arch, the packing may be implied and thus these > definitions evaluate down to the same ioctl command. If so, then having > those separate but equivalent value commands in the same switch > statement results in a compilation error. Am I following that correctly? > > If so, then it does seem we need an ifdef to exlude those definitions so > long as we follow the one huge switch statement implementation. But is > there anything that fundamentally prevents a multiple switch statement > implementation to avoid such syntax errors? I think nothing fundamentally prevents this approach and it is an option. The only point I see is that, without the configuration ifdefs, the impact of the change is expanded; there are (presumably benign) functional changes on more architectures, most of which I don't have the ability to test. Making it dependent on the arch configuration means nothing should change anywhere except on amd64 kernels that have x32 binary support turned on. > Note that in the end I don't care too much about whether we have an > ifdef or not. I'm more interested in the most simple and elegant > implementation and it just seemed that trying to dilute the existing > ifdef may push us in the opposite direction (as opposed to something > that for example called into the "native" ioctl and fell back to the _32 > variants on failure before returning an error). There may be technical > complications to that or using some form of the ifdef may just end up > being the more simple approach after all. OK, since I have some test results I'll submit the current one for RFC; afterwards I'll try to follow up with a less ifdeffy variant for comparison purposes, then we can decide which version is better. :) Cheers, Nick ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-11 12:27 ` Brian Foster 2018-12-11 20:13 ` Nick Bowler @ 2018-12-12 4:56 ` Nick Bowler 2018-12-13 3:53 ` Dave Chinner 1 sibling, 1 reply; 26+ messages in thread From: Nick Bowler @ 2018-12-12 4:56 UTC (permalink / raw) To: Brian Foster; +Cc: Dave Chinner, Darrick J. Wong, linux-xfs 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. I'll keep testing and hope to get to submitting these properly. diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c index 001b30605d45..eb6e80c7f2bd 100644 --- a/fs/xfs/xfs_ioctl32.c +++ b/fs/xfs/xfs_ioctl32.c @@ -241,6 +241,21 @@ xfs_compat_ioc_bulkstat( int done; int error; + /* These functions and size are used later to handle individual + * entries; x32 is annoying and needs different functions. */ + inumbers_fmt_pf inumbers_func = xfs_inumbers_fmt_compat; + bulkstat_one_pf bs_one_func = xfs_bulkstat_one_compat; + size_t bs_one_size = sizeof (compat_xfs_bstat_t); + +#ifdef CONFIG_X86_X32 + if (in_x32_syscall()) { + /* x32 matches native amd64 bstat and inogrp layout */ + inumbers_func = xfs_inumbers_fmt; + bs_one_func = xfs_bulkstat_one; + bs_one_size = sizeof (xfs_bstat_t); + } +#endif + /* done = 1 if there are more stats to get and if bulkstat */ /* should be called again (unused here, but used in dmapi) */ @@ -272,15 +287,15 @@ xfs_compat_ioc_bulkstat( if (cmd == XFS_IOC_FSINUMBERS_32) { error = xfs_inumbers(mp, &inlast, &count, - bulkreq.ubuffer, xfs_inumbers_fmt_compat); + bulkreq.ubuffer, inumbers_func); } else if (cmd == XFS_IOC_FSBULKSTAT_SINGLE_32) { int res; - error = xfs_bulkstat_one_compat(mp, inlast, bulkreq.ubuffer, - sizeof(compat_xfs_bstat_t), NULL, &res); + error = bs_one_func(mp, inlast, bulkreq.ubuffer, + bs_one_size, NULL, &res); } else if (cmd == XFS_IOC_FSBULKSTAT_32) { error = xfs_bulkstat(mp, &inlast, &count, - xfs_bulkstat_one_compat, sizeof(compat_xfs_bstat_t), + bs_one_func, bs_one_size, bulkreq.ubuffer, &done); } else error = -EINVAL; ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-12 4:56 ` Nick Bowler @ 2018-12-13 3:53 ` Dave Chinner 2018-12-13 4:14 ` Nick Bowler 2018-12-13 16:30 ` Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device Darrick J. Wong 0 siblings, 2 replies; 26+ messages in thread From: Dave Chinner @ 2018-12-13 3:53 UTC (permalink / raw) To: Nick Bowler; +Cc: Brian Foster, Darrick J. Wong, linux-xfs 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-13 3:53 ` Dave Chinner @ 2018-12-13 4:14 ` Nick Bowler 2018-12-13 4:49 ` Nick Bowler 2018-12-13 16:30 ` Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device Darrick J. Wong 1 sibling, 1 reply; 26+ messages in thread From: Nick Bowler @ 2018-12-13 4:14 UTC (permalink / raw) To: Dave Chinner; +Cc: Brian Foster, Darrick J. Wong, linux-xfs On 2018-12-12, Dave Chinner <david@fromorbit.com> wrote: > On Tue, Dec 11, 2018 at 11:56:33PM -0500, Nick Bowler wrote: >> 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. This is not the case: struct timeval is 16 bytes on x32: sizeof (struct timeval): 16 tv_sec size: 8 offset: 0 tv_usec size: 8 offset: 8 This is the same as what I get on native 64-bit compilations; but anyway the xfs_bstat structure has xfs_bstime members, with the following characteristics on x32: sizeof (struct xfs_bstime): 16 tv_sec size: 8 offset: 0 tv_nsec size: 4 offset: 8 which is also the same as native 64-bit (time_t is the same on x32 and native: 8 bytes with 8 byte alignment). I manually verified every member of the xfs_bstat structure with sizeof and offsetof on -mx32 and -m64 compilations to ensure that this structure matches precisely between the x32 and native 64-bit cases. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-13 4:14 ` Nick Bowler @ 2018-12-13 4:49 ` Nick Bowler 2018-12-13 21:39 ` Dave Chinner 0 siblings, 1 reply; 26+ messages in thread From: Nick Bowler @ 2018-12-13 4:49 UTC (permalink / raw) To: Dave Chinner; +Cc: Brian Foster, Darrick J. Wong, linux-xfs On 2018-12-12, Nick Bowler <nbowler@draconx.ca> wrote: > On 2018-12-12, Dave Chinner <david@fromorbit.com> wrote: >> On Tue, Dec 11, 2018 at 11:56:33PM -0500, Nick Bowler wrote: >>> 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. > > This is not the case: struct timeval is 16 bytes on x32: > > sizeof (struct timeval): 16 > tv_sec size: 8 offset: 0 > tv_usec size: 8 offset: 8 > > This is the same as what I get on native 64-bit compilations; but > anyway the xfs_bstat structure has xfs_bstime members, with the > following characteristics on x32: > > sizeof (struct xfs_bstime): 16 > tv_sec size: 8 offset: 0 > tv_nsec size: 4 offset: 8 > > which is also the same as native 64-bit (time_t is the same on x32 and > native: 8 bytes with 8 byte alignment). > > I manually verified every member of the xfs_bstat structure with sizeof > and offsetof on -mx32 and -m64 compilations to ensure that this structure > matches precisely between the x32 and native 64-bit cases. To expand on this, for each structure which my RFC patchset feeds up to the native handler, I first checked them by manual inspection and then double checked using the following program; we can compile with both -mx32 and -m64 and check that the output is identical. #include <stdio.h> #include <stddef.h> #include <xfs/xfs.h> #include <time.h> #define SHOWSIZE(type) do { \ printf("sizeof (%s): %zu\n", #type, sizeof (type)); \ } while(0) #define SHOWMEMBER(type, member) do { \ printf(" %-15s size: %3zu offset: %3zu\n", #member, \ sizeof (type){0}.member, offsetof(type, member)); \ } while(0) int main(void) { SHOWSIZE(struct xfs_flock64); SHOWMEMBER(struct xfs_flock64, l_type); SHOWMEMBER(struct xfs_flock64, l_whence); SHOWMEMBER(struct xfs_flock64, l_start); SHOWMEMBER(struct xfs_flock64, l_len); SHOWMEMBER(struct xfs_flock64, l_sysid); SHOWMEMBER(struct xfs_flock64, l_pid); SHOWMEMBER(struct xfs_flock64, l_pad); SHOWSIZE(struct xfs_fsop_geom_v1); SHOWMEMBER(struct xfs_fsop_geom_v1, blocksize); SHOWMEMBER(struct xfs_fsop_geom_v1, rtextsize); SHOWMEMBER(struct xfs_fsop_geom_v1, agblocks); SHOWMEMBER(struct xfs_fsop_geom_v1, agcount); SHOWMEMBER(struct xfs_fsop_geom_v1, logblocks); SHOWMEMBER(struct xfs_fsop_geom_v1, sectsize); SHOWMEMBER(struct xfs_fsop_geom_v1, inodesize); SHOWMEMBER(struct xfs_fsop_geom_v1, imaxpct); SHOWMEMBER(struct xfs_fsop_geom_v1, datablocks); SHOWMEMBER(struct xfs_fsop_geom_v1, rtblocks); SHOWMEMBER(struct xfs_fsop_geom_v1, rtextents); SHOWMEMBER(struct xfs_fsop_geom_v1, logstart); SHOWMEMBER(struct xfs_fsop_geom_v1, uuid); SHOWMEMBER(struct xfs_fsop_geom_v1, sunit); SHOWMEMBER(struct xfs_fsop_geom_v1, swidth); SHOWMEMBER(struct xfs_fsop_geom_v1, version); SHOWMEMBER(struct xfs_fsop_geom_v1, flags); SHOWMEMBER(struct xfs_fsop_geom_v1, logsectsize); SHOWMEMBER(struct xfs_fsop_geom_v1, rtsectsize); SHOWMEMBER(struct xfs_fsop_geom_v1, dirblocksize); SHOWSIZE(struct xfs_growfs_data); SHOWMEMBER(struct xfs_growfs_data, newblocks); SHOWMEMBER(struct xfs_growfs_data, imaxpct); SHOWSIZE(struct xfs_growfs_rt); SHOWMEMBER(struct xfs_growfs_rt, newblocks); SHOWMEMBER(struct xfs_growfs_rt, extsize); SHOWSIZE(struct xfs_bstime); SHOWMEMBER(struct xfs_bstime, tv_sec); SHOWMEMBER(struct xfs_bstime, tv_nsec); SHOWSIZE(struct xfs_bstat); SHOWMEMBER(struct xfs_bstat, bs_ino); SHOWMEMBER(struct xfs_bstat, bs_mode); SHOWMEMBER(struct xfs_bstat, bs_nlink); SHOWMEMBER(struct xfs_bstat, bs_uid); SHOWMEMBER(struct xfs_bstat, bs_gid); SHOWMEMBER(struct xfs_bstat, bs_rdev); SHOWMEMBER(struct xfs_bstat, bs_blksize); SHOWMEMBER(struct xfs_bstat, bs_size); SHOWMEMBER(struct xfs_bstat, bs_atime); SHOWMEMBER(struct xfs_bstat, bs_mtime); SHOWMEMBER(struct xfs_bstat, bs_ctime); SHOWMEMBER(struct xfs_bstat, bs_blocks); SHOWMEMBER(struct xfs_bstat, bs_xflags); SHOWMEMBER(struct xfs_bstat, bs_extsize); SHOWMEMBER(struct xfs_bstat, bs_extents); SHOWMEMBER(struct xfs_bstat, bs_gen); SHOWMEMBER(struct xfs_bstat, bs_projid_lo); SHOWMEMBER(struct xfs_bstat, bs_forkoff); SHOWMEMBER(struct xfs_bstat, bs_projid_hi); SHOWMEMBER(struct xfs_bstat, bs_pad); SHOWMEMBER(struct xfs_bstat, bs_cowextsize); SHOWMEMBER(struct xfs_bstat, bs_dmevmask); SHOWMEMBER(struct xfs_bstat, bs_dmstate); SHOWMEMBER(struct xfs_bstat, bs_aextents); SHOWSIZE(struct xfs_inogrp); SHOWMEMBER(struct xfs_inogrp, xi_startino); SHOWMEMBER(struct xfs_inogrp, xi_alloccount); SHOWMEMBER(struct xfs_inogrp, xi_allocmask); SHOWSIZE(struct xfs_swapext); SHOWMEMBER(struct xfs_swapext, sx_version); SHOWMEMBER(struct xfs_swapext, sx_fdtarget); SHOWMEMBER(struct xfs_swapext, sx_fdtmp); SHOWMEMBER(struct xfs_swapext, sx_offset); SHOWMEMBER(struct xfs_swapext, sx_length); SHOWMEMBER(struct xfs_swapext, sx_pad); SHOWMEMBER(struct xfs_swapext, sx_stat); } ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-13 4:49 ` Nick Bowler @ 2018-12-13 21:39 ` Dave Chinner 2018-12-13 21:53 ` Nick Bowler 2018-12-14 3:35 ` Nick Bowler 0 siblings, 2 replies; 26+ messages in thread From: Dave Chinner @ 2018-12-13 21:39 UTC (permalink / raw) To: Nick Bowler; +Cc: Brian Foster, Darrick J. Wong, linux-xfs On Wed, Dec 12, 2018 at 11:49:36PM -0500, Nick Bowler wrote: > On 2018-12-12, Nick Bowler <nbowler@draconx.ca> wrote: > > On 2018-12-12, Dave Chinner <david@fromorbit.com> wrote: > >> On Tue, Dec 11, 2018 at 11:56:33PM -0500, Nick Bowler wrote: > >>> 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. > > > > This is not the case: struct timeval is 16 bytes on x32: > > > > sizeof (struct timeval): 16 > > tv_sec size: 8 offset: 0 > > tv_usec size: 8 offset: 8 I was just quoting the kernel time subsystem maintainer who said that these structures had problems with size and packing. > > > > This is the same as what I get on native 64-bit compilations; but > > anyway the xfs_bstat structure has xfs_bstime members, with the > > following characteristics on x32: > > > > sizeof (struct xfs_bstime): 16 > > tv_sec size: 8 offset: 0 > > tv_nsec size: 4 offset: 8 > > > > which is also the same as native 64-bit (time_t is the same on x32 and > > native: 8 bytes with 8 byte alignment). > > > > I manually verified every member of the xfs_bstat structure with sizeof > > and offsetof on -mx32 and -m64 compilations to ensure that this structure > > matches precisely between the x32 and native 64-bit cases. > > To expand on this, for each structure which my RFC patchset feeds up to > the native handler, I first checked them by manual inspection and then > double checked using the following program; we can compile with both > -mx32 and -m64 and check that the output is identical. So, turn that into an xfstest so that it is always run, diffs the output between compat/native depending on which one is used complete with guards that break the test when we add a new ioctl. We already we have a test that is for explicitly checking that structures on disk are the same for 32/64 bit architectures: tests/xfs/122 That test automates the generation of the test code and output, and if it changes from the golden output, then the test fails. I'd suggest that a similar thing is done here for /all/ the structures we expose in ioctls. FWIW, pahole can make this easy. e.g you can harvest every ioctl structure from xfs_fs.h, write them into a file like so: $ cat t.c #include <xfs/xfs.h> /* these should be the same for x32/x86-64, but not i386 */ int main(int argc, char *argv[]) { struct xfs_bstat bs = {0}; struct xfs_fsop_geom_v1 geo1 = {0}; struct xfs_fsop_geom geo = {0}; struct xfs_growfs_data gd = {0}; struct xfs_growfs_rt gr = {0}; struct xfs_flock64 fl = {0}; struct xfs_inogrp ig = {0}; struct xfs_swapext se = {0}; return 0; } And then compile and dump the structure layouts like so: $ gcc -m64 -gdwarf-2 t.c -c; pahole t.o > t.x86-64 $ gcc -mx32 -gdwarf-2 t.c -c; pahole t.o > t.x32 $ gcc -m32 -gdwarf-2 t.c -c; pahole t.o > t.i386 Then we'll have tests that will fail if we ever change an ioctl or add a new one and don't add it to the test. That guarantees we won't ever forget about this.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-13 21:39 ` Dave Chinner @ 2018-12-13 21:53 ` Nick Bowler 2018-12-14 1:43 ` Dave Chinner 2018-12-14 3:35 ` Nick Bowler 1 sibling, 1 reply; 26+ messages in thread From: Nick Bowler @ 2018-12-13 21:53 UTC (permalink / raw) To: Dave Chinner; +Cc: Brian Foster, Darrick J. Wong, linux-xfs On 2018-12-13, Dave Chinner <david@fromorbit.com> wrote: > On Wed, Dec 12, 2018 at 11:49:36PM -0500, Nick Bowler wrote: >> To expand on this, for each structure which my RFC patchset feeds up to >> the native handler, I first checked them by manual inspection and then >> double checked using the following program; we can compile with both >> -mx32 and -m64 and check that the output is identical. > > So, turn that into an xfstest so that it is always run, diffs the > output between compat/native depending on which one is used complete > with guards that break the test when we add a new ioctl. We already > we have a test that is for explicitly checking that structures on disk > are the same for 32/64 bit architectures: tests/xfs/122 [...] > Then we'll have tests that will fail if we ever change an ioctl or > add a new one and don't add it to the test. That guarantees we won't > ever forget about this.... OK, I will give it a shot to implement such a test. A possible issue is that developers might not have a working x32 build or runtime environment so the test might not get run a lot. But hopefully people adding brand new ioctls don't introduce brand new compat problems; one can dream, right? Thanks, Nick ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-13 21:53 ` Nick Bowler @ 2018-12-14 1:43 ` Dave Chinner 0 siblings, 0 replies; 26+ messages in thread From: Dave Chinner @ 2018-12-14 1:43 UTC (permalink / raw) To: Nick Bowler; +Cc: Brian Foster, Darrick J. Wong, linux-xfs On Thu, Dec 13, 2018 at 04:53:56PM -0500, Nick Bowler wrote: > On 2018-12-13, Dave Chinner <david@fromorbit.com> wrote: > > On Wed, Dec 12, 2018 at 11:49:36PM -0500, Nick Bowler wrote: > >> To expand on this, for each structure which my RFC patchset feeds up to > >> the native handler, I first checked them by manual inspection and then > >> double checked using the following program; we can compile with both > >> -mx32 and -m64 and check that the output is identical. > > > > So, turn that into an xfstest so that it is always run, diffs the > > output between compat/native depending on which one is used complete > > with guards that break the test when we add a new ioctl. We already > > we have a test that is for explicitly checking that structures on disk > > are the same for 32/64 bit architectures: tests/xfs/122 > [...] > > Then we'll have tests that will fail if we ever change an ioctl or > > add a new one and don't add it to the test. That guarantees we won't > > ever forget about this.... > > OK, I will give it a shot to implement such a test. A possible issue is > that developers might not have a working x32 build or runtime environment > so the test might not get run a lot. We generally test for the <thing> being supported first and only run the test if it is supported. for x86-64 systems, we should at least already support -m32/-m64, so we'll at least get coverage on that, and the reminder that... > But hopefully people adding brand > new ioctls don't introduce brand new compat problems; one can dream, right? ... introducing new ioctls need thought to ensure we don't introduce new compat problems :) Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-13 21:39 ` Dave Chinner 2018-12-13 21:53 ` Nick Bowler @ 2018-12-14 3:35 ` Nick Bowler 2018-12-14 3:40 ` [RFC PATCH xfstests] xfs: add tests to validate ioctl structure layout Nick Bowler 1 sibling, 1 reply; 26+ messages in thread From: Nick Bowler @ 2018-12-14 3:35 UTC (permalink / raw) To: Dave Chinner; +Cc: Brian Foster, Darrick J. Wong, linux-xfs On 12/13/18, Dave Chinner <david@fromorbit.com> wrote: > That test automates the generation of the test code and output, > and if it changes from the golden output, then the test fails. > I'd suggest that a similar thing is done here for /all/ the > structures we expose in ioctls. > > FWIW, pahole can make this easy. e.g you can harvest every ioctl > structure from xfs_fs.h, OK, I did just that, by snarfing up the preprocessor output from #including "xfs/xfs_fs.h" and searching for 'struct'. The pahole tool was previously unknown to me. I like it. One thing I'll note is that it does not appear to be possible to use pahole to get information about structs that don't have a tag. The xfs_fsid_t typedef is such a type. Fortunately that one does not look like a problem! Cheers, Nick ^ permalink raw reply [flat|nested] 26+ messages in thread
* [RFC PATCH xfstests] xfs: add tests to validate ioctl structure layout. 2018-12-14 3:35 ` Nick Bowler @ 2018-12-14 3:40 ` Nick Bowler 2019-01-15 15:55 ` Luis Chamberlain 0 siblings, 1 reply; 26+ messages in thread From: Nick Bowler @ 2018-12-14 3:40 UTC (permalink / raw) To: linux-xfs; +Cc: Brian Foster, Dave Chinner, Darrick J. Wong Introduce two new test cases. The first one greps xfs_fs.h to find any structure types possibly relevant to ioctls, then compares the structure layouts between 64-bit and 32-bit objects. The goal is to ensure that any _new_ structure has the same characteristics on 32-bit versus 64-bit mode, ensuring a sane compat handler. This requires a toolchain that can build both 64-bit and 32-bit objects, and hopefully works on non-x86 platforms with compat woes. The whitelist was constructed by manually inspecting the current compat ioctl implementation. The test validates that everything in the whitelist actually got found by the grepping as a sort of self-sanity check, but if older versions lacked some of these types then this might need tweaking a bit. The second test validates that a well-known set of structures on x32 crrectly match the ia32 or amd64 layouts as expected. This requires a toolchain that can build for all three ABIs. The idea is that new additions to the header where the structures differ in 32 and 64-bit x86 will be caught by the first test case and that will be sufficient to detect potential x32 problems too. Signed-off-by: Nick Bowler <nbowler@draconx.ca> --- .gitignore | 1 + common/config | 1 + configure.ac | 2 +- include/build-env.in | 6 +++ tests/xfs/499 | 102 +++++++++++++++++++++++++++++++++++++++++++++++++ tests/xfs/499.out | 16 ++++++++ tests/xfs/500 | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/xfs/500.out | 2 + tests/xfs/group | 2 + 9 files changed, 236 insertions(+), 1 deletion(-) create mode 100644 include/build-env.in create mode 100755 tests/xfs/499 create mode 100644 tests/xfs/499.out create mode 100755 tests/xfs/500 create mode 100644 tests/xfs/500.out diff --git a/.gitignore b/.gitignore index ea1aac8a..7bfd20fe 100644 --- a/.gitignore +++ b/.gitignore @@ -28,6 +28,7 @@ /ltmain.sh # build system +/include/build-env /include/builddefs /include/config.h /include/config.h.in diff --git a/common/config b/common/config index a87cb4a2..707c5b2f 100644 --- a/common/config +++ b/common/config @@ -194,6 +194,7 @@ export GETCAP_PROG="$(type -P getcap)" export CHECKBASHISMS_PROG="$(type -P checkbashisms)" export XFS_INFO_PROG="$(type -P xfs_info)" export DUPEREMOVE_PROG="$(type -P duperemove)" +export PAHOLE_PROG="$(type -P pahole)" # use 'udevadm settle' or 'udevsettle' to wait for lv to be settled. # newer systems have udevadm command but older systems like RHEL5 don't. diff --git a/configure.ac b/configure.ac index 19798824..69a4e396 100644 --- a/configure.ac +++ b/configure.ac @@ -73,5 +73,5 @@ AC_HAVE_COPY_FILE_RANGE AC_CHECK_FUNCS([renameat2]) AC_CONFIG_HEADER(include/config.h) -AC_CONFIG_FILES([include/builddefs]) +AC_CONFIG_FILES([include/builddefs include/build-env]) AC_OUTPUT diff --git a/include/build-env.in b/include/build-env.in new file mode 100644 index 00000000..9a9a6270 --- /dev/null +++ b/include/build-env.in @@ -0,0 +1,6 @@ +# Generated by config.status to export build toolchain information +# for use in testcases. + +: "${CC_PROG=`command -v @CC@`}" +: "${CPPFLAGS=@CPPFLAGS@}" +: "${CFLAGS=@CFLAGS@}" diff --git a/tests/xfs/499 b/tests/xfs/499 new file mode 100755 index 00000000..48df2711 --- /dev/null +++ b/tests/xfs/499 @@ -0,0 +1,102 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2018 Nick Bowler. All Rights Reserved. +# +# Scan header files for userspace-visible structures and look for newly +# introduced compat problems between 32 and 64 bit flavours. Anything +# not on a whitelist is expected to have identical representation on +# both versions. + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter +. ./include/build-env + +# remove previous $seqres.full before test +rm -f $seqres.full + +_require_command "$CC_PROG" cc +_require_command "$PAHOLE_PROG" pahole + +compile() +{ + $CC_PROG $CPPFLAGS $CFLAGS "$@" +} + +# Toolchain check! +compile -m64 -include xfs/xfs.h -gdwarf-2 -c -xc /dev/null -o /dev/null || + _notrun 'cannot build 64-bit objects' +compile -m32 -D_FILE_OFFSET_BITS=64 -include xfs/xfs.h -gdwarf-2 -c \ + -xc /dev/null -o /dev/null || _notrun 'cannot build 32-bit objects' + +# Collect XFS types from "xfs/xfs_fs.h". We look for struct tags and +# identifiers that look like xfs_xyz_t under the assumption that all +# ioctl-relevant structures are at least _named_ in this file. +compile -E -include xfs/xfs_fs.h -xc /dev/null >"$tmp.hdump" || + _notrun 'cannot #include "xfs/xfs_fs.h"' + +sed -nf- "$tmp.hdump" <<'EOF' | LC_ALL=C sort -u >"$tmp.types" + h + s/.*[^_[:alpha:]]\(xfs_[_[:alnum:]]*_t\).*/\1/p + g + s/.*struct \([_[:alpha:]][_[:alnum:]]*\).*/struct \1/p +EOF + +# Create a source file containing definitions of objects for every type we +# found, enabling pahole to report on those types. +{ printf '#include "xfs/xfs.h"\n' + + n=0 + exec 3<"$tmp.types" 4>"$tmp.mustmatch" + while read t <&3; do + case $t in + # Whitelist structures known to potentially differ between + # 32-bit and 64-bit compilations. NOTE: if new structures + # get added to this list it is probably necessary to update + # xfs/500 test case as well to set the expected x32 behaviour. + "struct xfs_attr_multiop" | \ + "struct xfs_bstat" | \ + "struct xfs_bstime" | \ + "struct xfs_flock64" | \ + "struct xfs_fsop_attrlist_handlereq" | \ + "struct xfs_fsop_attrmulti_handlereq" | \ + "struct xfs_fsop_bulkreq" | \ + "struct xfs_fsop_geom_v1" | \ + "struct xfs_fsop_handlereq" | \ + "struct xfs_fsop_setdm_handlereq" | \ + "struct xfs_growfs_data" | \ + "struct xfs_growfs_rt" | \ + "struct xfs_inogrp" | \ + "struct xfs_swapext") + printf 'whitelist %s\n' "$t" 1>&2 ;; + *) printf '%s\n' "${t#struct }" >&4 ;; # everything else must match! + esac + printf '%s var%d = {0};\n' "$t" "$n" + n=$((n+1)) + done + exec 3<&- 4>&- + } >"$tmp.test.c" + +compile -m32 -D_FILE_OFFSET_BITS=64 -gdwarf-2 -c "$tmp.test.c" -o "$tmp.32.o" +$PAHOLE_PROG --class_name="file://$tmp.mustmatch" "$tmp.32.o" >"$tmp.32.dat" + +compile -m64 -gdwarf-2 -c "$tmp.test.c" -o "$tmp.64.o" +$PAHOLE_PROG --class_name="file://$tmp.mustmatch" "$tmp.64.o" >"$tmp.64.dat" + +printf 'Silence is golden\n' +diff -u "$tmp.32.dat" "$tmp.64.dat" +status=$? diff --git a/tests/xfs/499.out b/tests/xfs/499.out new file mode 100644 index 00000000..80e26046 --- /dev/null +++ b/tests/xfs/499.out @@ -0,0 +1,16 @@ +QA output created by 499 +whitelist struct xfs_attr_multiop +whitelist struct xfs_bstat +whitelist struct xfs_bstime +whitelist struct xfs_flock64 +whitelist struct xfs_fsop_attrlist_handlereq +whitelist struct xfs_fsop_attrmulti_handlereq +whitelist struct xfs_fsop_bulkreq +whitelist struct xfs_fsop_geom_v1 +whitelist struct xfs_fsop_handlereq +whitelist struct xfs_fsop_setdm_handlereq +whitelist struct xfs_growfs_data +whitelist struct xfs_growfs_rt +whitelist struct xfs_inogrp +whitelist struct xfs_swapext +Silence is golden diff --git a/tests/xfs/500 b/tests/xfs/500 new file mode 100755 index 00000000..80d37b0f --- /dev/null +++ b/tests/xfs/500 @@ -0,0 +1,105 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2018 Nick Bowler. All Rights Reserved. +# +# FS QA Test 500 +# +# Check that x32 ioctl-related structures are aligned with the ia32 or +# amd64 variants as expected. + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter +. ./include/build-env + +# remove previous $seqres.full before test +rm -f $seqres.full + +_require_command "$CC_PROG" cc +_require_command "$PAHOLE_PROG" pahole + +compile() +{ + $CC_PROG $CPPFLAGS $CFLAGS "$@" +} + +# Toolchain check! +compile -mx32 -include xfs/xfs.h -gdwarf-2 -c -xc /dev/null -o /dev/null || + _notrun 'cannot build x32 objects' +compile -m64 -include xfs/xfs.h -gdwarf-2 -c -xc /dev/null -o /dev/null || + _notrun 'cannot build amd64 objects' +compile -m32 -D_FILE_OFFSET_BITS=64 -include xfs/xfs.h -gdwarf-2 -c \ + -xc /dev/null -o /dev/null || _notrun 'cannot build ia32 objects' + +# struct tags where x32 is expected to match ia32 layout ... +cat >"$tmp.ia32structs" <<'EOF' +fsdmidata +xfs_attr_multiop +xfs_attrlist_cursor +xfs_fsop_attrlist_handlereq +xfs_fsop_attrmulti_handlereq +xfs_fsop_bulkreq +xfs_fsop_handlereq +xfs_fsop_setdm_handlereq +EOF + +# ... and the tags where x32 is expected to match amd64. +cat >"$tmp.amd64structs" <<'EOF' +fsdmidata +xfs_attrlist_cursor +xfs_bstat +xfs_bstime +xfs_flock64 +xfs_fsop_geom_v1 +xfs_growfs_data +xfs_growfs_rt +xfs_inogrp +xfs_swapext +EOF + +# Create source file containing definitions of objects with each type we care +# about, in order to produce relevant debug information which will be checked +# by pahole. +{ printf '#include "xfs/xfs.h"\n' + + n=0 + cat "$tmp.ia32structs" "$tmp.amd64structs" | while read l; do + printf 'struct %s var%d = {0};\n' "$l" "$n" + n=$((n+1)) + done + } >"$tmp.test.c" + +compile -m32 -D_FILE_OFFSET_BITS=64 -gdwarf-2 -c "$tmp.test.c" -o "$tmp.ia32.o" +compile -mx32 -gdwarf-2 -c "$tmp.test.c" -o "$tmp.x32.o" +compile -m64 -gdwarf-2 -c "$tmp.test.c" -o "$tmp.amd64.o" + +$PAHOLE_PROG --class_name="file://$tmp.ia32structs" "$tmp.ia32.o" \ + >"$tmp.ia32.dat" +$PAHOLE_PROG --class_name="file://$tmp.ia32structs" "$tmp.x32.o" \ + >"$tmp.x32-ia32.dat" + +$PAHOLE_PROG --class_name="file://$tmp.amd64structs" "$tmp.amd64.o" \ + >"$tmp.amd64.dat" +$PAHOLE_PROG --class_name="file://$tmp.amd64structs" "$tmp.x32.o" \ + >"$tmp.x32-amd64.dat" + +printf 'Silence is golden\n' + +ret=0 +diff -u "$tmp.ia32.dat" "$tmp.x32-ia32.dat" || ret=1 +diff -u "$tmp.amd64.dat" "$tmp.x32-amd64.dat" || ret=1 +status=$ret diff --git a/tests/xfs/500.out b/tests/xfs/500.out new file mode 100644 index 00000000..883b2caf --- /dev/null +++ b/tests/xfs/500.out @@ -0,0 +1,2 @@ +QA output created by 500 +Silence is golden diff --git a/tests/xfs/group b/tests/xfs/group index dfaae2bc..cb69d28a 100644 --- a/tests/xfs/group +++ b/tests/xfs/group @@ -496,3 +496,5 @@ 496 dangerous_fuzzers dangerous_scrub dangerous_repair 497 dangerous_fuzzers dangerous_scrub dangerous_online_repair 498 dangerous_fuzzers dangerous_norepair +499 auto quick +500 auto quick -- 2.16.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [RFC PATCH xfstests] xfs: add tests to validate ioctl structure layout. 2018-12-14 3:40 ` [RFC PATCH xfstests] xfs: add tests to validate ioctl structure layout Nick Bowler @ 2019-01-15 15:55 ` Luis Chamberlain 0 siblings, 0 replies; 26+ messages in thread From: Luis Chamberlain @ 2019-01-15 15:55 UTC (permalink / raw) To: Nick Bowler; +Cc: linux-xfs, Brian Foster, Dave Chinner, Darrick J. Wong On Thu, Dec 13, 2018 at 10:40:53PM -0500, Nick Bowler wrote: > Introduce two new test cases. The first one greps xfs_fs.h to find > any structure types possibly relevant to ioctls, then compares the > structure layouts between 64-bit and 32-bit objects. The goal is to > ensure that any _new_ structure has the same characteristics on 32-bit > versus 64-bit mode, ensuring a sane compat handler. This requires a > toolchain that can build both 64-bit and 32-bit objects, and hopefully > works on non-x86 platforms with compat woes. > > The whitelist was constructed by manually inspecting the current > compat ioctl implementation. The test validates that everything > in the whitelist actually got found by the grepping as a sort of > self-sanity check, but if older versions lacked some of these > types then this might need tweaking a bit. > > The second test validates that a well-known set of structures on x32 > crrectly match the ia32 or amd64 layouts as expected. This requires > a toolchain that can build for all three ABIs. > > The idea is that new additions to the header where the structures > differ in 32 and 64-bit x86 will be caught by the first test case > and that will be sufficient to detect potential x32 problems too. > > Signed-off-by: Nick Bowler <nbowler@draconx.ca> I believe this effort was dropped. Respin? Luis ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device 2018-12-13 3:53 ` Dave Chinner 2018-12-13 4:14 ` Nick Bowler @ 2018-12-13 16:30 ` Darrick J. Wong 1 sibling, 0 replies; 26+ messages in thread From: Darrick J. Wong @ 2018-12-13 16:30 UTC (permalink / raw) To: Dave Chinner; +Cc: Nick Bowler, Brian Foster, linux-xfs 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@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". 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2019-01-15 15:56 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-10 4:29 Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device Nick Bowler 2018-12-10 14:33 ` Brian Foster 2018-12-10 15:39 ` Nick Bowler 2018-12-10 16:11 ` Brian Foster 2018-12-10 16:50 ` Darrick J. Wong 2018-12-10 16:55 ` Darrick J. Wong 2018-12-10 17:46 ` Brian Foster 2018-12-10 20:54 ` Nick Bowler 2018-12-10 21:41 ` Dave Chinner 2018-12-11 7:04 ` Nick Bowler 2018-12-11 12:27 ` Brian Foster 2018-12-11 20:13 ` Nick Bowler 2018-12-11 20:20 ` Nick Bowler 2018-12-12 13:09 ` Brian Foster 2018-12-13 0:21 ` Nick Bowler 2018-12-12 4:56 ` Nick Bowler 2018-12-13 3:53 ` Dave Chinner 2018-12-13 4:14 ` Nick Bowler 2018-12-13 4:49 ` Nick Bowler 2018-12-13 21:39 ` Dave Chinner 2018-12-13 21:53 ` Nick Bowler 2018-12-14 1:43 ` Dave Chinner 2018-12-14 3:35 ` Nick Bowler 2018-12-14 3:40 ` [RFC PATCH xfstests] xfs: add tests to validate ioctl structure layout Nick Bowler 2019-01-15 15:55 ` Luis Chamberlain 2018-12-13 16:30 ` Enlarging w/ xfs_growfs: XFS_IOC_FSGROWFSDATA xfsctl failed: Inappropriate ioctl for device Darrick J. Wong
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.