From 799788bfc71b8382297fd603508ab1cc24bee74f Mon Sep 17 00:00:00 2001 From: Andrew Price Date: Apr 05 2023 10:32:00 +0000 Subject: [PATCH 1/2] libgfs2: Separate gfs and gfs2 code in lgfs2_sb_out() Accessing the buffer via the two pointer types can lead to bad optimizations due to undefined behaviour in some cases when link-time optimization is enabled. Separate the gfs_sb and gfs2_sb cases. Signed-off-by: Andrew Price --- diff --git a/gfs2/libgfs2/check_ondisk.c b/gfs2/libgfs2/check_ondisk.c index 940a6b0..b6d57b7 100644 --- a/gfs2/libgfs2/check_ondisk.c +++ b/gfs2/libgfs2/check_ondisk.c @@ -54,6 +54,7 @@ START_TEST(check_sb1_out) memset(buf, 0, sizeof(buf)); memset(&sbd, 0, sizeof(sbd)); + sbd.gfs1 = 1; sbd.sd_fs_format = 0x5a5a5a51; sbd.sd_multihost_format = 0x5a5a5a52; sbd.sd_flags = 0x5a5a5a53; diff --git a/gfs2/libgfs2/ondisk.c b/gfs2/libgfs2/ondisk.c index b6e6a86..2084236 100644 --- a/gfs2/libgfs2/ondisk.c +++ b/gfs2/libgfs2/ondisk.c @@ -48,26 +48,41 @@ void lgfs2_sb_in(struct lgfs2_sbd *sdp, void *buf) void lgfs2_sb_out(const struct lgfs2_sbd *sdp, void *buf) { - struct gfs2_sb *sb = buf; - struct gfs_sb *sb1 = buf; + if (sdp->gfs1) { + struct gfs_sb *sb = buf; + + sb->sb_header.mh_magic = cpu_to_be32(GFS2_MAGIC); + sb->sb_header.mh_type = cpu_to_be32(GFS2_METATYPE_SB); + sb->sb_header.mh_format = cpu_to_be32(GFS2_FORMAT_SB); + sb->sb_fs_format = cpu_to_be32(sdp->sd_fs_format); + sb->sb_multihost_format = cpu_to_be32(sdp->sd_multihost_format); + sb->sb_flags = cpu_to_be32(sdp->sd_flags); + sb->sb_bsize = cpu_to_be32(sdp->sd_bsize); + sb->sb_bsize_shift = cpu_to_be32(sdp->sd_bsize_shift); + sb->sb_seg_size = cpu_to_be32(sdp->sd_seg_size); + lgfs2_inum_out(&sdp->sd_jindex_di, &sb->sb_jindex_di); + lgfs2_inum_out(&sdp->sd_rindex_di, &sb->sb_rindex_di); + lgfs2_inum_out(&sdp->sd_root_dir, &sb->sb_root_di); + memcpy(sb->sb_lockproto, sdp->sd_lockproto, GFS2_LOCKNAME_LEN); + memcpy(sb->sb_locktable, sdp->sd_locktable, GFS2_LOCKNAME_LEN); + lgfs2_inum_out(&sdp->sd_quota_di, &sb->sb_quota_di); + lgfs2_inum_out(&sdp->sd_license_di, &sb->sb_license_di); + } else { + struct gfs2_sb *sb = buf; - sb->sb_header.mh_magic = cpu_to_be32(GFS2_MAGIC); - sb->sb_header.mh_type = cpu_to_be32(GFS2_METATYPE_SB); - sb->sb_header.mh_format = cpu_to_be32(GFS2_FORMAT_SB); - sb->sb_fs_format = cpu_to_be32(sdp->sd_fs_format); - sb->sb_multihost_format = cpu_to_be32(sdp->sd_multihost_format); - sb1->sb_flags = cpu_to_be32(sdp->sd_flags); - sb->sb_bsize = cpu_to_be32(sdp->sd_bsize); - sb->sb_bsize_shift = cpu_to_be32(sdp->sd_bsize_shift); - sb1->sb_seg_size = cpu_to_be32(sdp->sd_seg_size); - lgfs2_inum_out(&sdp->sd_meta_dir, &sb->sb_master_dir); - lgfs2_inum_out(&sdp->sd_root_dir, &sb->sb_root_dir); - memcpy(sb->sb_lockproto, sdp->sd_lockproto, GFS2_LOCKNAME_LEN); - memcpy(sb->sb_locktable, sdp->sd_locktable, GFS2_LOCKNAME_LEN); - lgfs2_inum_out(&sdp->sd_rindex_di, &sb1->sb_rindex_di); - lgfs2_inum_out(&sdp->sd_quota_di, &sb1->sb_quota_di); - lgfs2_inum_out(&sdp->sd_license_di, &sb1->sb_license_di); - memcpy(sb->sb_uuid, sdp->sd_uuid, 16); + sb->sb_header.mh_magic = cpu_to_be32(GFS2_MAGIC); + sb->sb_header.mh_type = cpu_to_be32(GFS2_METATYPE_SB); + sb->sb_header.mh_format = cpu_to_be32(GFS2_FORMAT_SB); + sb->sb_fs_format = cpu_to_be32(sdp->sd_fs_format); + sb->sb_multihost_format = cpu_to_be32(sdp->sd_multihost_format); + sb->sb_bsize = cpu_to_be32(sdp->sd_bsize); + sb->sb_bsize_shift = cpu_to_be32(sdp->sd_bsize_shift); + lgfs2_inum_out(&sdp->sd_meta_dir, &sb->sb_master_dir); + lgfs2_inum_out(&sdp->sd_root_dir, &sb->sb_root_dir); + memcpy(sb->sb_lockproto, sdp->sd_lockproto, GFS2_LOCKNAME_LEN); + memcpy(sb->sb_locktable, sdp->sd_locktable, GFS2_LOCKNAME_LEN); + memcpy(sb->sb_uuid, sdp->sd_uuid, 16); + } } void lgfs2_rindex_in(lgfs2_rgrp_t rg, void *buf) From a1a5d4ca83012e3d74834223f61956bfae0f2fdb Mon Sep 17 00:00:00 2001 From: Andrew Price Date: Apr 06 2023 11:55:08 +0000 Subject: [PATCH 2/2] Don't use char arrays as temporary buffers Using 'char foo[sizeof(struct bar)]' in these cases instead of the struct type can lead to alignment issues and undefined behaviour. Using the struct type directly also makes for tidier code. Signed-off-by: Andrew Price --- diff --git a/gfs2/convert/gfs2_convert.c b/gfs2/convert/gfs2_convert.c index 0b5e263..e76372a 100644 --- a/gfs2/convert/gfs2_convert.c +++ b/gfs2/convert/gfs2_convert.c @@ -1436,7 +1436,6 @@ static int fix_cdpn_symlinks(struct lgfs2_sbd *sbp, osi_list_t *cdpn_to_fix) static int read_gfs1_jiindex(struct lgfs2_sbd *sdp) { struct lgfs2_inode *ip = sdp->md.jiinode; - char buf[sizeof(struct gfs_jindex)]; unsigned int j; int error=0; unsigned int tmp_mode = 0; @@ -1464,9 +1463,10 @@ static int read_gfs1_jiindex(struct lgfs2_sbd *sdp) ip->i_mode &= ~S_IFMT; ip->i_mode |= S_IFDIR; for (j = 0; ; j++) { + struct gfs_jindex ji; uint32_t nseg; - error = lgfs2_readi(ip, buf, j * sizeof(struct gfs_jindex), + error = lgfs2_readi(ip, &ji, j * sizeof(struct gfs_jindex), sizeof(struct gfs_jindex)); if(!error) break; @@ -1475,7 +1475,7 @@ static int read_gfs1_jiindex(struct lgfs2_sbd *sdp) " journal index file.\n")); goto fail; } - memcpy(sd_jindex + j, buf, sizeof(struct gfs_jindex)); + memcpy(sd_jindex + j, &ji, sizeof(struct gfs_jindex)); nseg = be32_to_cpu(sd_jindex[j].ji_nsegment); sdp->jsize = (nseg * 16 * sdp->sd_bsize) >> 20; } diff --git a/gfs2/edit/extended.c b/gfs2/edit/extended.c index aec8cbd..875ea74 100644 --- a/gfs2/edit/extended.c +++ b/gfs2/edit/extended.c @@ -424,8 +424,6 @@ static void print_block_details(struct iinfo *ind, int level, int cur_height, static int print_gfs_jindex(struct lgfs2_inode *dij) { int error, start_line; - struct gfs_jindex *ji; - char jbuf[sizeof(struct gfs_jindex)]; start_line = line; print_gfs2("Journal index entries found: %"PRIu64".", @@ -433,10 +431,11 @@ static int print_gfs_jindex(struct lgfs2_inode *dij) eol(0); lines_per_row[dmode] = 4; for (print_entry_ndx=0; ; print_entry_ndx++) { - error = lgfs2_readi(dij, (void *)&jbuf, + struct gfs_jindex ji; + + error = lgfs2_readi(dij, &ji, print_entry_ndx*sizeof(struct gfs_jindex), sizeof(struct gfs_jindex)); - ji = (struct gfs_jindex *)jbuf; if (!error) /* end of file */ break; if (!termlines || @@ -446,13 +445,13 @@ static int print_gfs_jindex(struct lgfs2_inode *dij) if (edit_row[dmode] == print_entry_ndx) { COLORS_HIGHLIGHT; strcpy(efield, "ji_addr"); - sprintf(estring, "%"PRIx64, be64_to_cpu(ji->ji_addr)); + sprintf(estring, "%"PRIx64, be64_to_cpu(ji.ji_addr)); } print_gfs2("Journal #%d", print_entry_ndx); eol(0); if (edit_row[dmode] == print_entry_ndx) COLORS_NORMAL; - gfs_jindex_print(ji); + gfs_jindex_print(&ji); last_entry_onscreen[dmode] = print_entry_ndx; } } diff --git a/gfs2/edit/savemeta.c b/gfs2/edit/savemeta.c index 5c864d2..4ee76be 100644 --- a/gfs2/edit/savemeta.c +++ b/gfs2/edit/savemeta.c @@ -950,23 +950,21 @@ static void get_journal_inode_blocks(void) struct lgfs2_inode *j_inode = NULL; if (sbd.gfs1) { - struct gfs_jindex *ji; - char jbuf[sizeof(struct gfs_jindex)]; + struct gfs_jindex ji; j_inode = lgfs2_gfs_inode_read(&sbd, sbd.sd_jindex_di.in_addr); if (j_inode == NULL) { fprintf(stderr, "Error reading journal inode: %s\n", strerror(errno)); return; } - amt = lgfs2_readi(j_inode, (void *)&jbuf, + amt = lgfs2_readi(j_inode, &ji, journal * sizeof(struct gfs_jindex), sizeof(struct gfs_jindex)); lgfs2_inode_put(&j_inode); if (!amt) break; - ji = (struct gfs_jindex *)jbuf; - jblock = be64_to_cpu(ji->ji_addr); - gfs1_journal_size = (uint64_t)be32_to_cpu(ji->ji_nsegment) * 16; + jblock = be64_to_cpu(ji.ji_addr); + gfs1_journal_size = (uint64_t)be32_to_cpu(ji.ji_nsegment) * 16; } else { if (journal + 3 > indirect->ii[0].dirents) break; diff --git a/gfs2/fsck/fs_recovery.c b/gfs2/fsck/fs_recovery.c index 3c22099..dc978d5 100644 --- a/gfs2/fsck/fs_recovery.c +++ b/gfs2/fsck/fs_recovery.c @@ -751,7 +751,6 @@ int ji_update(struct lgfs2_sbd *sdp) struct lgfs2_inode *jip, *ip = sdp->md.jiinode; char journal_name[JOURNAL_NAME_SIZE]; int i, error; - char buf[sizeof(struct gfs_jindex)]; if (!ip) { log_crit(_("Journal index inode not found.\n")); @@ -777,11 +776,10 @@ int ji_update(struct lgfs2_sbd *sdp) memset(journal_name, 0, sizeof(*journal_name)); for (i = 0; i < sdp->md.journals; i++) { if (sdp->gfs1) { - struct gfs_jindex *ji; + struct gfs_jindex ji; - error = lgfs2_readi(ip, - buf, i * sizeof(struct gfs_jindex), - sizeof(struct gfs_jindex)); + error = lgfs2_readi(ip, &ji, i * sizeof(struct gfs_jindex), + sizeof(struct gfs_jindex)); if (!error) break; if (error != sizeof(struct gfs_jindex)){ @@ -789,8 +787,7 @@ int ji_update(struct lgfs2_sbd *sdp) " journal index file.\n")); return -1; } - ji = (struct gfs_jindex *)buf; - sdp->md.journal[i] = lgfs2_inode_read(sdp, be64_to_cpu(ji->ji_addr)); + sdp->md.journal[i] = lgfs2_inode_read(sdp, be64_to_cpu(ji.ji_addr)); if (sdp->md.journal[i] == NULL) return -1; } else { diff --git a/gfs2/fsck/initialize.c b/gfs2/fsck/initialize.c index 4cb8cb5..62fb610 100644 --- a/gfs2/fsck/initialize.c +++ b/gfs2/fsck/initialize.c @@ -1451,19 +1451,16 @@ static int reset_journal_seg_size(struct fsck_cx *cx, unsigned int jsize, unsign static int correct_journal_seg_size(struct fsck_cx *cx) { int count; - struct gfs_jindex *ji_0, *ji_1; + struct gfs_jindex ji_0, ji_1; struct lgfs2_sbd *sdp = cx->sdp; - char buf[sizeof(struct gfs_jindex)]; unsigned int jsize = LGFS2_DEFAULT_JSIZE * 1024 * 1024; - count = lgfs2_readi(sdp->md.jiinode, buf, 0, sizeof(struct gfs_jindex)); + count = lgfs2_readi(sdp->md.jiinode, &ji_0, 0, sizeof(struct gfs_jindex)); if (count != sizeof(struct gfs_jindex)) { log_crit(_("Error %d reading system journal index inode. " "Aborting\n"), count); return -1; } - ji_0 = (struct gfs_jindex *)buf; - if (sdp->md.journals == 1) { if (sdp->sd_seg_size == 0) { if (!query(cx, _("The gfs2 journal segment size is 0 and a" @@ -1481,19 +1478,16 @@ static int correct_journal_seg_size(struct fsck_cx *cx) */ return 0; } - - count = lgfs2_readi(sdp->md.jiinode, buf, sizeof(struct gfs_jindex), + count = lgfs2_readi(sdp->md.jiinode, &ji_1, sizeof(struct gfs_jindex), sizeof(struct gfs_jindex)); if (count != sizeof(struct gfs_jindex)) { log_crit(_("Error %d reading system journal index inode. " "Aborting\n"), count); return -1; } - ji_1 = (struct gfs_jindex *)buf; - - jsize = (be64_to_cpu(ji_1->ji_addr) - be64_to_cpu(ji_0->ji_addr)) * sdp->sd_bsize; + jsize = (be64_to_cpu(ji_1.ji_addr) - be64_to_cpu(ji_0.ji_addr)) * sdp->sd_bsize; out: - return reset_journal_seg_size(cx, jsize, be32_to_cpu(ji_0->ji_nsegment)); + return reset_journal_seg_size(cx, jsize, be32_to_cpu(ji_0.ji_nsegment)); } /* @@ -1504,10 +1498,8 @@ out: */ static int reconstruct_journals(struct fsck_cx *cx) { - int i, count; - struct gfs_jindex *ji; struct lgfs2_sbd *sdp = cx->sdp; - char buf[sizeof(struct gfs_jindex)]; + int i, count; /* Ensure that sb_seg_size is valid */ if (correct_journal_seg_size(cx)) { @@ -1517,15 +1509,16 @@ static int reconstruct_journals(struct fsck_cx *cx) log_err(_("Clearing GFS journals (this may take a while)\n")); for (i = 0; i < sdp->md.journals; i++) { - count = lgfs2_readi(sdp->md.jiinode, buf, + struct gfs_jindex ji; + + count = lgfs2_readi(sdp->md.jiinode, &ji, i * sizeof(struct gfs_jindex), sizeof(struct gfs_jindex)); if (count != sizeof(struct gfs_jindex)) return 0; - ji = (struct gfs_jindex *)buf; if ((i % 2) == 0) log_err("."); - if (reconstruct_single_journal(sdp, i, be32_to_cpu(ji->ji_nsegment))) + if (reconstruct_single_journal(sdp, i, be32_to_cpu(ji.ji_nsegment))) return -1; } log_err(_("\nJournals cleared.\n")); diff --git a/gfs2/libgfs2/check_ondisk.c b/gfs2/libgfs2/check_ondisk.c index b6d57b7..bd439ef 100644 --- a/gfs2/libgfs2/check_ondisk.c +++ b/gfs2/libgfs2/check_ondisk.c @@ -5,12 +5,13 @@ Suite *suite_ondisk(void); START_TEST(check_sb_in) { - char buf[sizeof(struct gfs2_sb)]; char namechk[GFS2_LOCKNAME_LEN]; struct lgfs2_sbd sbd; char uuidchk[sizeof(sbd.sd_uuid)]; + struct gfs2_sb buf_sb; + void *buf = &buf_sb; - memset(buf, 0x5a, sizeof(buf)); + memset(&buf_sb, 0x5a, sizeof(buf_sb)); memset(namechk, 0x5a, GFS2_LOCKNAME_LEN); memset(uuidchk, 0x5a, sizeof(sbd.sd_uuid)); memset(&sbd, 0, sizeof(sbd)); @@ -44,14 +45,15 @@ END_TEST START_TEST(check_sb1_out) { char namechk[GFS2_LOCKNAME_LEN]; - char buf[sizeof(struct gfs_sb)]; + struct gfs_sb buf_sb; + void *buf = &buf_sb; struct lgfs2_sbd sbd; struct gfs_sb *sb; memset(namechk, 0x5a, GFS2_LOCKNAME_LEN); /* 1. If only the gfs1 fields are set, the sb must be filled */ - memset(buf, 0, sizeof(buf)); + memset(&buf_sb, 0, sizeof(buf_sb)); memset(&sbd, 0, sizeof(sbd)); sbd.gfs1 = 1; @@ -100,17 +102,18 @@ END_TEST START_TEST(check_sb2_out) { - char buf[sizeof(struct gfs2_sb)]; char namechk[GFS2_LOCKNAME_LEN]; struct lgfs2_sbd sbd; struct gfs2_sb *sb; char uuidchk[sizeof(sbd.sd_uuid)]; + struct gfs2_sb buf_sb; + void *buf = &buf_sb; memset(namechk, 0x5a, GFS2_LOCKNAME_LEN); memset(uuidchk, 0x5a, sizeof(sbd.sd_uuid)); /* 2. If only the gfs2 fields are set, the sb must be filled */ - memset(buf, 0, sizeof(buf)); + memset(&buf_sb, 0, sizeof(buf_sb)); memset(&sbd, 0, sizeof(sbd)); sbd.sd_fs_format = 0x5a5a5a50; diff --git a/gfs2/libgfs2/structures.c b/gfs2/libgfs2/structures.c index 2835dda..2da4680 100644 --- a/gfs2/libgfs2/structures.c +++ b/gfs2/libgfs2/structures.c @@ -383,7 +383,7 @@ struct lgfs2_inode *lgfs2_build_rindex(struct lgfs2_sbd *sdp) struct lgfs2_inode *ip; struct osi_node *n, *next = NULL; struct lgfs2_rgrp_tree *rl; - char buf[sizeof(struct gfs2_rindex)]; + struct gfs2_rindex ri; int count; ip = lgfs2_createi(sdp->master_dir, "rindex", S_IFREG | 0600, @@ -394,20 +394,21 @@ struct lgfs2_inode *lgfs2_build_rindex(struct lgfs2_sbd *sdp) ip->i_payload_format = GFS2_FORMAT_RI; lgfs2_bmodified(ip->i_bh); + memset(&ri, 0, sizeof(struct gfs2_rindex)); for (n = osi_first(&sdp->rgtree); n; n = next) { next = osi_next(n); rl = (struct lgfs2_rgrp_tree *)n; - lgfs2_rindex_out(rl, buf); + lgfs2_rindex_out(rl, &ri); - count = lgfs2_writei(ip, buf, ip->i_size, sizeof(struct gfs2_rindex)); + count = lgfs2_writei(ip, &ri, ip->i_size, sizeof(struct gfs2_rindex)); if (count != sizeof(struct gfs2_rindex)) { lgfs2_inode_free(&ip); return NULL; } } - memset(buf, 0, sizeof(struct gfs2_rindex)); - count = __lgfs2_writei(ip, buf, ip->i_size, sizeof(struct gfs2_rindex), 0); + memset(&ri, 0, sizeof(struct gfs2_rindex)); + count = __lgfs2_writei(ip, &ri, ip->i_size, sizeof(struct gfs2_rindex), 0); if (count != sizeof(struct gfs2_rindex)) { lgfs2_inode_free(&ip); return NULL;