xfs: Fix xfs_dir2_sf_entry_t size check
diff mbox series

Message ID 20200109141459.21808-1-vincenzo.frascino@arm.com
State New
Headers show
Series
  • xfs: Fix xfs_dir2_sf_entry_t size check
Related show

Commit Message

Vincenzo Frascino Jan. 9, 2020, 2:14 p.m. UTC
xfs_check_ondisk_structs() verifies that the sizes of the data types
used by xfs are correct via the XFS_CHECK_STRUCT_SIZE() macro.

xfs_dir2_sf_entry_t size is set erroneously to 3 which breaks the
compilation with the assertion below:

In file included from linux/include/linux/string.h:6,
                 from linux/include/linux/uuid.h:12,
                 from linux/fs/xfs/xfs_linux.h:10,
                 from linux/fs/xfs/xfs.h:22,
                 from linux/fs/xfs/xfs_super.c:7:
In function ‘xfs_check_ondisk_structs’,
    inlined from ‘init_xfs_fs’ at linux/fs/xfs/xfs_super.c:2025:2:
linux/include/linux/compiler.h:350:38:
    error: call to ‘__compiletime_assert_107’ declared with attribute
    error: XFS: sizeof(xfs_dir2_sf_entry_t) is wrong, expected 3
    _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)

Restore the correct behavior defining the correct size.

Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 fs/xfs/xfs_ondisk.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Eric Sandeen Jan. 9, 2020, 3:01 p.m. UTC | #1
On 1/9/20 8:14 AM, Vincenzo Frascino wrote:
> xfs_check_ondisk_structs() verifies that the sizes of the data types
> used by xfs are correct via the XFS_CHECK_STRUCT_SIZE() macro.
> 
> xfs_dir2_sf_entry_t size is set erroneously to 3 which breaks the
> compilation with the assertion below:
> 
> In file included from linux/include/linux/string.h:6,
>                  from linux/include/linux/uuid.h:12,
>                  from linux/fs/xfs/xfs_linux.h:10,
>                  from linux/fs/xfs/xfs.h:22,
>                  from linux/fs/xfs/xfs_super.c:7:
> In function ‘xfs_check_ondisk_structs’,
>     inlined from ‘init_xfs_fs’ at linux/fs/xfs/xfs_super.c:2025:2:
> linux/include/linux/compiler.h:350:38:
>     error: call to ‘__compiletime_assert_107’ declared with attribute
>     error: XFS: sizeof(xfs_dir2_sf_entry_t) is wrong, expected 3
>     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> 
> Restore the correct behavior defining the correct size.

# pahole -C xfs_dir2_sf_entry fs/xfs/xfs.o 

struct xfs_dir2_sf_entry {
	__u8                       namelen;              /*     0     1 */
	__u8                       offset[2];            /*     1     2 */
	__u8                       name[0];              /*     3     0 */

	/* size: 3, cachelines: 1, members: 3 */
	/* last cacheline: 3 bytes */
};

Can you please the same command on your machine, along with which arm abi is
in use etc just for clarity?

-Eric

> Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
>  fs/xfs/xfs_ondisk.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index b6701b4f59a9..ee487ddc60c7 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -104,7 +104,7 @@ xfs_check_ondisk_structs(void)
>  	XFS_CHECK_STRUCT_SIZE(xfs_dir2_leaf_hdr_t,		16);
>  	XFS_CHECK_STRUCT_SIZE(xfs_dir2_leaf_t,			16);
>  	XFS_CHECK_STRUCT_SIZE(xfs_dir2_leaf_tail_t,		4);
> -	XFS_CHECK_STRUCT_SIZE(xfs_dir2_sf_entry_t,		3);
> +	XFS_CHECK_STRUCT_SIZE(xfs_dir2_sf_entry_t,		4);
>  	XFS_CHECK_OFFSET(xfs_dir2_sf_entry_t, namelen,		0);
>  	XFS_CHECK_OFFSET(xfs_dir2_sf_entry_t, offset,		1);
>  	XFS_CHECK_OFFSET(xfs_dir2_sf_entry_t, name,		3);
>
Vincenzo Frascino Jan. 9, 2020, 3:35 p.m. UTC | #2
Hi Eric,

