Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/trunk]: src/sys/ufs/lfs Avoid misaligned access to lfs64 on-disk records...



details:   https://anonhg.NetBSD.org/src/rev/1dc49f9f090a
branches:  trunk
changeset: 1008374:1dc49f9f090a
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Sat Mar 21 06:11:05 2020 +0000

description:
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.

diffstat:

 sys/ufs/lfs/lfs.h           |  38 +++++++++++++++++++++++++++-----------
 sys/ufs/lfs/lfs_accessors.h |  25 +++----------------------
 2 files changed, 30 insertions(+), 33 deletions(-)

diffs (198 lines):

diff -r 0e4b79ed7770 -r 1dc49f9f090a sys/ufs/lfs/lfs.h
--- a/sys/ufs/lfs/lfs.h Sat Mar 21 06:09:33 2020 +0000
+++ b/sys/ufs/lfs/lfs.h Sat Mar 21 06:11:05 2020 +0000
@@ -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 @@
 __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 @@
        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 @@
        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 @@
        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 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 @@
        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 @@
        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 @@
        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 @@
        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 @@
        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 @@
        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 @@
        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 @@
        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 @@
        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 -r 0e4b79ed7770 -r 1dc49f9f090a sys/ufs/lfs/lfs_accessors.h
--- a/sys/ufs/lfs/lfs_accessors.h       Sat Mar 21 06:09:33 2020 +0000
+++ b/sys/ufs/lfs/lfs_accessors.h       Sat Mar 21 06:11:05 2020 +0000
@@ -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 @@
 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 @@
 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);
        }



Home | Main Index | Thread Index | Old Index