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@xxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxx
>