On 09/01/2020 15:01, Eric Sandeen wrote:
> On 1/9/20 8:14 AM, Vincenzo Frascino wrote:
>> xfs_check_ondisk_structs() verifies that the sizes of the data types
>> used by xfs are correct via the XFS_CHECK_STRUCT_SIZE() macro.
>>
>> xfs_dir2_sf_entry_t size is set erroneously to 3 which breaks the
>> compilation with the assertion below:
>>
>> In file included from linux/include/linux/string.h:6,
>>                  from linux/include/linux/uuid.h:12,
>>                  from linux/fs/xfs/xfs_linux.h:10,
>>                  from linux/fs/xfs/xfs.h:22,
>>                  from linux/fs/xfs/xfs_super.c:7:
>> In function ‘xfs_check_ondisk_structs’,
>>     inlined from ‘init_xfs_fs’ at linux/fs/xfs/xfs_super.c:2025:2:
>> linux/include/linux/compiler.h:350:38:
>>     error: call to ‘__compiletime_assert_107’ declared with attribute
>>     error: XFS: sizeof(xfs_dir2_sf_entry_t) is wrong, expected 3
>>     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
>>
>> Restore the correct behavior defining the correct size.
> 
> # pahole -C xfs_dir2_sf_entry fs/xfs/xfs.o 
> 
> struct xfs_dir2_sf_entry {
> 	__u8                       namelen;              /*     0     1 */
> 	__u8                       offset[2];            /*     1     2 */
> 	__u8                       name[0];              /*     3     0 */
> 
> 	/* size: 3, cachelines: 1, members: 3 */
> 	/* last cacheline: 3 bytes */
> };
> 
> Can you please the same command on your machine, along with which arm abi is
> in use etc just for clarity?
>

The abi is arm32 eabihf. You can reproduce my scenario using randconfig with
seed 0x72F68201.

In this case I get size 4, hence my patch.

If I enable xfs on the defconfig though size is 3 accordingly to what you have
reported. I will continue the investigation.

Vincenzo

> -Eric
> 
>> Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
>> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
>> ---
>>  fs/xfs/xfs_ondisk.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
>> index b6701b4f59a9..ee487ddc60c7 100644
>> --- a/fs/xfs/xfs_ondisk.h
>> +++ b/fs/xfs/xfs_ondisk.h
>> @@ -104,7 +104,7 @@ xfs_check_ondisk_structs(void)
>>  	XFS_CHECK_STRUCT_SIZE(xfs_dir2_leaf_hdr_t,		16);
>>  	XFS_CHECK_STRUCT_SIZE(xfs_dir2_leaf_t,			16);
>>  	XFS_CHECK_STRUCT_SIZE(xfs_dir2_leaf_tail_t,		4);
>> -	XFS_CHECK_STRUCT_SIZE(xfs_dir2_sf_entry_t,		3);
>> +	XFS_CHECK_STRUCT_SIZE(xfs_dir2_sf_entry_t,		4);
>>  	XFS_CHECK_OFFSET(xfs_dir2_sf_entry_t, namelen,		0);
>>  	XFS_CHECK_OFFSET(xfs_dir2_sf_entry_t, offset,		1);
>>  	XFS_CHECK_OFFSET(xfs_dir2_sf_entry_t, name,		3);
>>
Darrick J. Wong Jan. 9, 2020, 4:50 p.m. UTC | #3
On Thu, Jan 09, 2020 at 03:35:46PM +0000, Vincenzo Frascino wrote:
> Hi Eric,
> 
> On 09/01/2020 15:01, Eric Sandeen wrote:
> > On 1/9/20 8:14 AM, Vincenzo Frascino wrote:
> >> xfs_check_ondisk_structs() verifies that the sizes of the data types
> >> used by xfs are correct via the XFS_CHECK_STRUCT_SIZE() macro.
> >>
> >> xfs_dir2_sf_entry_t size is set erroneously to 3 which breaks the
> >> compilation with the assertion below:
> >>
> >> In file included from linux/include/linux/string.h:6,
> >>                  from linux/include/linux/uuid.h:12,
> >>                  from linux/fs/xfs/xfs_linux.h:10,
> >>                  from linux/fs/xfs/xfs.h:22,
> >>                  from linux/fs/xfs/xfs_super.c:7:
> >> In function ‘xfs_check_ondisk_structs’,
> >>     inlined from ‘init_xfs_fs’ at linux/fs/xfs/xfs_super.c:2025:2:
> >> linux/include/linux/compiler.h:350:38:
> >>     error: call to ‘__compiletime_assert_107’ declared with attribute
> >>     error: XFS: sizeof(xfs_dir2_sf_entry_t) is wrong, expected 3

