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

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

On Wed, Oct 16, 2024 at 09:01:14AM +1100, Dave Chinner wrote:
> 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.

My goal was to get rid of xfs/122 but as this is turning into a messy
cleanup I'm going to drop the whole thing on the floor.  I don't think
anyone runs xfs/122 anyway so I'll just put it in my expunge list;
someone else who isn't as burned out as I am can pick it up if they
choose.

--D

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