fix (mostly) minor nits spotted by gcc sanitization
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.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/4 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 files 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 · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 1/4] libxfs: avoid negative (and full-width) shifts in radix-tree.c
Quoted 34 lines · Eric Sandeen
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 -0700avoid negative (and full-width) shifts in
radix-tree.cNegative 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.
Quoted 4 lines
Signed-off-by Peter Lund <[email protected]>Signed-off-by Eric Sandeen <[email protected]>---
Quoted 35 lines
libxfs/radix-tree.c | 14 ++++++++------ 1 files 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 · 6 lines
1.7.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 2/4 xfs_repair: fix unaligned accesses
In reply to Eric SandeenThis fixes some unaligned accesses spotted by libubsan in repair.
--- repair/dinode.c | 19 +++++++++---------- repair/prefetch.c | 4 ++-- 2 files changed, 11 insertions(+), 12 deletions(-)
repair/dinode.c +9 -10
@@ -960,13 +960,13 @@ _("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]))) { +if (!verify_dfsbno (mp,get_unaligned_be64 (&pp[i]))) {do_warn (_ ("bad bmap btree ptr 0x%llx in ino %" PRIu64"\n" ), - (unsigned long long )be64_to_cpu (pp[i]), lino); +get_unaligned_be64 (&pp[i]), lino);return (1 ); } -if (scan_lbtree (be64_to_cpu (pp[i]), level, scan_bmapbt, type, +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 );
@@ -977,25 +977,24 @@ _("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), +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 ( +cpu_to_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), +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 · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 2/4] xfs_repair: fix unaligned accesses
Quoted 2 lines · Eric Sandeen
This fixes some unaligned accesses spotted by libubsan in repair.
Could we add a couple sentences about why this is a problem? I take it unaligned accesses are "bad" on certain arches..?
Quoted 39 lines
Signed-off-by Eric Sandeen <[email protected]>--- repair/dinode.c | 19 +++++++++---------- repair/prefetch.c | 4 ++-- 2 files changed, 11 insertions(+), 12 deletions(-)repair/dinode.c +4
-6@@ -960,13 +960,13 @@ _("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]))) { +if (!verify_dfsbno (mp,get_unaligned_be64 (&pp[i]))) {do_warn (_ ("bad bmap btree ptr 0x%llx in ino %" PRIu64"\n" ), - (unsigned long long )be64_to_cpu (pp[i]), lino); +get_unaligned_be64 (&pp[i]), lino);return (1 ); } -if (scan_lbtree (be64_to_cpu (pp[i]), level, scan_bmapbt, type, +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 );@@ -977,25 +977,24 @@ _("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),
repair/dinode.c _("bad numrecs 0 in inode %" PRIu64 " bmap btree root block\n"), · line 992
Quoted 10 lines
repair/dinode.c +4
-2+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 ( +cpu_to_be64 (cursor.level[level-1 ].first_key), + &pkey[i].br_startoff);
I could be confused here... but if get_unaligned_be64() takes a be64 and transforms to cpu order, shouldn't put_unaligned_be64() take a cpu order parameter? Is this a double byte order swap?
Brian
Quoted 39 lines
repair/dinode.c +1
-2}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), +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 · 6 lines
1.7.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/4] xfs_repair: fix unaligned accesses
Quoted 6 lines · Brian Foster
Quoted 2 lines · Eric Sandeen
This fixes some unaligned accesses spotted by libubsan in repair.
Could we add a couple sentences about why this is a problem? I take it unaligned accesses are "bad" on certain arches..?
To the commit perhaps? Probably not in the code, we have lots of places that do this trick, or use [get|put]_unaligned_be[32|64] with no explanation of the problem.
Documentation/unaligned-memory-access.txt in the kernel covers it:
Why unaligned access is bad
===========================
The effects of performing an unaligned memory access vary from architecture to architecture. It would be easy to write a whole document on the differences here; a summary of the common scenarios is presented below:
- Some architectures are able to perform unaligned memory accesses transparently, but there is usually a significant performance cost.
- Some architectures raise processor exceptions when unaligned accesses happen. The exception handler is able to correct the unaligned access, at significant cost to performance.
- Some architectures raise processor exceptions when unaligned accesses happen, but the exceptions do not contain enough information for the unaligned access to be corrected.
- Some architectures are not capable of unaligned memory access, but will silently perform a different memory access to the one that was requested, resulting in a subtle code bug that is hard to detect!
It should be obvious from the above that if your code causes unaligned memory accesses to happen, your code will not work correctly on certain platforms and will cause performance problems on others.
Maybe I can refer to that in the commit?
Quoted 91 lines
Quoted 39 lines
Signed-off-by Eric Sandeen <[email protected]>--- repair/dinode.c | 19 +++++++++---------- repair/prefetch.c | 4 ++-- 2 files changed, 11 insertions(+), 12 deletions(-)repair/dinode.c +4
-6@@ -960,13 +960,13 @@ _("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]))) { +if (!verify_dfsbno (mp,get_unaligned_be64 (&pp[i]))) {do_warn (_ ("bad bmap btree ptr 0x%llx in ino %" PRIu64"\n" ), - (unsigned long long )be64_to_cpu (pp[i]), lino); +get_unaligned_be64 (&pp[i]), lino);return (1 ); } -if (scan_lbtree (be64_to_cpu (pp[i]), level, scan_bmapbt, type, +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 );@@ -977,25 +977,24 @@ _("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),repair/dinode.c _("bad numrecs 0 in inode %" PRIu64 " bmap btree root block\n"), · line 992
Quoted 10 lines
repair/dinode.c +4
-2+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 ( +cpu_to_be64 (cursor.level[level-1 ].first_key), + &pkey[i].br_startoff);I could be confused here... but if
get_unaligned_be64()takes a be64 and transforms to cpu order, shouldn'tput_unaligned_be64()take a cpu order parameter? Is this a double byte order swap?Brian
Quoted 35 lines
repair/dinode.c +1
-2}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), +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 · 1 line
1.7.1
Quoted 10 lines
Quoted 4 lines
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs_______________________________________________
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/4] xfs_repair: fix unaligned accesses
In reply to Eric SandeenQuoted 13 lines · Eric Sandeen
This fixes some unaligned accesses spotted by libubsan in repair.
Signed-off-by Eric Sandeen <[email protected]>--- repair/dinode.c | 19 +++++++++---------- repair/prefetch.c | 4 ++-- 2 files changed, 11 insertions(+), 12 deletions(-)repair/dinode.c +0
-0
repair/dinode.c _("bad numrecs 0 in inode %" PRIu64 " bmap btree root block\n"), · line 963
Quoted 5 lines
repair/dinode.c +1
-1@@ -960,13 +960,13 @@ _("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]))) { +if (!verify_dfsbno (mp,get_unaligned_be64 (&pp[i]))) {
I don't understand - when are pointers in the BMBT not 64 bit aligned? The buffers are allocated by memalign to be 64 bit aligned, and all the internal BMBT structures are 64 bit aligned, too. i.e the BMBT block header is 24/72 bytes in length (depending on CRCs), the pointers are 64 bit, and the records are 128 bit.
So where's the unaligned access coming from?
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 2/4] xfs_repair: fix unaligned accesses
Quoted 27 lines · Dave Chinner
Quoted 15 lines · Eric Sandeen
This fixes some unaligned accesses spotted by libubsan in repair.
Signed-off-by Eric Sandeen <[email protected]>---
repair/dinode.c| 19 +++++++++----------repair/prefetch.c| 4 ++--2 files changed, 11 insertions(+), 12 deletions(-)repair/dinode.c +0
-0repair/dinode.c _("bad numrecs 0 in inode %" PRIu64 " bmap btree root block\n"), · line 963
Quoted 5 lines
repair/dinode.c +1
-1@@ -960,13 +960,13 @@ _("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]))) { +if (!verify_dfsbno (mp,get_unaligned_be64 (&pp[i]))) {I don't understand - when are pointers in the BMBT not 64 bit aligned? The buffers are allocated by memalign to be 64 bit aligned, and all the internal BMBT structures are 64 bit aligned, too. i.e the BMBT block header is 24/72 bytes in length (depending on CRCs), the pointers are 64 bit, and the records are 128 bit.
Quoted 2 lines
So where's the unaligned access coming from?
Hrm, I'm sure I hit it and just took it at face value, sorry - I'll run through it again and look more closely...
Thanks,
Eric
Quoted 12 lines
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ 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/4] xfs_repair: fix unaligned accesses
In reply to Dave ChinnerQuoted 26 lines · Dave Chinner
Quoted 13 lines · Eric Sandeen
This fixes some unaligned accesses spotted by libubsan in repair.
Signed-off-by Eric Sandeen <[email protected]>--- repair/dinode.c | 19 +++++++++---------- repair/prefetch.c | 4 ++-- 2 files changed, 11 insertions(+), 12 deletions(-)repair/dinode.c +0
-0repair/dinode.c _("bad numrecs 0 in inode %" PRIu64 " bmap btree root block\n"), · line 963
Quoted 5 lines
repair/dinode.c +1
-1@@ -960,13 +960,13 @@ _("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]))) { +if (!verify_dfsbno (mp,get_unaligned_be64 (&pp[i]))) {I don't understand - when are pointers in the BMBT not 64 bit aligned? The buffers are allocated by memalign to be 64 bit aligned, and all the internal BMBT structures are 64 bit aligned, too. i.e the BMBT block header is 24/72 bytes in length (depending on CRCs), the pointers are 64 bit, and the records are 128 bit.
Quoted 1 line
So where's the unaligned access coming from?
Ok, so on a recheck, I'm not crazy w.r.t. what gcc said, anyway:
dinode.c:964:26: runtime error: load of misaligned address 0x7fc4f800ef54 for type 'xfs_bmbt_ptr_t', which requires 8 byte alignment 0x7fc4f800ef54: note: pointer points here
00 00 00 00 00 00 00 00 00 20 38 5e 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
^
with some added printfs, it came from:
pp = XFS_BMDR_PTR_ADDR(dib, 1,
xfs_bmdr_maxrecs(XFS_DFORK_SIZE(dip, mp, whichfork), 0));
printf("dib at %p pp at %p\n", dib, pp);dib at 0x7fc4f800eeb0 pp at 0x7fc4f800ef54
so pp is at not an 8-multiple away from dib ...now how'd that happen?
#define XFS_BMDR_PTR_ADDR(block, index, maxrecs) \
((xfs_bmdr_ptr_t *) \
((char *)(block) + \
sizeof(struct xfs_bmdr_block) + \
(maxrecs) * sizeof(xfs_bmdr_key_t) + \
((index) - 1) * sizeof(xfs_bmdr_ptr_t)))xfs_bmdr_block is 32 bits, not 64.
But everything in my patch is BMDR not BMBT, right? I don't think I ran into any problems in BMBT land, and
#define XFS_BMBT_PTR_ADDR(mp, block, index, maxrecs) \
((xfs_bmbt_ptr_t *) \
((char *)(block) + \
XFS_BMBT_BLOCK_LEN(mp) + \
(maxrecs) * sizeof(xfs_bmbt_key_t) + \
((index) - 1) * sizeof(xfs_bmbt_ptr_t)))all those offsets seem fine.
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 2/4] xfs_repair: fix unaligned accesses
Quoted 38 lines · Eric Sandeen
Quoted 26 lines · Dave Chinner
Quoted 13 lines · Eric Sandeen
This fixes some unaligned accesses spotted by libubsan in repair.
Signed-off-by Eric Sandeen <[email protected]>--- repair/dinode.c | 19 +++++++++---------- repair/prefetch.c | 4 ++-- 2 files changed, 11 insertions(+), 12 deletions(-)repair/dinode.c +0
-0repair/dinode.c _("bad numrecs 0 in inode %" PRIu64 " bmap btree root block\n"), · line 963
Quoted 5 lines
repair/dinode.c +1
-1@@ -960,13 +960,13 @@ _("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]))) { +if (!verify_dfsbno (mp,get_unaligned_be64 (&pp[i]))) {I don't understand - when are pointers in the BMBT not 64 bit aligned? The buffers are allocated by memalign to be 64 bit aligned, and all the internal BMBT structures are 64 bit aligned, too. i.e the BMBT block header is 24/72 bytes in length (depending on CRCs), the pointers are 64 bit, and the records are 128 bit.
Quoted 1 line
So where's the unaligned access coming from?
Ok, so on a recheck, I'm not crazy w.r.t. what gcc said, anyway:
dinode.c:964:26: runtime error: load of misaligned address
0x7fc4f800ef54for type 'xfs_bmbt_ptr_t', which requires 8 byte alignment0x7fc4f800ef54: note: pointer points here
00 00 00 00 00 00 00 00 00 20 38 5e 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00^
Quoted 7 lines
with some added printfs, it came from:
pp = XFS_BMDR_PTR_ADDR(dib, 1, xfs_bmdr_maxrecs(XFS_DFORK_SIZE(dip, mp, whichfork), 0)); printf("dib at %p pp at %p\n", dib, pp);dib at
0x7fc4f800eeb0pp at0x7fc4f800ef54
Ah, ok, it's in extent format in the inode fork, not in btree format in blocks. Let me go back and look at it again.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 2/4] xfs_repair: fix unaligned accesses
Quoted 41 lines · Dave Chinner
Quoted 40 lines · Eric Sandeen
Quoted 26 lines · Dave Chinner
Quoted 13 lines · Eric Sandeen
This fixes some unaligned accesses spotted by libubsan in repair.
Signed-off-by Eric Sandeen <[email protected]>--- repair/dinode.c | 19 +++++++++---------- repair/prefetch.c | 4 ++-- 2 files changed, 11 insertions(+), 12 deletions(-)repair/dinode.c +0
-0repair/dinode.c _("bad numrecs 0 in inode %" PRIu64 " bmap btree root block\n"), · line 963
Quoted 5 lines
repair/dinode.c +1
-1@@ -960,13 +960,13 @@ _("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]))) { +if (!verify_dfsbno (mp,get_unaligned_be64 (&pp[i]))) {I don't understand - when are pointers in the BMBT not 64 bit aligned? The buffers are allocated by memalign to be 64 bit aligned, and all the internal BMBT structures are 64 bit aligned, too. i.e the BMBT block header is 24/72 bytes in length (depending on CRCs), the pointers are 64 bit, and the records are 128 bit.
Quoted 1 line
So where's the unaligned access coming from?
Ok, so on a recheck, I'm not crazy w.r.t. what gcc said, anyway:
dinode.c:964:26: runtime error: load of misaligned address
0x7fc4f800ef54for type 'xfs_bmbt_ptr_t', which requires 8 byte alignment0x7fc4f800ef54: note: pointer points here
00 00 00 00 00 00 00 00 00 20 38 5e 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00^with some added printfs, it came from:
Quoted 8 lines
Quoted 5 lines
pp = XFS_BMDR_PTR_ADDR(dib, 1, xfs_bmdr_maxrecs(XFS_DFORK_SIZE(dip, mp, whichfork), 0)); printf("dib at %p pp at %p\n", dib, pp);dib at
0x7fc4f800eeb0pp at0x7fc4f800ef54Ah, ok, it's in extent format in the inode fork, not in btree format in blocks. Let me go back and look at it again.
My head was not screwed on properly that early in the morning. BMDR is the btree root block in the inode, not an extent format inode. And that set of pointers are being walked as an array which is then fed into the block scan itself. OK, makes sense now.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 3/4 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.
--- logprint/log_misc.c | 18 +++++++++++++++--- repair/btree.c | 1 + 2 files changed, 16 insertions(+), 3 deletions(-)
logprint/log_misc.c +15 -3
@@ -325,7 +325,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_s; + +/* memmove because *ptr may not be 8-byte aligned */ +memmove (&agi_s, *ptr,sizeof (struct xfs_agi )); + agi = &agi_s;printf (_ ("AGI Buffer: XAGI " ));/* * v4 filesystems only contain the fields before the uuid.
@@ -375,7 +379,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_s; + +/* memmove because *ptr may not be 8-byte aligned */ +memmove (&agf_s, *ptr,sizeof (struct xfs_agf )); + agf = &agf_s;printf (_ ("AGF Buffer: XAGF " ));/* * v4 filesystems only contain the fields before the uuid.
@@ -408,7 +416,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_s; + +/* memmove because *ptr may not be 8-byte aligned */ +memmove (&dq_s, *ptr,sizeof (struct xfs_disk_dquot )); + dq = &dq_s;printf (_ ("DQUOT Buffer: DQ " ));if (be32_to_cpu (head->oh_len) <sizeof (xfs_disk_dquot_t)) {
repair/btree.c +1 -0
@@ -230,6 +230,7 @@ btree_get_next( }if (level ==0 ) {if (key) { +/* XXXX what if index past MAX? What if no next? */ cur->index++; *key =btree_key_of_cursor (cur, root->height); cur->index--;
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/4] xfs_logprint: fix some unaligned accesses
Quoted 14 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]>--- logprint/log_misc.c | 18 +++++++++++++++--- repair/btree.c | 1 + 2 files changed, 16 insertions(+), 3 deletions(-)logprint/log_misc.c +0
-0
logprint/log_misc.c xlog_print_trans_buffer(char **ptr, int len, int *i, int num_ops) · line 332
Quoted 9 lines
logprint/log_misc.c +5
-1@@ -325,7 +325,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_s; + +/* memmove because *ptr may not be 8-byte aligned */ +memmove (&agi_s, *ptr,sizeof (struct xfs_agi )); + agi = &agi_s;
Nit: could we either define the new variables in the same scope as the pointer (either here or at the top of the function), or just ditch the pointers altogether?
Quoted 3 lines
logprint/log_misc.c +0
-0printf (_ ("AGI Buffer: XAGI " ));/* * v4 filesystems only contain the fields before the uuid.
...
repair/btree.c btree_get_next( · line 233
Quoted 9 lines
repair/btree.c +1
-0@@ -230,6 +230,7 @@ btree_get_next( }if (level ==0 ) {if (key) { +/* XXXX what if index past MAX? What if no next? */
Unintentional hunk?
Brian
Quoted 10 lines
repair/btree.c +0
-0cur->index++; *key =btree_key_of_cursor (cur, root->height); cur->index--;Signature · 6 lines
1.7.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 3/4] xfs_logprint: fix some unaligned accesses
Quoted 20 lines · Brian Foster
Quoted 19 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]>--- logprint/log_misc.c | 18 +++++++++++++++--- repair/btree.c | 1 + 2 files changed, 16 insertions(+), 3 deletions(-)logprint/log_misc.c +1
-1@@ -325,7 +325,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_s;
Quoted 8 lines
logprint/log_misc.c xlog_print_trans_buffer(char **ptr, int len, int *i, int num_ops) · line 332
Quoted 4 lines
logprint/log_misc.c +4
-0+ +/* memmove because *ptr may not be 8-byte aligned */ +memmove (&agi_s, *ptr,sizeof (struct xfs_agi )); + agi = &agi_s;Nit: could we either define the new variables in the same scope as the pointer (either here or at the top of the function), or just ditch the pointers altogether?
Let me see how that looks, sure.
Quoted 4 lines
Quoted 3 lines
logprint/log_misc.c +0
-0printf (_ ("AGI Buffer: XAGI " ));/* * v4 filesystems only contain the fields before the uuid. ...
Quoted 11 lines
repair/btree.c btree_get_next( · line 233
Quoted 9 lines
repair/btree.c +1
-0@@ -230,6 +230,7 @@ btree_get_next( }if (level ==0 ) {if (key) { +/* XXXX what if index past MAX? What if no next? */ Unintentional hunk?
Yeah, dammit, I thought I removed that, sorry.
Thanks,
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 4/4 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...
The code in prefetch.c has another issue with large fs blocks (32 or 64k) because it shifts by up to 128 bits, but that's left for later...
--- 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
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 4/4] xfs_repair: fix left-shift overflows
Quoted 4 lines · Eric Sandeen
pmask in struct
parent_listis 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.
Quoted 8 lines
Also spotted by libubsan...
The code in
prefetch.chas another issue with large fs blocks (32 or 64k) because it shifts by up to 128 bits, but that's left for later...Signed-off-by Eric Sandeen <[email protected]>---
Quoted 77 lines
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
1.7.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