So, working as expected -- with size == 4 the directory metadata block
pointer calculations will be incorrect, and you'll end up with a corrupt
filesystem.

> >>     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> >>
> >> Restore the correct behavior defining the correct size.
> > 
> > # pahole -C xfs_dir2_sf_entry fs/xfs/xfs.o 
> > 
> > struct xfs_dir2_sf_entry {
> > 	__u8                       namelen;              /*     0     1 */
> > 	__u8                       offset[2];            /*     1     2 */
> > 	__u8                       name[0];              /*     3     0 */

This sounds like gcc getting confused by the zero length array.  Though
it's odd that randconfig breaks, but defconfig doesn't?  This sounds
like one of the kernel gcc options causing problems.

> > 
> > 	/* size: 3, cachelines: 1, members: 3 */
> > 	/* last cacheline: 3 bytes */
> > };
> > 
> > Can you please the same command on your machine, along with which arm abi is
> > in use etc just for clarity?
> >
> 
> The abi is arm32 eabihf. You can reproduce my scenario using randconfig with
> seed 0x72F68201.

Please send the actual .config file produced by randconfig 72f68201...

> In this case I get size 4, hence my patch.
> 
> If I enable xfs on the defconfig though size is 3 accordingly to what you have
> reported. I will continue the investigation.

...and the .config file produced by defconfig, in the hopes that someone
will spot the culprit using differential analysis.  Assuming you haven't
done that already.

--D

> Vincenzo
> 
> > -Eric
> > 
> >> Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
> >> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> >> ---
> >>  fs/xfs/xfs_ondisk.h | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> >> index b6701b4f59a9..ee487ddc60c7 100644
> >> --- a/fs/xfs/xfs_ondisk.h
> >> +++ b/fs/xfs/xfs_ondisk.h
> >> @@ -104,7 +104,7 @@ xfs_check_ondisk_structs(void)
> >>  	XFS_CHECK_STRUCT_SIZE(xfs_dir2_leaf_hdr_t,		16);
> >>  	XFS_CHECK_STRUCT_SIZE(xfs_dir2_leaf_t,			16);
> >>  	XFS_CHECK_STRUCT_SIZE(xfs_dir2_leaf_tail_t,		4);
> >> -	XFS_CHECK_STRUCT_SIZE(xfs_dir2_sf_entry_t,		3);
> >> +	XFS_CHECK_STRUCT_SIZE(xfs_dir2_sf_entry_t,		4);
> >>  	XFS_CHECK_OFFSET(xfs_dir2_sf_entry_t, namelen,		0);
> >>  	XFS_CHECK_OFFSET(xfs_dir2_sf_entry_t, offset,		1);
> >>  	XFS_CHECK_OFFSET(xfs_dir2_sf_entry_t, name,		3);
> >>
> 
> -- 
> Regards,
> Vincenzo

pub   RSA 4096/072FD436 2019-09-02 Vincenzo Frascino <vincenzo.frascino@arm.com>
> sub   RSA 2048/4205BF15 2019-09-02
> sub   RSA 2048/296522AA 2019-09-02
> sub   RSA 2048/7CAB726B 2019-09-02
>
Vincenzo Frascino Jan. 9, 2020, 5 p.m. UTC | #4
Hi Darrick,

On 09/01/2020 16:50, Darrick J. Wong wrote:
> This sounds like gcc getting confused by the zero length array.  Though
> it's odd that randconfig breaks, but defconfig doesn't?  This sounds
> like one of the kernel gcc options causing problems.
>

