[PATCH 0/4] fix (mostly) minor nits spotted by gcc sanitization

[PATCH 0/4] fix (mostly) minor nits spotted by gcc sanitization

[Eric Sandeen]

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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/4] libxfs: avoid negative (and full-width) shifts in radix-tree.c

[Eric Sandeen]

Pull this commit over from the kernel, as libubsan spotted a
bad shift at runtime:

    commit 430d275a399175c7c0673459738979287ec1fd22
    Author: Peter Lund <firefly@vax64.dk>
    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.

    Signed-off-by: Peter Lund <firefly@vax64.dk>

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 libxfs/radix-tree.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/libxfs/radix-tree.c b/libxfs/radix-tree.c
index 4d44ab4..eef9c36 100644
--- a/libxfs/radix-tree.c
+++ b/libxfs/radix-tree.c
@@ -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)
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/4] xfs_repair: fix unaligned accesses

[Eric Sandeen]

This fixes some unaligned accesses spotted by libubsan in repair.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 repair/dinode.c   |   19 +++++++++----------
 repair/prefetch.c |    4 ++--
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/repair/dinode.c b/repair/dinode.c
index f78f907..44bbb8f 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -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);
diff --git a/repair/prefetch.c b/repair/prefetch.c
index 32ec55e..52238ca 100644
--- a/repair/prefetch.c
+++ b/repair/prefetch.c
@@ -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))
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/4] xfs_logprint: fix some unaligned accesses

[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 <sandeen@redhat.com>
---
 logprint/log_misc.c |   18 +++++++++++++++---
 repair/btree.c      |    1 +
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/logprint/log_misc.c b/logprint/log_misc.c
index d76145c..6cd249a 100644
--- a/logprint/log_misc.c
+++ b/logprint/log_misc.c
@@ -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)) {
diff --git a/repair/btree.c b/repair/btree.c
index 66fb40b..e31e67a 100644
--- a/repair/btree.c
+++ b/repair/btree.c
@@ -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--;
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 4/4] xfs_repair: fix left-shift overflows

[Eric Sandeen]

pmask 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...

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 repair/incore_ino.c |   14 +++++++-------
 repair/prefetch.c   |    2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/repair/incore_ino.c b/repair/incore_ino.c
index 32d7678..1898257 100644
--- a/repair/incore_ino.c
+++ b/repair/incore_ino.c
@@ -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++)  {
diff --git a/repair/prefetch.c b/repair/prefetch.c
index 52238ca..b11dcb3 100644
--- a/repair/prefetch.c
+++ b/repair/prefetch.c
@@ -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);
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/4] libxfs: avoid negative (and full-width) shifts in radix-tree.c

[Brian Foster]

On Thu, Oct 08, 2015 at 07:24:53PM -0500, Eric Sandeen wrote:
> Pull this commit over from the kernel, as libubsan spotted a
> bad shift at runtime:
> 
>     commit 430d275a399175c7c0673459738979287ec1fd22
>     Author: Peter Lund <firefly@vax64.dk>
>     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.
> 
>     Signed-off-by: Peter Lund <firefly@vax64.dk>
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  libxfs/radix-tree.c |   14 ++++++++------
>  1 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/libxfs/radix-tree.c b/libxfs/radix-tree.c
> index 4d44ab4..eef9c36 100644
> --- a/libxfs/radix-tree.c
> +++ b/libxfs/radix-tree.c
> @@ -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)
> -- 
> 1.7.1
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/4] xfs_repair: fix unaligned accesses

[Brian Foster]

On Thu, Oct 08, 2015 at 07:25:24PM -0500, Eric Sandeen wrote:
> 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..?

> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  repair/dinode.c   |   19 +++++++++----------
>  repair/prefetch.c |    4 ++--
>  2 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index f78f907..44bbb8f 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -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);

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

>  			} 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);
> diff --git a/repair/prefetch.c b/repair/prefetch.c
> index 32ec55e..52238ca 100644
> --- a/repair/prefetch.c
> +++ b/repair/prefetch.c
> @@ -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))
> -- 
> 1.7.1
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/4] xfs_logprint: fix some unaligned accesses

[Brian Foster]

On Thu, Oct 08, 2015 at 07:25:50PM -0500, Eric Sandeen wrote:
> 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 <sandeen@redhat.com>
> ---
>  logprint/log_misc.c |   18 +++++++++++++++---
>  repair/btree.c      |    1 +
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/logprint/log_misc.c b/logprint/log_misc.c
> index d76145c..6cd249a 100644
> --- a/logprint/log_misc.c
> +++ b/logprint/log_misc.c
> @@ -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?

