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@xxxxxxxxxx> 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
