[PATCH] Btrfs-progs: fix bus error on sparc

[PATCH] Btrfs-progs: fix bus error on sparc

[Liu Bo]

A type punning from (void *) to (u64 *) plus reading its value will cause
bus error on the Linux OS installed on sparc arch, because sparc core will
do a misaligned access and we don't have an option like -misalign or -xmemalign
in gcc.

Instead we introduce a struct to avoid the memory misalignment.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 disk-io.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/disk-io.c b/disk-io.c
index 0af3898..eed7d7e 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -650,10 +650,14 @@ insert:
 	return root;
 }
 
+struct root_obj {
+	u64 objectid;
+};
+
 static int btrfs_fs_roots_compare_objectids(struct rb_node *node,
 					    void *data)
 {
-	u64 objectid = *((u64 *)data);
+	u64 objectid = ((struct root_obj *)data)->objectid;
 	struct btrfs_root *root;
 
 	root = rb_entry(node, struct btrfs_root, rb_node);
@@ -669,9 +673,11 @@ static int btrfs_fs_roots_compare_roots(struct rb_node *node1,
 					struct rb_node *node2)
 {
 	struct btrfs_root *root;
+	struct root_obj obj;
 
 	root = rb_entry(node2, struct btrfs_root, rb_node);
-	return btrfs_fs_roots_compare_objectids(node1, (void *)&root->objectid);
+	obj.objectid = root->objectid;
+	return btrfs_fs_roots_compare_objectids(node1, (void *)&obj);
 }
 
 struct btrfs_root *btrfs_read_fs_root(struct btrfs_fs_info *fs_info,
@@ -680,6 +686,7 @@ struct btrfs_root *btrfs_read_fs_root(struct btrfs_fs_info *fs_info,
 	struct btrfs_root *root;
 	struct rb_node *node;
 	int ret;
+	struct root_obj obj;
 
 	if (location->objectid == BTRFS_ROOT_TREE_OBJECTID)
 		return fs_info->tree_root;
@@ -695,7 +702,8 @@ struct btrfs_root *btrfs_read_fs_root(struct btrfs_fs_info *fs_info,
 	BUG_ON(location->objectid == BTRFS_TREE_RELOC_OBJECTID ||
 	       location->offset != (u64)-1);
 
-	node = rb_search(&fs_info->fs_root_tree, (void *)&location->objectid,
+	obj.objectid = location->objectid;
+	node = rb_search(&fs_info->fs_root_tree, (void *)&obj,
 			 btrfs_fs_roots_compare_objectids, NULL);
 	if (node)
 		return container_of(node, struct btrfs_root, rb_node);
-- 
1.8.1.4

[PATCH] Btrfs-progs: Fix bus error on sparc

[Ivan Jager]

[-- Attachment #1: Type: text/plain, Size: 3233 bytes --]

Hello,

Currently, as of 8cae1840afb3ea44dcc298f32983e577480dfee4 when running
btrfs-convert I get a bus error.

The problem is that struct btrfs_key has __attribute__ ((__packed__))
so it is not aligned. Then, a pointer to it's objectid field is taken,
cast to a  void*, then eventually cast back to a u64* and
dereferenced. The problem is that the dereferenced u64* is not
necessarily aligned (ie, not necessarily a valid u64*), resulting in
undefined behavior.

This patch adds a local u64 variable which would of course be properly
aligned and then uses a pointer to that.

I did not modify the call from btrfs_fs_roots_compare_roots as that
uses struct btrfs_root which is a regular struct and would thus have
it's members correctly aligned to begin with.

After patching this I realized Liu Bo had already written a similar
patch, but I think mine is cleaner, so I'm sending it anyway.

If you like, I could also change the location->objectid references
between my two changes, which would make the patch bigger, but would
make it actually reduce the overall code size slightly.

Feel free to make or request any necessary style changes as I couldn't
find documentation on the coding style for btrfs-tools.

Please CC me as I'm not on the list.

Thanks,
Ivan

PS: Here is the gdb output in case anyone is interested.

ivan@chacoi:~/src/btrfs-progs$ gdb ./btrfs-convert
GNU gdb (GDB) 7.6.1 (Debian 7.6.1-1)
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "sparc-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /home/ivan/src/btrfs-progs/btrfs-convert...done.
(gdb) run ../e4.img
Starting program: /home/ivan/src/btrfs-progs/./btrfs-convert ../e4.img
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/sparc-linux-gnu/libthread_db.so.1".

Program received signal SIGBUS, Bus error.
btrfs_fs_roots_compare_objectids (node=0x7ab00, data=0xffffd1fb) at
disk-io.c:656
656             u64 objectid = *((u64 *)data);
(gdb) bt
#0  btrfs_fs_roots_compare_objectids (node=0x7ab00, data=0xffffd1fb)
at disk-io.c:656
#1  0x0004d964 in rb_search (root=<optimized out>, key=0xffffd1fb,
    comp=0x1ae24 <btrfs_fs_roots_compare_objectids>, next_ret=0x0) at
rbtree.c:425
#2  0x0001e048 in btrfs_read_fs_root (fs_info=0x78be0,
location=0xffffd1fb) at disk-io.c:698
#3  0x00046244 in create_subvol (trans=0x7adc8, root=0x7a8e0,
root_objectid=256) at btrfs-convert.c:1583
#4  0x00048eb8 in init_btrfs (root=0x7a8e0) at btrfs-convert.c:1631
#5  do_convert (devname=0xffffd935 "../e4.img", datacsum=1, packing=1,
noxattr=0) at btrfs-convert.c:2275
#6  0x0004cfcc in main (argc=1, argv=0xffffd814) at btrfs-convert.c:2743
(gdb) frame 3
#3  0x00046244 in create_subvol (trans=0x7adc8, root=0x7a8e0,
root_objectid=256) at btrfs-convert.c:1583
1583            new_root = btrfs_read_fs_root(root->fs_info, &key);
(gdb) print &key
$1 = (struct btrfs_key *) 0xffffd1fb

[-- Attachment #2: btrfs-tools-patch-sparc.diff --]
[-- Type: text/plain, Size: 811 bytes --]

diff --git a/disk-io.c b/disk-io.c
index 0af3898..a995b52 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -680,6 +680,7 @@ struct btrfs_root *btrfs_read_fs_root(struct btrfs_fs_info *fs_info,
 	struct btrfs_root *root;
 	struct rb_node *node;
 	int ret;
+	u64 objectid = location->objectid;
 
 	if (location->objectid == BTRFS_ROOT_TREE_OBJECTID)
 		return fs_info->tree_root;
@@ -695,7 +696,7 @@ struct btrfs_root *btrfs_read_fs_root(struct btrfs_fs_info *fs_info,
 	BUG_ON(location->objectid == BTRFS_TREE_RELOC_OBJECTID ||
 	       location->offset != (u64)-1);
 
-	node = rb_search(&fs_info->fs_root_tree, (void *)&location->objectid,
+	node = rb_search(&fs_info->fs_root_tree, (void *)&objectid,
 			 btrfs_fs_roots_compare_objectids, NULL);
 	if (node)
 		return container_of(node, struct btrfs_root, rb_node);

Re: [PATCH] Btrfs-progs: Fix bus error on sparc

[David Sterba]

On Thu, Jan 16, 2014 at 09:22:57PM -0500, Ivan Jager wrote:
> After patching this I realized Liu Bo had already written a similar
> patch, but I think mine is cleaner, so I'm sending it anyway.

Thanks for taking the time, I like your version better and will replace
Liu Bo's patch in integration branch.

> If you like, I could also change the location->objectid references
> between my two changes, which would make the patch bigger, but would
> make it actually reduce the overall code size slightly.

Not necessary.

> Feel free to make or request any necessary style changes as I couldn't
> find documentation on the coding style for btrfs-tools.

I'ts the same as the kernel coding style, code and people are mostly the
shared.

> PS: Here is the gdb output in case anyone is interested.

It helps to verify how the unaligned access propagated, thanks.

I'll put your analysis as a changelog and the missing Signed-off-by
line from your name + email.
It's the "Developer's Certificate of Origin 1.1", see
http://lxr.free-electrons.com/source/Documentation/SubmittingPatches#L307
if you'r not familiar with this practice.


david

Re: [PATCH] Btrfs-progs: Fix bus error on sparc

[Ivan Jager]

On Fri, Jan 17, 2014 at 8:58 AM, David Sterba <dsterba@suse.cz> wrote:
> I'll put your analysis as a changelog and the missing Signed-off-by
> line from your name + email.
> It's the "Developer's Certificate of Origin 1.1", see
> http://lxr.free-electrons.com/source/Documentation/SubmittingPatches#L307
> if you'r not familiar with this practice.

Ok, I had seen some of those lines but misunderstood what they meant.
I thought it was code review related but in a different way.

Ivan