This is what I started suspecting as well.

>>> 	/* size: 3, cachelines: 1, members: 3 */
>>> 	/* last cacheline: 3 bytes */
>>> };
>>>
>>> Can you please the same command on your machine, along with which arm abi is
>>> in use etc just for clarity?
>>>
>> The abi is arm32 eabihf. You can reproduce my scenario using randconfig with
>> seed 0x72F68201.
> Please send the actual .config file produced by randconfig 72f68201...
> 
>> In this case I get size 4, hence my patch.
>>
>> If I enable xfs on the defconfig though size is 3 accordingly to what you have
>> reported. I will continue the investigation.
> ...and the .config file produced by defconfig, in the hopes that someone
> will spot the culprit using differential analysis.  Assuming you haven't
> done that already.

I did not spot anything unusual yet. I am attaching the files so that someone
else can have a look in the meantime.
kbuild test robot Jan. 12, 2020, 12:44 a.m. UTC | #5
Hi Vincenzo,

I love your patch! Yet something to improve:

[auto build test ERROR on xfs-linux/for-next]
[also build test ERROR on djwong-xfs/djwong-devel v5.5-rc5 next-20200110]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Vincenzo-Frascino/xfs-Fix-xfs_dir2_sf_entry_t-size-check/20200110-144608
base:   https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git for-next
config: x86_64-lkp (attached as .config)
compiler: gcc-7 (Debian 7.5.0-3) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/string.h:6:0,
                    from include/linux/uuid.h:12,
                    from fs/xfs/xfs_linux.h:10,
                    from fs/xfs/xfs.h:22,
                    from fs/xfs/xfs_super.c:7:
   In function 'xfs_check_ondisk_structs',
       inlined from 'init_xfs_fs' at fs/xfs/xfs_super.c:2025:2:
