Module Name: src
Committed By: riastradh
Date: Sat Mar 21 06:11:05 UTC 2020
Modified Files:
src/sys/ufs/lfs: lfs.h lfs_accessors.h
Log Message:
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.
To generate a diff of this commit:
cvs rdiff -u -r1.206 -r1.207 src/sys/ufs/lfs/lfs.h
cvs rdiff -u -r1.48 -r1.49 src/sys/ufs/lfs/lfs_accessors.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/ufs/lfs/lfs.h
diff -u src/sys/ufs/lfs/lfs.h:1.206 src/sys/ufs/lfs/lfs.h:1.207
--- src/sys/ufs/lfs/lfs.h:1.206 Sat Mar 21 06:09:33 2020
+++ src/sys/ufs/lfs/lfs.h Sat Mar 21 06:11:05 2020
@@ -1,4 +1,4 @@
-/* $NetBSD: lfs.h,v 1.206 2020/03/21 06:09:33 riastradh Exp $ */
+/* $NetBSD: lfs.h,v 1.207 2020/03/21 06:11:05 riastradh Exp $ */
/* from NetBSD: dinode.h,v 1.25 2016/01/22 23:06:10 dholland Exp */
/* from NetBSD: dir.h,v 1.25 2015/09/01 06:16:03 dholland Exp */
@@ -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;
Index: src/sys/ufs/lfs/lfs_accessors.h
diff -u src/sys/ufs/lfs/lfs_accessors.h:1.48 src/sys/ufs/lfs/lfs_accessors.h:1.49
--- src/sys/ufs/lfs/lfs_accessors.h:1.48 Sat Jun 10 05:29:36 2017
+++ src/sys/ufs/lfs/lfs_accessors.h Sat Mar 21 06:11:05 2020
@@ -1,4 +1,4 @@
-/* $NetBSD: lfs_accessors.h,v 1.48 2017/06/10 05:29:36 maya Exp $ */
+/* $NetBSD: lfs_accessors.h,v 1.49 2020/03/21 06:11:05 riastradh Exp $ */
/* from NetBSD: lfs.h,v 1.165 2015/07/24 06:59:32 dholland Exp */
/* from NetBSD: dinode.h,v 1.25 2016/01/22 23:06:10 dholland Exp */
@@ -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);
}
