diff mbox

fix unaligned memory accesses (Closes: #656955)

Message ID 1341625062-13044-1-git-send-email-shawnlandden@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shawn Landden July 7, 2012, 1:37 a.m. UTC
From: Shawn Landen <shawnlandden@gmail.com>

Fix creation of volumes using mkfs.btrfs on armv5.

Signed-off-by: Shawn Landen <shawnlandden@gmail.com>
---
 ctree.h   |   26 ++++++++++++++++++++------
 volumes.c |    5 +++--
 2 files changed, 23 insertions(+), 8 deletions(-)

Comments

Alexander Block July 7, 2012, 10:48 a.m. UTC | #1
On Sat, Jul 7, 2012 at 3:37 AM, Shawn Landden <shawnlandden@gmail.com> wrote:
> From: Shawn Landen <shawnlandden@gmail.com>
>
> Fix creation of volumes using mkfs.btrfs on armv5.
>
> Signed-off-by: Shawn Landen <shawnlandden@gmail.com>
> ---
>  ctree.h   |   26 ++++++++++++++++++++------
>  volumes.c |    5 +++--
>  2 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/ctree.h b/ctree.h
> index 6545c50..ef3f0cc 100644
> --- a/ctree.h
> +++ b/ctree.h
> @@ -19,6 +19,8 @@
>  #ifndef __BTRFS__
>  #define __BTRFS__
>
> +#include <stdint.h>
> +
>  #include "list.h"
>  #include "kerncompat.h"
>  #include "radix-tree.h"
> @@ -970,13 +972,17 @@ struct btrfs_root {
>  static inline u##bits btrfs_##name(struct extent_buffer *eb)           \
>  {                                                                      \
>         struct btrfs_header *h = (struct btrfs_header *)eb->data;       \
> -       return le##bits##_to_cpu(h->member);                            \
> +       uint##bits##_t t;                                               \
> +       memcpy(&t, &h->member, sizeof(h->member));                      \
> +       return le##bits##_to_cpu(t);                                    \
>  }                                                                      \
>  static inline void btrfs_set_##name(struct extent_buffer *eb,          \
>                                     u##bits val)                        \
>  {                                                                      \
>         struct btrfs_header *h = (struct btrfs_header *)eb->data;       \
> -       h->member = cpu_to_le##bits(val);                               \
> +       uint##bits##_t t;                                               \
> +       t = cpu_to_le##bits(val);                                       \
> +       memcpy(&h->member, &t, sizeof(h->member));                      \
>  }
>
>  #define BTRFS_SETGET_FUNCS(name, type, member, bits)                   \
> @@ -984,25 +990,33 @@ static inline u##bits btrfs_##name(struct extent_buffer *eb,              \
>                                    type *s)                             \
>  {                                                                      \
>         unsigned long offset = (unsigned long)s;                        \
> +       uint##bits##_t t;                                               \
>         type *p = (type *) (eb->data + offset);                         \
> -       return le##bits##_to_cpu(p->member);                            \
> +       memcpy(&t, &p->member, sizeof(p->member));                      \
> +       return le##bits##_to_cpu(t);                                    \
>  }                                                                      \
>  static inline void btrfs_set_##name(struct extent_buffer *eb,          \
>                                     type *s, u##bits val)               \
>  {                                                                      \
>         unsigned long offset = (unsigned long)s;                        \
> +       uint##bits##_t t;                                               \
>         type *p = (type *) (eb->data + offset);                         \
> -       p->member = cpu_to_le##bits(val);                               \
> +       t = cpu_to_le##bits(val);                                       \
> +       memcpy(&p->member, &t, sizeof(p->member));                      \
>  }
>
>  #define BTRFS_SETGET_STACK_FUNCS(name, type, member, bits)             \
>  static inline u##bits btrfs_##name(type *s)                            \
>  {                                                                      \
> -       return le##bits##_to_cpu(s->member);                            \
> +       uint##bits##_t t;                                               \
> +       memcpy(&t, &s->member, sizeof(s->member));                      \
> +       return le##bits##_to_cpu(t);                                    \
>  }                                                                      \
>  static inline void btrfs_set_##name(type *s, u##bits val)              \
>  {                                                                      \
> -       s->member = cpu_to_le##bits(val);                               \
> +       uint##bits##_t t;                                               \
> +       t = cpu_to_le##bits(val);                                       \
> +       memcpy(&s->member, &t, sizeof(s->member));                      \
>  }
>
Were these setter/getters really the problem? They all access the
members through the struct pointers and the structs are packed, so the
compiler knows that we do unaligned access. Doesn't gcc handle that
already correctly on ARM?
>  BTRFS_SETGET_FUNCS(device_type, struct btrfs_dev_item, type, 64);
> diff --git a/volumes.c b/volumes.c
> index 8dca5e1..0401eeb 100644
> --- a/volumes.c
> +++ b/volumes.c
> @@ -425,10 +425,11 @@ static int find_next_chunk(struct btrfs_root *root, u64 objectid, u64 *offset)
>                 if (found_key.objectid != objectid)
>                         *offset = 0;
>                 else {
> +                       u64 t;
>                         chunk = btrfs_item_ptr(path->nodes[0], path->slots[0],
>                                                struct btrfs_chunk);
> -                       *offset = found_key.offset +
> -                               btrfs_chunk_length(path->nodes[0], chunk);
> +                       t = found_key.offset + btrfs_chunk_length(path->nodes[0], chunk);
> +                       memcpy(offset, &t, sizeof(found_key.offset));
This however is really needed. As far as I remember, we give this
function pointers to struct members, so the compiler loses the
information that it's from a packed/unaligned struct.
>                 }
>         }
>         ret = 0;
> --
> 1.7.10.4
>
> --
> 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
--
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
Chris Mason July 9, 2012, 6:16 p.m. UTC | #2
On Fri, Jul 06, 2012 at 07:37:42PM -0600, Shawn Landden wrote:
> From: Shawn Landen <shawnlandden@gmail.com>
> 
> Fix creation of volumes using mkfs.btrfs on armv5.