>> include/linux/compiler.h:350:38: error: call to '__compiletime_assert_107' declared with attribute error: XFS: sizeof(xfs_dir2_sf_entry_t) is wrong, expected 4
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
                                         ^
   include/linux/compiler.h:331:4: note: in definition of macro '__compiletime_assert'
       prefix ## suffix();    \
       ^~~~~~
   include/linux/compiler.h:350:2: note: in expansion of macro '_compiletime_assert'
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
     ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
    #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
                                        ^~~~~~~~~~~~~~~~~~
   fs/xfs/xfs_ondisk.h:10:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
     BUILD_BUG_ON_MSG(sizeof(structname) != (size), "XFS: sizeof(" \
     ^~~~~~~~~~~~~~~~
   fs/xfs/xfs_ondisk.h:107:2: note: in expansion of macro 'XFS_CHECK_STRUCT_SIZE'
     XFS_CHECK_STRUCT_SIZE(xfs_dir2_sf_entry_t,  4);
     ^~~~~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/string.h:6:0,
                    from include/linux/uuid.h:12,
                    from fs//xfs/xfs_linux.h:10,
                    from fs//xfs/xfs.h:22,
                    from fs//xfs/xfs_super.c:7:
   In function 'xfs_check_ondisk_structs',
       inlined from 'init_xfs_fs' at fs//xfs/xfs_super.c:2025:2:
>> include/linux/compiler.h:350:38: error: call to '__compiletime_assert_107' declared with attribute error: XFS: sizeof(xfs_dir2_sf_entry_t) is wrong, expected 4
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
                                         ^
   include/linux/compiler.h:331:4: note: in definition of macro '__compiletime_assert'
       prefix ## suffix();    \
       ^~~~~~
   include/linux/compiler.h:350:2: note: in expansion of macro '_compiletime_assert'
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
     ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
    #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
                                        ^~~~~~~~~~~~~~~~~~
   fs//xfs/xfs_ondisk.h:10:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
     BUILD_BUG_ON_MSG(sizeof(structname) != (size), "XFS: sizeof(" \
     ^~~~~~~~~~~~~~~~
   fs//xfs/xfs_ondisk.h:107:2: note: in expansion of macro 'XFS_CHECK_STRUCT_SIZE'
     XFS_CHECK_STRUCT_SIZE(xfs_dir2_sf_entry_t,  4);
     ^~~~~~~~~~~~~~~~~~~~~

vim +/__compiletime_assert_107 +350 include/linux/compiler.h

9a8ab1c39970a4 Daniel Santos 2013-02-21  336  
9a8ab1c39970a4 Daniel Santos 2013-02-21  337  #define _compiletime_assert(condition, msg, prefix, suffix) \
9a8ab1c39970a4 Daniel Santos 2013-02-21  338  	__compiletime_assert(condition, msg, prefix, suffix)
9a8ab1c39970a4 Daniel Santos 2013-02-21  339  
9a8ab1c39970a4 Daniel Santos 2013-02-21  340  /**
9a8ab1c39970a4 Daniel Santos 2013-02-21  341   * compiletime_assert - break build and emit msg if condition is false
9a8ab1c39970a4 Daniel Santos 2013-02-21  342   * @condition: a compile-time constant condition to check
9a8ab1c39970a4 Daniel Santos 2013-02-21  343   * @msg:       a message to emit if condition is false
9a8ab1c39970a4 Daniel Santos 2013-02-21  344   *
9a8ab1c39970a4 Daniel Santos 2013-02-21  345   * In tradition of POSIX assert, this macro will break the build if the
9a8ab1c39970a4 Daniel Santos 2013-02-21  346   * supplied condition is *false*, emitting the supplied error message if the
9a8ab1c39970a4 Daniel Santos 2013-02-21  347   * compiler has support to do so.
9a8ab1c39970a4 Daniel Santos 2013-02-21  348   */
9a8ab1c39970a4 Daniel Santos 2013-02-21  349  #define compiletime_assert(condition, msg) \
9a8ab1c39970a4 Daniel Santos 2013-02-21 @350  	_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
9a8ab1c39970a4 Daniel Santos 2013-02-21  351  

:::::: The code at line 350 was first introduced by commit
:::::: 9a8ab1c39970a4938a72d94e6fd13be88a797590 bug.h, compiler.h: introduce compiletime_assert & BUILD_BUG_ON_MSG

:::::: TO: Daniel Santos <daniel.santos@pobox.com>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
Arnd Bergmann Jan. 13, 2020, 1:55 p.m. UTC | #6
On Thu, Jan 9, 2020 at 10:01 PM Vincenzo Frascino
<vincenzo.frascino@arm.com> wrote:
>
> Hi Darrick,
>
> On 09/01/2020 16:50, Darrick J. Wong wrote:
> > This sounds like gcc getting confused by the zero length array.  Though
> > it's odd that randconfig breaks, but defconfig doesn't?  This sounds
> > like one of the kernel gcc options causing problems.
> >
>
> This is what I started suspecting as well.

The important bit into the configuration is

# CONFIG_AEABI is not set

With ARM OABI (which you get when EABI is disabled), structures are padded
to multiples of 32 bits. See commits 8353a649f577 ("xfs: kill
xfs_dir2_sf_off_t")
and aa2dd0ad4d6d ("xfs: remove __arch_pack"). Those could be partially
reverted to fix it again, but it doesn't seem worth it as there is
probably nobody
running XFS on OABI machines (actually with the build failure we can
be fairly sure there isn't ;-).

I am testing randconfig builds with OABI and a few other things like ARCH_RPC
disabled because of random issues like this.

      Arnd
Christoph Hellwig Jan. 13, 2020, 1:58 p.m. UTC | #7
On Mon, Jan 13, 2020 at 02:55:15PM +0100, Arnd Bergmann wrote:
> With ARM OABI (which you get when EABI is disabled), structures are padded
> to multiples of 32 bits. See commits 8353a649f577 ("xfs: kill
> xfs_dir2_sf_off_t")
> and aa2dd0ad4d6d ("xfs: remove __arch_pack"). Those could be partially
> reverted to fix it again, but it doesn't seem worth it as there is
> probably nobody
> running XFS on OABI machines (actually with the build failure we can
> be fairly sure there isn't ;-).

Or just try adding a __packed to the xfs_dir2_sf_entry definition?
Vincenzo Frascino Jan. 13, 2020, 2:05 p.m. UTC | #8
Hi Arnd,

On 1/13/20 1:55 PM, Arnd Bergmann wrote:
> On Thu, Jan 9, 2020 at 10:01 PM Vincenzo Frascino
> <vincenzo.frascino@arm.com> wrote:
>>
>> Hi Darrick,
>>
>> On 09/01/2020 16:50, Darrick J. Wong wrote:
>>> This sounds like gcc getting confused by the zero length array.  Though
>>> it's odd that randconfig breaks, but defconfig doesn't?  This sounds
>>> like one of the kernel gcc options causing problems.
>>>
>>
>> This is what I started suspecting as well.
> 
> The important bit into the configuration is
> 
> # CONFIG_AEABI is not set
> 
> With ARM OABI (which you get when EABI is disabled), structures are padded
> to multiples of 32 bits. See commits 8353a649f577 ("xfs: kill
> xfs_dir2_sf_off_t")
> and aa2dd0ad4d6d ("xfs: remove __arch_pack"). Those could be partially
> reverted to fix it again, but it doesn't seem worth it as there is
> probably nobody
> running XFS on OABI machines (actually with the build failure we can
> be fairly sure there isn't ;-).
> 

Thanks for this, for some reasons I was convinced that CONFIG_AEABI was set in
this configuration file as I reported as well in my previous email.
Since it is OABI makes sense disabling xfs for randconfig purposes.
Arnd Bergmann Jan. 13, 2020, 2:06 p.m. UTC | #9
On Mon, Jan 13, 2020 at 2:58 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Jan 13, 2020 at 02:55:15PM +0100, Arnd Bergmann wrote:
> > With ARM OABI (which you get when EABI is disabled), structures are padded
> > to multiples of 32 bits. See commits 8353a649f577 ("xfs: kill
> > xfs_dir2_sf_off_t")
> > and aa2dd0ad4d6d ("xfs: remove __arch_pack"). Those could be partially
> > reverted to fix it again, but it doesn't seem worth it as there is
> > probably nobody
> > running XFS on OABI machines (actually with the build failure we can
> > be fairly sure there isn't ;-).
>
> Or just try adding a __packed to the xfs_dir2_sf_entry definition?

Yes, that should be correct on all architectures, and I just noticed
that this is what we already have on xfs_dir2_sf_hdr_t directly
above it for the same reason.

       Arnd
Darrick J. Wong Jan. 13, 2020, 5:01 p.m. UTC | #10
On Mon, Jan 13, 2020 at 03:06:50PM +0100, Arnd Bergmann wrote:
> On Mon, Jan 13, 2020 at 2:58 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Mon, Jan 13, 2020 at 02:55:15PM +0100, Arnd Bergmann wrote:
> > > With ARM OABI (which you get when EABI is disabled), structures are padded
> > > to multiples of 32 bits. See commits 8353a649f577 ("xfs: kill
> > > xfs_dir2_sf_off_t")
> > > and aa2dd0ad4d6d ("xfs: remove __arch_pack"). Those could be partially
> > > reverted to fix it again, but it doesn't seem worth it as there is
> > > probably nobody
> > > running XFS on OABI machines (actually with the build failure we can
> > > be fairly sure there isn't ;-).
> >
> > Or just try adding a __packed to the xfs_dir2_sf_entry definition?
> 
> Yes, that should be correct on all architectures, and I just noticed
> that this is what we already have on xfs_dir2_sf_hdr_t directly
> above it for the same reason.

Yeah, that sounds like a reasonable way forward, short of cleaning out
all the array[0] cr^Hode... ;)

To the original submitter: can you add __packed to the structure
definition and (assuming it passes oabi compilation) send that to the
list, please?

--D

> 
>        Arnd
Eric Sandeen Jan. 13, 2020, 5:04 p.m. UTC | #11
On 1/13/20 11:01 AM, Darrick J. Wong wrote:
> On Mon, Jan 13, 2020 at 03:06:50PM +0100, Arnd Bergmann wrote:
>> On Mon, Jan 13, 2020 at 2:58 PM Christoph Hellwig <hch@lst.de> wrote:
>>>
>>> On Mon, Jan 13, 2020 at 02:55:15PM +0100, Arnd Bergmann wrote:
>>>> With ARM OABI (which you get when EABI is disabled), structures are padded
>>>> to multiples of 32 bits. See commits 8353a649f577 ("xfs: kill
>>>> xfs_dir2_sf_off_t")
>>>> and aa2dd0ad4d6d ("xfs: remove __arch_pack"). Those could be partially
>>>> reverted to fix it again, but it doesn't seem worth it as there is
>>>> probably nobody
>>>> running XFS on OABI machines (actually with the build failure we can
>>>> be fairly sure there isn't ;-).
>>>
>>> Or just try adding a __packed to the xfs_dir2_sf_entry definition?
>>
>> Yes, that should be correct on all architectures, and I just noticed
>> that this is what we already have on xfs_dir2_sf_hdr_t directly
>> above it for the same reason.
> 
> Yeah, that sounds like a reasonable way forward, short of cleaning out
> all the array[0] cr^Hode... ;)
> 
> To the original submitter: can you add __packed to the structure
> definition and (assuming it passes oabi compilation) send that to the
> list, please?

Probably worth doing this iteratively until all the build-time size checks
pass on OABI - just to be sure there are no more lurking?

-Eric
Vincenzo Frascino Jan. 13, 2020, 5:26 p.m. UTC | #12
Hi Darrick,

On 1/13/20 5:01 PM, Darrick J. Wong wrote:
> On Mon, Jan 13, 2020 at 03:06:50PM +0100, Arnd Bergmann wrote:
>> On Mon, Jan 13, 2020 at 2:58 PM Christoph Hellwig <hch@lst.de> wrote:
>>>
>>> On Mon, Jan 13, 2020 at 02:55:15PM +0100, Arnd Bergmann wrote:
>>>> With ARM OABI (which you get when EABI is disabled), structures are padded
>>>> to multiples of 32 bits. See commits 8353a649f577 ("xfs: kill
>>>> xfs_dir2_sf_off_t")
>>>> and aa2dd0ad4d6d ("xfs: remove __arch_pack"). Those could be partially
>>>> reverted to fix it again, but it doesn't seem worth it as there is
>>>> probably nobody
>>>> running XFS on OABI machines (actually with the build failure we can
>>>> be fairly sure there isn't ;-).
>>>
>>> Or just try adding a __packed to the xfs_dir2_sf_entry definition?
>>
>> Yes, that should be correct on all architectures, and I just noticed
>> that this is what we already have on xfs_dir2_sf_hdr_t directly
>> above it for the same reason.
> 
> Yeah, that sounds like a reasonable way forward, short of cleaning out
> all the array[0] cr^Hode... ;)
> 
> To the original submitter: can you add __packed to the structure
> definition and (assuming it passes oabi compilation) send that to the
> list, please?
> 

I will test it tomorrow morning as first thing and will send a patch out.
Thank you all for your help!

> --D
> 
>>
>>        Arnd

Patch
diff mbox series

diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
index b6701b4f59a9..ee487ddc60c7 100644
--- a/fs/xfs/xfs_ondisk.h
+++ b/fs/xfs/xfs_ondisk.h
@@ -104,7 +104,7 @@  xfs_check_ondisk_structs(void)
 	XFS_CHECK_STRUCT_SIZE(xfs_dir2_leaf_hdr_t,		16);
 	XFS_CHECK_STRUCT_SIZE(xfs_dir2_leaf_t,			16);
 	XFS_CHECK_STRUCT_SIZE(xfs_dir2_leaf_tail_t,		4);
-	XFS_CHECK_STRUCT_SIZE(xfs_dir2_sf_entry_t,		3);
+	XFS_CHECK_STRUCT_SIZE(xfs_dir2_sf_entry_t,		4);
 	XFS_CHECK_OFFSET(xfs_dir2_sf_entry_t, namelen,		0);
 	XFS_CHECK_OFFSET(xfs_dir2_sf_entry_t, offset,		1);
 	XFS_CHECK_OFFSET(xfs_dir2_sf_entry_t, name,		3);