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

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

[Nick Bowler]

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

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

[Brian Foster]

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

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

[Nick Bowler]

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

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

[Brian Foster]

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

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

[Darrick J. Wong]

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

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

[Darrick J. Wong]

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

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

[Brian Foster]

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

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

[Nick Bowler]

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

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

[Dave Chinner]

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

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

[Nick Bowler]

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:

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

[Brian Foster]

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:

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

[Nick Bowler]

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

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

[Nick Bowler]

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

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

[Nick Bowler]

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;

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

[Brian Foster]

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

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

[Nick Bowler]

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

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

[Dave Chinner]

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

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

[Nick Bowler]

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.

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

[Nick Bowler]

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);
  }

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

[Darrick J. Wong]

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

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

[Dave Chinner]

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

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

[Nick Bowler]

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

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

[Dave Chinner]

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

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

[Nick Bowler]

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

[RFC PATCH xfstests] xfs: add tests to validate ioctl structure layout.

[Nick Bowler]

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

Re: [RFC PATCH xfstests] xfs: add tests to validate ioctl structure layout.

[Luis Chamberlain]

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