From 874b904a48186ee9b5735ff99c7bf38f5ad6cbd3 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 17 Mar 2020 03:37:57 +0000 Subject: [PATCH] Avoid misaligned access to lfs64 on-disk records in memory. lfs64 directory entries are only 32-bit aligned in order to conserve space in directory blocks, and we had a hack to stuff a 64-bit inode in them. This replaces the hack by __aligned(4) __packed, and goes further: 1. It's not clear that all the other lfs64 data structures are 64-bit aligned on disk to begin with. We can go through these later and upgrade them from struct foo64 { ... } __aligned(4) __packed; union foo { struct foo64 f64; ... }; to struct foo64 { ... }; union foo { struct foo64 f64 __aligned(8); ... } __aligned(4) __packed; if we really want to take advantage of 64-bit memory accesses. However, the __aligned(4) __packed must remain on the union because: 2. We access even the lfs32 data structures via a union that has lfs64 members, and it turns out that compilers will assume access through a union with 64-bit aligned members implies the whole union has 64-bit alignment, even if we're only accessing a 32-bit aligned member. --- sys/ufs/lfs/lfs.h | 36 ++++++++++++++++++++++++++---------- sys/ufs/lfs/lfs_accessors.h | 23 ++--------------------- 2 files changed, 28 insertions(+), 31 deletions(-) diff --git a/sys/ufs/lfs/lfs.h b/sys/ufs/lfs/lfs.h index 932ded1e1040..b5819761708a 100644 --- a/sys/ufs/lfs/lfs.h +++ b/sys/ufs/lfs/lfs.h @@ -358,18 +358,19 @@ struct lfs_dirheader32 { __CTASSERT(sizeof(struct lfs_dirheader32) == 8); struct lfs_dirheader64 { - uint32_t dh_inoA; /* inode number of entry */ - uint32_t dh_inoB; /* inode number of entry */ + uint64_t dh_ino; /* inode number of entry */ uint16_t dh_reclen; /* length of this record */ uint8_t dh_type; /* file type, see below */ uint8_t dh_namlen; /* length of string in d_name */ -}; +} __aligned(4) __packed; __CTASSERT(sizeof(struct lfs_dirheader64) == 12); union lfs_dirheader { struct lfs_dirheader64 u_64; struct lfs_dirheader32 u_32; }; +__CTASSERT(__alignof(union lfs_dirheader) == __alignof(struct lfs_dirheader64)); +__CTASSERT(__alignof(union lfs_dirheader) == __alignof(struct lfs_dirheader32)); typedef union lfs_dirheader LFS_DIRHEADER; @@ -481,6 +482,8 @@ union lfs_dinode { struct lfs64_dinode u_64; struct lfs32_dinode u_32; }; +__CTASSERT(__alignof(union lfs_dinode) == __alignof(struct lfs64_dinode)); +__CTASSERT(__alignof(union lfs_dinode) == __alignof(struct lfs32_dinode)); /* * The di_db fields may be overlaid with other information for @@ -563,7 +566,7 @@ struct finfo64 { uint64_t fi_ino; /* inode number */ uint32_t fi_lastlength; /* length of last block in array */ uint32_t fi_pad; /* unused */ -}; +} __aligned(4) __packed; __CTASSERT(sizeof(struct finfo64) == 24); typedef struct finfo32 FINFO32; @@ -579,6 +582,8 @@ typedef union finfo { struct finfo64 u_64; struct finfo32 u_32; } FINFO; +__CTASSERT(__alignof(union finfo) == __alignof(struct finfo64)); +__CTASSERT(__alignof(union finfo) == __alignof(struct finfo32)); /* * inode info (part of the segment summary) @@ -590,7 +595,7 @@ typedef union finfo { typedef struct iinfo64 { uint64_t ii_block; /* block number */ -} IINFO64; +} __aligned(4) __packed IINFO64; __CTASSERT(sizeof(struct iinfo64) == 8); typedef struct iinfo32 { @@ -602,6 +607,8 @@ typedef union iinfo { struct iinfo64 u_64; struct iinfo32 u_32; } IINFO; +__CTASSERT(__alignof(union iinfo) == __alignof(struct iinfo64)); +__CTASSERT(__alignof(union iinfo) == __alignof(struct iinfo32)); /* * Index file inode entries. @@ -620,7 +627,7 @@ struct ifile64 { uint64_t if_atime_sec; /* Last access time, seconds */ int64_t if_daddr; /* inode disk address */ uint64_t if_nextfree; /* next-unallocated inode */ -}; +} __aligned(4) __packed; __CTASSERT(sizeof(struct ifile64) == 32); typedef struct ifile32 IFILE32; @@ -655,6 +662,9 @@ typedef union ifile { struct ifile32 u_32; struct ifile_v1 u_v1; } IFILE; +__CTASSERT(__alignof(union ifile) == __alignof(struct ifile64)); +__CTASSERT(__alignof(union ifile) == __alignof(struct ifile32)); +__CTASSERT(__alignof(union ifile) == __alignof(struct ifile_v1)); /* * Cleaner information structure. This resides in the ifile and is used @@ -684,7 +694,7 @@ typedef struct _cleanerinfo64 { uint64_t free_tail; /* 32: tail of the inode free list */ uint32_t flags; /* 40: status word from the kernel */ uint32_t pad; /* 44: must be 64-bit aligned */ -} CLEANERINFO64; +} __aligned(4) __packed CLEANERINFO64; __CTASSERT(sizeof(struct _cleanerinfo64) == 48); /* this must not go to disk directly of course */ @@ -692,6 +702,8 @@ typedef union _cleanerinfo { CLEANERINFO32 u_32; CLEANERINFO64 u_64; } CLEANERINFO; +__CTASSERT(__alignof(union _cleanerinfo) == __alignof(struct _cleanerinfo32)); +__CTASSERT(__alignof(union _cleanerinfo) == __alignof(struct _cleanerinfo64)); /* * On-disk segment summary information @@ -740,7 +752,7 @@ struct segsum32 { uint64_t ss_serial; /* 32: serial number */ uint64_t ss_create; /* 40: time stamp */ /* FINFO's and inode daddr's... */ -}; +} __aligned(4) __packed; __CTASSERT(sizeof(struct segsum32) == 48); typedef struct segsum64 SEGSUM64; @@ -758,7 +770,7 @@ struct segsum64 { uint64_t ss_serial; /* 40: serial number */ uint64_t ss_create; /* 48: time stamp */ /* FINFO's and inode daddr's... */ -}; +} __aligned(4) __packed; __CTASSERT(sizeof(struct segsum64) == 56); typedef union segsum SEGSUM; @@ -767,7 +779,9 @@ union segsum { struct segsum32 u_32; struct segsum_v1 u_v1; }; - +__CTASSERT(__alignof(union segsum) == __alignof(struct segsum64)); +__CTASSERT(__alignof(union segsum) == __alignof(struct segsum32)); +__CTASSERT(__alignof(union segsum) == __alignof(struct segsum_v1)); /* * On-disk super block. @@ -956,6 +970,8 @@ struct dlfs64 { uint32_t dlfs_cksum; /* 508: checksum for superblock checking */ }; +__CTASSERT(__alignof(struct dlfs) == __alignof(struct dlfs64)); + /* Type used for the inode bitmap */ typedef uint32_t lfs_bm_t; diff --git a/sys/ufs/lfs/lfs_accessors.h b/sys/ufs/lfs/lfs_accessors.h index b4accd6c358a..f058ace2359d 100644 --- a/sys/ufs/lfs/lfs_accessors.h +++ b/sys/ufs/lfs/lfs_accessors.h @@ -274,17 +274,7 @@ static __inline uint64_t lfs_dir_getino(const STRUCT_LFS *fs, const LFS_DIRHEADER *dh) { if (fs->lfs_is64) { - uint64_t ino; - - /* - * XXX we can probably write this in a way that's both - * still legal and generates better code. - */ - memcpy(&ino, &dh->u_64.dh_inoA, sizeof(dh->u_64.dh_inoA)); - memcpy((char *)&ino + sizeof(dh->u_64.dh_inoA), - &dh->u_64.dh_inoB, - sizeof(dh->u_64.dh_inoB)); - return LFS_SWAP_uint64_t(fs, ino); + return LFS_SWAP_uint64_t(fs, dh->u_64.dh_ino); } else { return LFS_SWAP_uint32_t(fs, dh->u_32.dh_ino); } @@ -331,16 +321,7 @@ static __inline void lfs_dir_setino(STRUCT_LFS *fs, LFS_DIRHEADER *dh, uint64_t ino) { if (fs->lfs_is64) { - - ino = LFS_SWAP_uint64_t(fs, ino); - /* - * XXX we can probably write this in a way that's both - * still legal and generates better code. - */ - memcpy(&dh->u_64.dh_inoA, &ino, sizeof(dh->u_64.dh_inoA)); - memcpy(&dh->u_64.dh_inoB, - (char *)&ino + sizeof(dh->u_64.dh_inoA), - sizeof(dh->u_64.dh_inoB)); + dh->u_64.dh_ino = LFS_SWAP_uint64_t(fs, ino); } else { dh->u_32.dh_ino = LFS_SWAP_uint32_t(fs, ino); }