>  		printf(_("AGI Buffer: XAGI  "));
>  		/*
>  		 * v4 filesystems only contain the fields before the uuid.
...
> diff --git a/repair/btree.c b/repair/btree.c
> index 66fb40b..e31e67a 100644
> --- a/repair/btree.c
> +++ b/repair/btree.c
> @@ -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

>  			cur->index++;
>  			*key = btree_key_of_cursor(cur, root->height);
>  			cur->index--;
> -- 
> 1.7.1
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] xfs_repair: fix left-shift overflows

[Brian Foster]

On Thu, Oct 08, 2015 at 07:27:21PM -0500, Eric Sandeen wrote:
> pmask 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...
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  repair/incore_ino.c |   14 +++++++-------
>  repair/prefetch.c   |    2 +-
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/repair/incore_ino.c b/repair/incore_ino.c
> index 32d7678..1898257 100644
> --- a/repair/incore_ino.c
> +++ b/repair/incore_ino.c
> @@ -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++)  {
> diff --git a/repair/prefetch.c b/repair/prefetch.c
> index 52238ca..b11dcb3 100644
> --- a/repair/prefetch.c
> +++ b/repair/prefetch.c
> @@ -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);
> -- 
> 1.7.1
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/4] xfs_logprint: fix some unaligned accesses

[Eric Sandeen]

On 10/9/15 8:24 AM, Brian Foster wrote:
> On Thu, Oct 08, 2015 at 07:25:50PM -0500, Eric Sandeen wrote:
>> 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 <sandeen@redhat.com>
>> ---
>>  logprint/log_misc.c |   18 +++++++++++++++---
>>  repair/btree.c      |    1 +
>>  2 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/logprint/log_misc.c b/logprint/log_misc.c
>> index d76145c..6cd249a 100644
>> --- a/logprint/log_misc.c
>> +++ b/logprint/log_misc.c
>> @@ -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?

Let me see how that looks, sure.

>>  		printf(_("AGI Buffer: XAGI  "));
>>  		/*
>>  		 * v4 filesystems only contain the fields before the uuid.
> ...
>> diff --git a/repair/btree.c b/repair/btree.c
>> index 66fb40b..e31e67a 100644
>> --- a/repair/btree.c
>> +++ b/repair/btree.c
>> @@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/4] xfs_repair: fix unaligned accesses

[Eric Sandeen]

On 10/9/15 8:24 AM, Brian Foster wrote:
> On Thu, Oct 08, 2015 at 07:25:24PM -0500, Eric Sandeen wrote:
>> 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?

>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>  repair/dinode.c   |   19 +++++++++----------
>>  repair/prefetch.c |    4 ++--
>>  2 files changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/repair/dinode.c b/repair/dinode.c
>> index f78f907..44bbb8f 100644
>> --- a/repair/dinode.c
>> +++ b/repair/dinode.c
>> @@ -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);
> 
> 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
> 
>>  			} 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);
>> diff --git a/repair/prefetch.c b/repair/prefetch.c
>> index 32ec55e..52238ca 100644
>> --- a/repair/prefetch.c
>> +++ b/repair/prefetch.c
>> @@ -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))
>> -- 
>> 1.7.1
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/4] xfs_repair: fix unaligned accesses

[Dave Chinner]

On Thu, Oct 08, 2015 at 07:25:24PM -0500, Eric Sandeen wrote:
> This fixes some unaligned accesses spotted by libubsan in repair.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  repair/dinode.c   |   19 +++++++++----------
>  repair/prefetch.c |    4 ++--
>  2 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index f78f907..44bbb8f 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -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.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/4] xfs_repair: fix unaligned accesses

[Eric Sandeen]

On Oct 11, 2015, at 5:26 PM, Dave Chinner <david@fromorbit.com> wrote:
> 
>> On Thu, Oct 08, 2015 at 07:25:24PM -0500, Eric Sandeen wrote:
>> This fixes some unaligned accesses spotted by libubsan in repair.
>> 
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>> repair/dinode.c   |   19 +++++++++----------
>> repair/prefetch.c |    4 ++--
>> 2 files changed, 11 insertions(+), 12 deletions(-)
>> 
>> diff --git a/repair/dinode.c b/repair/dinode.c
>> index f78f907..44bbb8f 100644
>> --- a/repair/dinode.c
>> +++ b/repair/dinode.c
>> @@ -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?
> 
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

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/4] xfs_repair: fix unaligned accesses

[Eric Sandeen]

On 10/11/15 5:26 PM, Dave Chinner wrote:
> On Thu, Oct 08, 2015 at 07:25:24PM -0500, Eric Sandeen wrote:
>> This fixes some unaligned accesses spotted by libubsan in repair.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>  repair/dinode.c   |   19 +++++++++----------
>>  repair/prefetch.c |    4 ++--
>>  2 files changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/repair/dinode.c b/repair/dinode.c
>> index f78f907..44bbb8f 100644
>> --- a/repair/dinode.c
>> +++ b/repair/dinode.c
>> @@ -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?

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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/4] xfs_repair: fix unaligned accesses

[Dave Chinner]

On Mon, Oct 12, 2015 at 04:31:32PM -0500, Eric Sandeen wrote:
> 
> 
> On 10/11/15 5:26 PM, Dave Chinner wrote:
> > On Thu, Oct 08, 2015 at 07:25:24PM -0500, Eric Sandeen wrote:
> >> This fixes some unaligned accesses spotted by libubsan in repair.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>  repair/dinode.c   |   19 +++++++++----------
> >>  repair/prefetch.c |    4 ++--
> >>  2 files changed, 11 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/repair/dinode.c b/repair/dinode.c
> >> index f78f907..44bbb8f 100644
> >> --- a/repair/dinode.c
> >> +++ b/repair/dinode.c
> >> @@ -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?
> 
> 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

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.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/4] xfs_repair: fix unaligned accesses

[Dave Chinner]

On Tue, Oct 13, 2015 at 08:45:32AM +1100, Dave Chinner wrote:
> On Mon, Oct 12, 2015 at 04:31:32PM -0500, Eric Sandeen wrote:
> > 
> > 
> > On 10/11/15 5:26 PM, Dave Chinner wrote:
> > > On Thu, Oct 08, 2015 at 07:25:24PM -0500, Eric Sandeen wrote:
> > >> This fixes some unaligned accesses spotted by libubsan in repair.
> > >>
> > >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > >> ---
> > >>  repair/dinode.c   |   19 +++++++++----------
> > >>  repair/prefetch.c |    4 ++--
> > >>  2 files changed, 11 insertions(+), 12 deletions(-)
> > >>
> > >> diff --git a/repair/dinode.c b/repair/dinode.c
> > >> index f78f907..44bbb8f 100644
> > >> --- a/repair/dinode.c
> > >> +++ b/repair/dinode.c
> > >> @@ -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?
> > 
> > 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
> 
> 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.

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.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs