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

[Dave Chinner <david@fromorbit.com>]

On Tue, Oct 15, 2024 at 09:59:21AM -0700, Darrick J. Wong wrote:
> 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?

This feels kinda fragile. We want the compiler to do the validation
work for both the normal and compat ioctl structures. Just
specifying two sizes doesn't actually do that.

i.e. this really needs to check that the structure size is the same
on all 64 bit platforms, the compat structure is the same on -all-
platforms (32 and 64 bit), and that the the structure size is the
same as the compat structure size on all 32 bit platforms....

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

If that's your goal, then this needs to be validating the compat
structures are the correct size, too.

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