Re: [PATCH] xfs: port xfs/122 to the kernel

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Oct 15, 2024 at 08:09:53PM +1100, Dave Chinner wrote:
> On Fri, Oct 11, 2024 at 11:24:07AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Check this with every kernel and userspace build, so we can drop the
> > nonsense in xfs/122.  Roughly drafted with:
> > 
> > sed -e 's/^offsetof/\tXFS_CHECK_OFFSET/g' \
> > 	-e 's/^sizeof/\tXFS_CHECK_STRUCT_SIZE/g' \
> > 	-e 's/ = \([0-9]*\)/,\t\t\t\1);/g' \
> > 	-e 's/xfs_sb_t/struct xfs_dsb/g' \
> > 	-e 's/),/,/g' \
> > 	-e 's/xfs_\([a-z0-9_]*\)_t,/struct xfs_\1,/g' \
> > 	< tests/xfs/122.out | sort
> > 
> > and then manual fixups.
> 
> [snip on disk structures]
> 
> I don't think we can type check all these ioctl structures,
> especially the old ones.
> 
> i.e. The old ioctl structures are not padded to 64 bit boundaries,
> nor are they constructed without internal padding holes, and this is
> why compat ioctls exist. Hence any ioctl structure that has a compat
> definition in xfs_ioctl32.h can't be size checked like this....
> 
> > +	XFS_CHECK_STRUCT_SIZE(struct xfs_error_injection,		8);
> > +	XFS_CHECK_STRUCT_SIZE(struct xfs_exchange_range,		40);
> > +	XFS_CHECK_STRUCT_SIZE(xfs_exntst_t,				4);
> > +	XFS_CHECK_STRUCT_SIZE(struct xfs_fid,				16);
> > +	XFS_CHECK_STRUCT_SIZE(struct xfs_fs_eofblocks,			128);
> > +	XFS_CHECK_STRUCT_SIZE(struct xfs_fsid,				8);
> > +	XFS_CHECK_STRUCT_SIZE(struct xfs_fsop_counts,			32);
> > +	XFS_CHECK_STRUCT_SIZE(struct xfs_fsop_geom,			256);
> > +	XFS_CHECK_STRUCT_SIZE(struct xfs_fsop_geom_v1,			112);
> 
> e.g. xfs_fsop_geom_v1 is 108 bytes on 32 bit systems, not 112:
> 
> struct compat_xfs_fsop_geom_v1 {
>         __u32                      blocksize;            /*     0     4 */
>         __u32                      rtextsize;            /*     4     4 */
>         __u32                      agblocks;             /*     8     4 */
>         __u32                      agcount;              /*    12     4 */
>         __u32                      logblocks;            /*    16     4 */
>         __u32                      sectsize;             /*    20     4 */
>         __u32                      inodesize;            /*    24     4 */
>         __u32                      imaxpct;              /*    28     4 */
>         __u64                      datablocks;           /*    32     8 */
>         __u64                      rtblocks;             /*    40     8 */
>         __u64                      rtextents;            /*    48     8 */
>         __u64                      logstart;             /*    56     8 */
>         /* --- cacheline 1 boundary (64 bytes) --- */
>         unsigned char              uuid[16];             /*    64    16 */
>         __u32                      sunit;                /*    80     4 */
>         __u32                      swidth;               /*    84     4 */
>         __s32                      version;              /*    88     4 */
>         __u32                      flags;                /*    92     4 */
>         __u32                      logsectsize;          /*    96     4 */
>         __u32                      rtsectsize;           /*   100     4 */
>         __u32                      dirblocksize;         /*   104     4 */
> 
>         /* size: 108, cachelines: 2, members: 20 */
>         /* last cacheline: 44 bytes */
> } __attribute__((__packed__));
> 
> I'm not sure we need to size check these structures - if they change
> size, the ioctl number will change and that means all the userspace
> test code built against the system /usr/include/xfs/xfs_fs.h file
> that exercises the ioctls will stop working, right? i.e. breakage
> should be pretty obvious...

It should, though I worry about the case where we accidentally change
the size on some weird architecture, some distro ships a new release
with everything built against the broken headers, and only later does
someone notice that their old program now starts failing.

I guess the question is, do we hardcode the known sizes here, e.g.

	XFS_CHECK_IOCTL_SIZE(struct xfs_fsop_geom_v1, 108, 112);

wherein we'd assert that sizeof() == 108 || sizeof() == 112?

Or not care if things happen to the ioctls?  Part of aim of this posting
was to trick the build bots into revealing which architectures break on
the compat ioctl stuff... ;)

--D

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>