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