fix (mostly) minor nits spotted by gcc sanitization
v2 cover fix (mostly) minor nits spotted by gcc sanitization
newer gcc supports -fsanitize=undefined, which spits out errors when the code does something ... undefined.
These are a handful of the (IMHO) obvious and unobtrusive fixes that knock down the noise.
One more patch than last time, found a couple things in metadump as well.
Thanks,
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 1/5 libxfs: avoid negative (and full-width) shifts in radix-tree.c
Pull this commit over from the kernel, as libubsan spotted a bad shift at runtime:
commit 430d275a399175c7c0673459738979287ec1fd22
Author: Peter Lund <[email protected]>
Date: Tue Oct 16 23:29:35 2007 -0700
avoid negative (and full-width) shifts in radix-tree.c
Negative shifts are not allowed in C (the result is undefined). Same thing with full-width shifts.
It works on most platforms but not on the VAX with gcc 4.0.1 (it results in an "operand reserved" fault).
Shifting by more than the width of the value on the left is also not allowed. I think the extra '>> 1' tacked on at the end in the original code was an attempt to work around that. Getting rid of that is an extra feature of this patch.
Here's the chapter and verse, taken from the final draft of the C99 standard ("6.5.7 Bitwise shift operators", paragraph 3):
"The integer promotions are performed on each of the operands. The type of the result is that of the promoted left operand. If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined."
Thank you to Jan-Benedict Glaw, Christoph Hellwig, Maciej Rozycki, Pekka Enberg, Andreas Schwab, and Christoph Lameter for review. Special thanks to Andreas for spotting that my fix only removed half the undefined behaviour.
--- libxfs/radix-tree.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
libxfs/radix-tree.c +8 -6
@@ -784,12 +784,14 @@ int radix_tree_tagged(struct radix_tree_root *root, unsigned int tag)static unsigned long __maxindex (unsigned int height) { -unsigned int tmp = height *RADIX_TREE_MAP_SHIFT ; -unsigned long index = (~0UL >> (RADIX_TREE_INDEX_BITS - tmp -1 )) >>1 ; - -if (tmp >=RADIX_TREE_INDEX_BITS ) - index = ~0UL ; -return index; +unsigned int width = height *RADIX_TREE_MAP_SHIFT ; +int shift =RADIX_TREE_INDEX_BITS - width; + +if (shift <0 ) +return ~0UL ; +if (shift >=BITS_PER_LONG ) +return 0UL ; +return ~0UL >> shift; }static void radix_tree_init_maxindex (void )
Signature · 7 lines
2.6.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 2/5 xfs_repair: fix unaligned accesses
In reply to Eric SandeenThis fixes some unaligned accesses spotted by libubsan in repair.
See Documentation/unaligned-memory-access.txt in the kernel tree for why these can be a problem.
---
V2:
Add note about why ...
Add another in libxfs_bmbt_disk_get_all
Fix mistaken double-swap in dinode.c in original patch
include/libxfs.h | 4 ++-- repair/dinode.c | 47 ++++++++++++++++++++++++----------------------- repair/prefetch.c | 4 ++-- 3 files changed, 28 insertions(+), 27 deletions(-)
include/libxfs.h +2 -2
@@ -206,8 +206,8 @@ libxfs_bmbt_disk_get_all( {struct xfs_bmbt_rec_host hrec; - hrec.l0 =be64_to_cpu (rp->l0); - hrec.l1 =be64_to_cpu (rp->l1); + hrec.l0 =get_unaligned_be64 (&rp->l0); + hrec.l1 =get_unaligned_be64 (&rp->l1);libxfs_bmbt_get_all (&hrec, irec); }
repair/dinode.c +24 -23
67 lines not on this page · raw message
repair/prefetch.c +2 -2
@@ -330,7 +330,7 @@ pf_scanfunc_bmap( pp =XFS_BMBT_PTR_ADDR (mp, block,1 , mp->m_bmap_dmxr[1 ]);for (i =0 ; i < numrecs; i++) { - dbno =be64_to_cpu (pp[i]); + dbno =get_unaligned_be64 (&pp[i]);if (!verify_dfsbno (mp, dbno))return 0 ;if (!pf_scan_lbtree (dbno, level, isadir, args, pf_scanfunc_bmap))
@@ -372,7 +372,7 @@ pf_read_btinode( pp =XFS_BMDR_PTR_ADDR (dib,1 ,xfs_bmdr_maxrecs (dsize,0 ));for (i =0 ; i < numrecs; i++) { - dbno =be64_to_cpu (pp[i]); + dbno =get_unaligned_be64 (&pp[i]);if (!verify_dfsbno (mp, dbno))break ;if (!pf_scan_lbtree (dbno, level, isadir, args, pf_scanfunc_bmap))
Signature · 7 lines
2.6.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 2/5] xfs_repair: fix unaligned accesses
Quoted 7 lines · Eric Sandeen
This fixes some unaligned accesses spotted by libubsan in repair.
See
Documentation/unaligned-memory-access.txtin the kernel tree for why these can be a problem.Signed-off-by Eric Sandeen <[email protected]>---
Quoted 129 lines
V2:
Add note about why ...
Add another inlibxfs_bmbt_disk_get_all
Fix mistaken double-swap indinode.cin original patchinclude/libxfs.h | 4 ++-- repair/dinode.c | 47 ++++++++++++++++++++++++----------------------- repair/prefetch.c | 4 ++-- 3 files changed, 28 insertions(+), 27 deletions(-)include/libxfs.h +2
-2@@ -206,8 +206,8 @@ libxfs_bmbt_disk_get_all( {struct xfs_bmbt_rec_host hrec; - hrec.l0 =be64_to_cpu (rp->l0); - hrec.l1 =be64_to_cpu (rp->l1); + hrec.l0 =get_unaligned_be64 (&rp->l0); + hrec.l1 =get_unaligned_be64 (&rp->l1);libxfs_bmbt_get_all (&hrec, irec); }repair/dinode.c +24
-23@@ -960,15 +960,17 @@ _("bad numrecs 0 in inode %" PRIu64 " bmap btree root block\n"),* btree, we'd do it right here. For now, if there's a * problem, we'll bail out and presumably clear the inode. */ -if (!verify_dfsbno (mp,be64_to_cpu (pp[i]))) { -do_warn (_ ("bad bmap btree ptr 0x%llx in ino %" PRIu64"\n" ), - (unsigned long long )be64_to_cpu (pp[i]), lino); +if (!verify_dfsbno (mp,get_unaligned_be64 (&pp[i]))) { +do_warn ( +("bad bmap btree ptr 0x%" PRIx64" in ino %" PRIu64"\n" ), +get_unaligned_be64 (&pp[i]), lino);return (1 ); } -if (scan_lbtree (be64_to_cpu (pp[i]), level, scan_bmapbt, type, - whichfork, lino, tot, nex, blkmapp, &cursor, -1 , check_dups, magic, &xfs_bmbt_buf_ops)) +if (scan_lbtree (get_unaligned_be64 (&pp[i]), level, scan_bmapbt, + type, whichfork, lino, tot, nex, blkmapp, + &cursor,1 , check_dups, magic, + &xfs_bmbt_buf_ops))return (1 );/* * fix key (offset) mismatches between the keys in root @@ -977,28 +979,27 @@ _("bad numrecs 0 in inode %" PRIu64 " bmap btree root block\n"),* blocks but the parent hasn't been updated */ if (!check_dups && cursor.level[level-1 ].first_key != -be64_to_cpu (pkey[i].br_startoff)) { +get_unaligned_be64 (&pkey[i].br_startoff)) {if (!no_modify) {do_warn ( -_ ("correcting key in bmbt root (was %llu, now %" PRIu64") in inode " -"%" PRIu64" %s fork\n" ), - (unsigned long long ) -be64_to_cpu (pkey[i].br_startoff), - cursor.level[level-1 ].first_key, -XFS_AGINO_TO_INO (mp, agno, ino), - forkname); +_ ("correcting key in bmbt root (was %" PRIu64", now %" PRIu64") in inode " +"%" PRIu64" %s fork\n" ), +get_unaligned_be64 (&pkey[i].br_startoff), + cursor.level[level-1 ].first_key, +XFS_AGINO_TO_INO (mp, agno, ino), + forkname); *dirty =1 ; - pkey[i].br_startoff =cpu_to_be64 ( - cursor.level[level-1 ].first_key); +put_unaligned_be64 ( + cursor.level[level-1 ].first_key, + &pkey[i].br_startoff); }else {do_warn ( -_ ("bad key in bmbt root (is %llu, would reset to %" PRIu64") in inode " -"%" PRIu64" %s fork\n" ), - (unsigned long long ) -be64_to_cpu (pkey[i].br_startoff), - cursor.level[level-1 ].first_key, -XFS_AGINO_TO_INO (mp, agno, ino), - forkname); +_ ("bad key in bmbt root (is %" PRIu64", would reset to %" PRIu64") in inode " +"%" PRIu64" %s fork\n" ), +get_unaligned_be64 (&pkey[i].br_startoff), + cursor.level[level-1 ].first_key, +XFS_AGINO_TO_INO (mp, agno, ino), + forkname); } }/* repair/prefetch.c +2
-2@@ -330,7 +330,7 @@ pf_scanfunc_bmap( pp =XFS_BMBT_PTR_ADDR (mp, block,1 , mp->m_bmap_dmxr[1 ]);for (i =0 ; i < numrecs; i++) { - dbno =be64_to_cpu (pp[i]); + dbno =get_unaligned_be64 (&pp[i]);if (!verify_dfsbno (mp, dbno))return 0 ;if (!pf_scan_lbtree (dbno, level, isadir, args, pf_scanfunc_bmap))@@ -372,7 +372,7 @@ pf_read_btinode( pp =XFS_BMDR_PTR_ADDR (dib,1 ,xfs_bmdr_maxrecs (dsize,0 ));for (i =0 ; i < numrecs; i++) { - dbno =be64_to_cpu (pp[i]); + dbno =get_unaligned_be64 (&pp[i]);if (!verify_dfsbno (mp, dbno))break ;if (!pf_scan_lbtree (dbno, level, isadir, args, pf_scanfunc_bmap))Signature · 7 lines
2.6.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 2/5] xfs_repair: fix unaligned accesses
Quoted 10 lines · Brian Foster
Quoted 7 lines · Eric Sandeen
This fixes some unaligned accesses spotted by libubsan in repair.
See
Documentation/unaligned-memory-access.txtin the kernel tree for why these can be a problem.Signed-off-by Eric Sandeen <[email protected]>---
Reviewed-by Brian Foster <[email protected]>
....
Quoted 11 lines
repair/dinode.c _("bad numrecs 0 in inode %" PRIu64 " bmap btree root block\n"), · line 966
Quoted 11 lines
repair/dinode.c +4
-3@@ -960,15 +960,17 @@ _("bad numrecs 0 in inode %" PRIu64 " bmap btree root block\n"),* btree, we'd do it right here. For now, if there's a * problem, we'll bail out and presumably clear the inode. */ -if (!verify_dfsbno (mp,be64_to_cpu (pp[i]))) { -do_warn (_ ("bad bmap btree ptr 0x%llx in ino %" PRIu64"\n" ), - (unsigned long long )be64_to_cpu (pp[i]), lino); +if (!verify_dfsbno (mp,get_unaligned_be64 (&pp[i]))) { +do_warn ( +("bad bmap btree ptr 0x%" PRIx64" in ino %" PRIu64"\n" ), +get_unaligned_be64 (&pp[i]), lino);
drops the "_" from the translation string. I'll fix it on commit.
/me can't help but think that a local variable or two would make this code so much more readable....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 3/5 xfs_logprint: fix some unaligned accesses
In reply to Eric SandeenThis routine had a fair bit of gyration to avoid unaligned accesses, but didn't fix them all.
Fix some more spotted at runtime by libubsan.
---
V2: Change variable scope, remove extraneous hunk
logprint/log_misc.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
logprint/log_misc.c +15 -6
@@ -243,9 +243,6 @@ int xlog_print_trans_buffer(char **ptr, int len, int *i, int num_ops) {xfs_buf_log_format_t *f; -xfs_agi_t *agi; -xfs_agf_t *agf; -xfs_disk_dquot_t *dq;xlog_op_header_t *head =NULL ;int num, skip;int super_block =0 ;
@@ -325,7 +322,11 @@ xlog_print_trans_buffer(char **ptr, int len, int *i, int num_ops) } super_block =0 ; }else if (be32_to_cpu (*(__be32 *)(*ptr)) ==XFS_AGI_MAGIC ) { - agi = (xfs_agi_t *)(*ptr); +struct xfs_agi *agi, agi_s; + +/* memmove because *ptr may not be 8-byte aligned */ + agi = &agi_s; +memmove (agi, *ptr,sizeof (struct xfs_agi ));printf (_ ("AGI Buffer: XAGI " ));/* * v4 filesystems only contain the fields before the uuid.
@@ -375,7 +376,11 @@ xlog_print_trans_buffer(char **ptr, int len, int *i, int num_ops) } } }else if (be32_to_cpu (*(__be32 *)(*ptr)) ==XFS_AGF_MAGIC ) { - agf = (xfs_agf_t *)(*ptr); +struct xfs_agf *agf, agf_s; + +/* memmove because *ptr may not be 8-byte aligned */ + agf = &agf_s; +memmove (agf, *ptr,sizeof (struct xfs_agf ));printf (_ ("AGF Buffer: XAGF " ));/* * v4 filesystems only contain the fields before the uuid.
@@ -408,7 +413,11 @@ xlog_print_trans_buffer(char **ptr, int len, int *i, int num_ops) be32_to_cpu(agf->agf_longest)); } }else if (be32_to_cpu (*(__be32 *)(*ptr)) ==XFS_DQUOT_MAGIC ) { - dq = (xfs_disk_dquot_t *)(*ptr); +struct xfs_disk_dquot *dq, dq_s; + +/* memmove because *ptr may not be 8-byte aligned */ + dq = &dq_s; +memmove (dq, *ptr,sizeof (struct xfs_disk_dquot ));printf (_ ("DQUOT Buffer: DQ " ));if (be32_to_cpu (head->oh_len) <sizeof (xfs_disk_dquot_t)) {
Signature · 7 lines
2.6.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/5] xfs_logprint: fix some unaligned accesses
Quoted 9 lines · Eric Sandeen
This routine had a fair bit of gyration to avoid unaligned accesses, but didn't fix them all.
Fix some more spotted at runtime by libubsan.Signed-off-by Eric Sandeen <[email protected]>---
V2: Change variable scope, remove extraneous hunk
Thanks!
Quoted 64 lines
logprint/log_misc.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)logprint/log_misc.c +15
-6@@ -243,9 +243,6 @@ int xlog_print_trans_buffer(char **ptr, int len, int *i, int num_ops) {xfs_buf_log_format_t *f; -xfs_agi_t *agi; -xfs_agf_t *agf; -xfs_disk_dquot_t *dq;xlog_op_header_t *head =NULL ;int num, skip;int super_block =0 ;@@ -325,7 +322,11 @@ xlog_print_trans_buffer(char **ptr, int len, int *i, int num_ops) } super_block =0 ; }else if (be32_to_cpu (*(__be32 *)(*ptr)) ==XFS_AGI_MAGIC ) { - agi = (xfs_agi_t *)(*ptr); +struct xfs_agi *agi, agi_s; + +/* memmove because *ptr may not be 8-byte aligned */ + agi = &agi_s; +memmove (agi, *ptr,sizeof (struct xfs_agi ));printf (_ ("AGI Buffer: XAGI " ));/* * v4 filesystems only contain the fields before the uuid. @@ -375,7 +376,11 @@ xlog_print_trans_buffer(char **ptr, int len, int *i, int num_ops) } } }else if (be32_to_cpu (*(__be32 *)(*ptr)) ==XFS_AGF_MAGIC ) { - agf = (xfs_agf_t *)(*ptr); +struct xfs_agf *agf, agf_s; + +/* memmove because *ptr may not be 8-byte aligned */ + agf = &agf_s; +memmove (agf, *ptr,sizeof (struct xfs_agf ));printf (_ ("AGF Buffer: XAGF " ));/* * v4 filesystems only contain the fields before the uuid. @@ -408,7 +413,11 @@ xlog_print_trans_buffer(char **ptr, int len, int *i, int num_ops) be32_to_cpu(agf->agf_longest)); } }else if (be32_to_cpu (*(__be32 *)(*ptr)) ==XFS_DQUOT_MAGIC ) { - dq = (xfs_disk_dquot_t *)(*ptr); +struct xfs_disk_dquot *dq, dq_s; + +/* memmove because *ptr may not be 8-byte aligned */ + dq = &dq_s; +memmove (dq, *ptr,sizeof (struct xfs_disk_dquot ));printf (_ ("DQUOT Buffer: DQ " ));if (be32_to_cpu (head->oh_len) <sizeof (xfs_disk_dquot_t)) {Signature · 7 lines
2.6.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 4/5 xfs_metadump: Fix unaligned accesses
In reply to Eric SandeenThis fixes some unaligned accesses spotted by libubsan in xfs_metadump.
--- db/metadump.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
db/metadump.c +4 -4
@@ -1872,8 +1872,8 @@ scanfunc_bmap(xfs_agnumber_t ag;xfs_agblock_t bno; - ag =XFS_FSB_TO_AGNO (mp,be64_to_cpu (pp[i])); - bno =XFS_FSB_TO_AGBNO (mp,be64_to_cpu (pp[i])); + ag =XFS_FSB_TO_AGNO (mp,get_unaligned_be64 (&pp[i])); + bno =XFS_FSB_TO_AGBNO (mp,get_unaligned_be64 (&pp[i]));if (bno ==0 || bno > mp->m_sb.sb_agblocks || ag > mp->m_sb.sb_agcount) {
@@ -1938,8 +1938,8 @@ process_btinode(xfs_agnumber_t ag;xfs_agblock_t bno; - ag =XFS_FSB_TO_AGNO (mp,be64_to_cpu (pp[i])); - bno =XFS_FSB_TO_AGBNO (mp,be64_to_cpu (pp[i])); + ag =XFS_FSB_TO_AGNO (mp,get_unaligned_be64 (&pp[i])); + bno =XFS_FSB_TO_AGBNO (mp,get_unaligned_be64 (&pp[i]));if (bno ==0 || bno > mp->m_sb.sb_agblocks || ag > mp->m_sb.sb_agcount) {
Signature · 7 lines
2.6.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 4/5] xfs_metadump: Fix unaligned accesses
Quoted 5 lines · Eric Sandeen
This fixes some unaligned accesses spotted by libubsan in
xfs_metadump.Signed-off-by Eric Sandeen <[email protected]>---
Looks good:
Quoted 37 lines
db/metadump.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)db/metadump.c +4
-4@@ -1872,8 +1872,8 @@ scanfunc_bmap(xfs_agnumber_t ag;xfs_agblock_t bno; - ag =XFS_FSB_TO_AGNO (mp,be64_to_cpu (pp[i])); - bno =XFS_FSB_TO_AGBNO (mp,be64_to_cpu (pp[i])); + ag =XFS_FSB_TO_AGNO (mp,get_unaligned_be64 (&pp[i])); + bno =XFS_FSB_TO_AGBNO (mp,get_unaligned_be64 (&pp[i]));if (bno ==0 || bno > mp->m_sb.sb_agblocks || ag > mp->m_sb.sb_agcount) {@@ -1938,8 +1938,8 @@ process_btinode(xfs_agnumber_t ag;xfs_agblock_t bno; - ag =XFS_FSB_TO_AGNO (mp,be64_to_cpu (pp[i])); - bno =XFS_FSB_TO_AGBNO (mp,be64_to_cpu (pp[i])); + ag =XFS_FSB_TO_AGNO (mp,get_unaligned_be64 (&pp[i])); + bno =XFS_FSB_TO_AGBNO (mp,get_unaligned_be64 (&pp[i]));if (bno ==0 || bno > mp->m_sb.sb_agblocks || ag > mp->m_sb.sb_agcount) {Signature · 7 lines
2.6.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 5/5 xfs_repair: fix left-shift overflows
In reply to Eric Sandeenpmask in struct parent_list is a __uint64_t, but in some places we populated it with "1LL << shift" where shift could be up to 63; this really needs to be a 1ULL type for this to be correct.
Also spotted by libubsan...
--- repair/incore_ino.c | 14 +++++++------- repair/prefetch.c | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-)
repair/incore_ino.c +7 -7
@@ -625,7 +625,7 @@ set_inode_parent(else irec->ino_un.plist = ptbl; - ptbl->pmask =1LL << offset; + ptbl->pmask =1ULL << offset; ptbl->pentries = (xfs_ino_t *)memalign (sizeof (xfs_ino_t),sizeof (xfs_ino_t));if (!ptbl->pentries)
@@ -638,8 +638,8 @@ set_inode_parent(return ; } -if (ptbl->pmask & (1LL << offset)) { - bitmask =1LL ; +if (ptbl->pmask & (1ULL << offset)) { + bitmask =1ULL ; target =0 ;for (i =0 ; i < offset; i++) {
@@ -655,7 +655,7 @@ set_inode_parent(return ; } - bitmask =1LL ; + bitmask =1ULL ; cnt = target =0 ;for (i =0 ; i <XFS_INODES_PER_CHUNK ; i++) {
@@ -691,7 +691,7 @@ set_inode_parent( ptbl->cnt++;#endif ptbl->pentries[target] = parent; - ptbl->pmask |= (1LL << offset); + ptbl->pmask |= (1ULL << offset); } xfs_ino_t
@@ -707,8 +707,8 @@ get_inode_parent(ino_tree_node_t *irec, int offset)else ptbl = irec->ino_un.plist; -if (ptbl->pmask & (1LL << offset)) { - bitmask =1LL ; +if (ptbl->pmask & (1ULL << offset)) { + bitmask =1ULL ; target =0 ;for (i =0 ; i < offset; i++) {
repair/prefetch.c +1 -1
@@ -762,7 +762,7 @@ pf_queuing_worker(* sparse state in cluster sized chunks as cluster size * is the min. granularity of sparse irec regions. */ -if ((sparse & ((1 << inodes_per_cluster) -1 )) ==0 ) +if ((sparse & ((1ULL << inodes_per_cluster) -1 )) ==0 )pf_queue_io (args, &map,1 , (cur_irec->ino_isa_dir !=0 ) ?B_DIR_INODE :B_INODE );
Signature · 6 lines
2.6.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs