Re: [PATCH] xfs: Fix xfs_dir2_sf_entry_t size check

[Darrick J. Wong]

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@xxxxxxxxxx>
> >> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@xxxxxxx>
> >> ---
> >>  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@xxxxxxx>
> sub   RSA 2048/4205BF15 2019-09-02
> sub   RSA 2048/296522AA 2019-09-02
> sub   RSA 2048/7CAB726B 2019-09-02
>