[ use memcpy instead of direct structure accesses ]

Thanks for the patch.  This should be a problem in the way gcc is run.
The kernel uses the same functions without memcpy, so there must be a
way to get gcc to turn the unaligned access into something arch safe.

-chris
--
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 mbox

Patch

diff --git a/ctree.h b/ctree.h
index 6545c50..ef3f0cc 100644
--- a/ctree.h
+++ b/ctree.h
@@ -19,6 +19,8 @@ 
 #ifndef __BTRFS__
 #define __BTRFS__
 
+#include <stdint.h>
+
 #include "list.h"
 #include "kerncompat.h"
 #include "radix-tree.h"
@@ -970,13 +972,17 @@  struct btrfs_root {
 static inline u##bits btrfs_##name(struct extent_buffer *eb)		\
 {									\
 	struct btrfs_header *h = (struct btrfs_header *)eb->data;	\
-	return le##bits##_to_cpu(h->member);				\
+	uint##bits##_t t;						\
+	memcpy(&t, &h->member, sizeof(h->member));			\
+	return le##bits##_to_cpu(t);					\
 }									\
 static inline void btrfs_set_##name(struct extent_buffer *eb,		\
 				    u##bits val)			\
 {									\
 	struct btrfs_header *h = (struct btrfs_header *)eb->data;	\
-	h->member = cpu_to_le##bits(val);				\
+	uint##bits##_t t;						\
+	t = cpu_to_le##bits(val);					\
+	memcpy(&h->member, &t, sizeof(h->member));			\
 }
 
 #define BTRFS_SETGET_FUNCS(name, type, member, bits)			\
@@ -984,25 +990,33 @@  static inline u##bits btrfs_##name(struct extent_buffer *eb,		\
 				   type *s)				\
 {									\
 	unsigned long offset = (unsigned long)s;			\
+	uint##bits##_t t;						\
 	type *p = (type *) (eb->data + offset);				\
-	return le##bits##_to_cpu(p->member);				\
+	memcpy(&t, &p->member, sizeof(p->member));			\
+	return le##bits##_to_cpu(t);					\
 }									\
 static inline void btrfs_set_##name(struct extent_buffer *eb,		\
 				    type *s, u##bits val)		\
 {									\
 	unsigned long offset = (unsigned long)s;			\
+	uint##bits##_t t;						\
 	type *p = (type *) (eb->data + offset);				\
-	p->member = cpu_to_le##bits(val);				\
+	t = cpu_to_le##bits(val);					\
+	memcpy(&p->member, &t, sizeof(p->member));			\
 }
 
 #define BTRFS_SETGET_STACK_FUNCS(name, type, member, bits)		\
 static inline u##bits btrfs_##name(type *s)				\
 {									\
-	return le##bits##_to_cpu(s->member);				\
+	uint##bits##_t t;						\
+	memcpy(&t, &s->member, sizeof(s->member));			\
+	return le##bits##_to_cpu(t);					\
 }									\
 static inline void btrfs_set_##name(type *s, u##bits val)		\
 {									\
-	s->member = cpu_to_le##bits(val);				\
+	uint##bits##_t t;						\
+	t = cpu_to_le##bits(val);					\
+	memcpy(&s->member, &t, sizeof(s->member));			\
 }
 
 BTRFS_SETGET_FUNCS(device_type, struct btrfs_dev_item, type, 64);
diff --git a/volumes.c b/volumes.c
index 8dca5e1..0401eeb 100644
--- a/volumes.c
+++ b/volumes.c
@@ -425,10 +425,11 @@  static int find_next_chunk(struct btrfs_root *root, u64 objectid, u64 *offset)
 		if (found_key.objectid != objectid)
 			*offset = 0;
 		else {
+			u64 t;
 			chunk = btrfs_item_ptr(path->nodes[0], path->slots[0],
 					       struct btrfs_chunk);
-			*offset = found_key.offset +
-				btrfs_chunk_length(path->nodes[0], chunk);
+			t = found_key.offset + btrfs_chunk_length(path->nodes[0], chunk);
+			memcpy(offset, &t, sizeof(found_key.offset));
 		}
 	}
 	ret = 0;