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.
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
