| Message ID | CAB5sJHwdf+MmNPfFrrmX7HYnmTjKXY-Wju2db_0i3VQ7A+eMag@mail.gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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