[PATCH 0/6] RFC: (partially) endian-annotate e2fsprogs

[PATCH 0/6] RFC: (partially) endian-annotate e2fsprogs

[Eric Sandeen]

This is really only partial, and in the end didn't spot any
actual problems.  And things are a bit odd and tricky, because
some structures (superblocks, inodes, etc) are swapped in-place
in the same structure (so they can't be easily annotated - 
if we wish to, we should define separate on-disk and in-memory
structures).

Further, i_block in the inode is sometimes swapped on read, and
sometimes not (!), depending on whether it's indirect blocks,
extents, or inline data.  So that's still messy too.

So this is really just kind of an RFC; I did it on a whim, and
things aren't yet totally sparse-check clean, but figured I'd send
it out and see what people think, whether it's worth merging,
or working on cleaning up the above issues to make it all tidier.

(sparse is pretty good at looking for casts in and out of blk64_t
too, though I haven't looked much at those.)

Thanks,
-Eric

[PATCH 1/6] e2fsprogs: define bitwise types and annotate conversion routines

[Eric Sandeen]

This lays the groundwork for sparse-checking e2fsprogs for
endianness; defines bitwise types, and fixes up the ext2fs_*
swapping routines to do the proper casts.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 e2fsck/jfs_user.h          |    3 --
 lib/blkid/blkid_types.h.in |   15 +++++++++++++
 lib/ext2fs/bitops.h        |   50 ++++++++++++++++++++++---------------------
 lib/ext2fs/ext2_types.h.in |   15 +++++++++++++
 lib/ext2fs/ext2fs.h        |    6 -----
 5 files changed, 56 insertions(+), 33 deletions(-)

diff --git a/e2fsck/jfs_user.h b/e2fsck/jfs_user.h
index cbc14e8..f05c18c 100644
--- a/e2fsck/jfs_user.h
+++ b/e2fsck/jfs_user.h
@@ -87,9 +87,6 @@ typedef struct {
 
 #define cond_resched()	do { } while (0)
 
-typedef unsigned int __be32;
-typedef __u64 __be64;
-
 #define __init
 
 /*
diff --git a/lib/blkid/blkid_types.h.in b/lib/blkid/blkid_types.h.in
index d4c81d0..2bb3d85 100644
--- a/lib/blkid/blkid_types.h.in
+++ b/lib/blkid/blkid_types.h.in
@@ -164,4 +164,19 @@ typedef long		__s64;
 #undef __S64_TYPEDEF
 #undef __U64_TYPEDEF
 
+#ifdef __CHECKER__
+#define __bitwise		__attribute__((bitwise))
+#define __force			__attribute__((force))
+#else
+#define __bitwise
+#define __force
+#endif
+
+typedef __u16	__bitwise	__le16;
+typedef __u32	__bitwise	__le32;
+typedef __u64	__bitwise	__le64;
+typedef __u16	__bitwise	__be16;
+typedef __u32	__bitwise	__be32;
+typedef __u64	__bitwise	__be64;
+
 #endif /* _*_TYPES_H */
diff --git a/lib/ext2fs/bitops.h b/lib/ext2fs/bitops.h
index 4fb7dc6..bc59608 100644
--- a/lib/ext2fs/bitops.h
+++ b/lib/ext2fs/bitops.h
@@ -11,31 +11,33 @@
  */
 
 #ifdef WORDS_BIGENDIAN
-#define ext2fs_cpu_to_le64(x) ext2fs_swab64((x))
-#define ext2fs_le64_to_cpu(x) ext2fs_swab64((x))
-#define ext2fs_cpu_to_le32(x) ext2fs_swab32((x))
-#define ext2fs_le32_to_cpu(x) ext2fs_swab32((x))
-#define ext2fs_cpu_to_le16(x) ext2fs_swab16((x))
-#define ext2fs_le16_to_cpu(x) ext2fs_swab16((x))
-#define ext2fs_cpu_to_be64(x) ((__u64)(x))
-#define ext2fs_be64_to_cpu(x) ((__u64)(x))
-#define ext2fs_cpu_to_be32(x) ((__u32)(x))
-#define ext2fs_be32_to_cpu(x) ((__u32)(x))
-#define ext2fs_cpu_to_be16(x) ((__u16)(x))
-#define ext2fs_be16_to_cpu(x) ((__u16)(x))
+#define ext2fs_cpu_to_le64(x) ((__force __le64)ext2fs_swab64((__u64)(x)))
+#define ext2fs_le64_to_cpu(x) ext2fs_swab64((__force __u64)(__le64)(x))
+#define ext2fs_cpu_to_le32(x) ((__force __le32)ext2fs_swab32((__u32)(x)))
+#define ext2fs_le32_to_cpu(x) ext2fs_swab32((__force __u32)(__le32)(x))
+#define ext2fs_cpu_to_le16(x) ((__force __le16)ext2fs_swab16((__u16)(x)))
+#define ext2fs_le16_to_cpu(x) ext2fs_swab16((__force __u16)(__le16)(x))
+
+#define ext2fs_cpu_to_be64(x) ((__force __be64)(__u64)(x))
+#define ext2fs_be64_to_cpu(x) ((__force __u64)(__be64)(x))
+#define ext2fs_cpu_to_be32(x) ((__force __be32)(__u32)(x))
+#define ext2fs_be32_to_cpu(x) ((__force __u32)(__be32)(x))
+#define ext2fs_cpu_to_be16(x) ((__force __be16)(__u16)(x))
+#define ext2fs_be16_to_cpu(x) ((__force __u16)(__be16)(x))
 #else
-#define ext2fs_cpu_to_le64(x) ((__u64)(x))
-#define ext2fs_le64_to_cpu(x) ((__u64)(x))
-#define ext2fs_cpu_to_le32(x) ((__u32)(x))
-#define ext2fs_le32_to_cpu(x) ((__u32)(x))
-#define ext2fs_cpu_to_le16(x) ((__u16)(x))
-#define ext2fs_le16_to_cpu(x) ((__u16)(x))
-#define ext2fs_cpu_to_be64(x) ext2fs_swab64((x))
-#define ext2fs_be64_to_cpu(x) ext2fs_swab64((x))
-#define ext2fs_cpu_to_be32(x) ext2fs_swab32((x))
-#define ext2fs_be32_to_cpu(x) ext2fs_swab32((x))
-#define ext2fs_cpu_to_be16(x) ext2fs_swab16((x))
-#define ext2fs_be16_to_cpu(x) ext2fs_swab16((x))
+#define ext2fs_cpu_to_le64(x) ((__force __le64)(__u64)(x))
+#define ext2fs_le64_to_cpu(x) ((__force __u64)(__le64)(x))
+#define ext2fs_cpu_to_le32(x) ((__force __le32)(__u32)(x))
+#define ext2fs_le32_to_cpu(x) ((__force __u32)(__le32)(x))
+#define ext2fs_cpu_to_le16(x) ((__force __le16)(__u16)(x))
+#define ext2fs_le16_to_cpu(x) ((__force __u16)(__le16)(x))
+
+#define ext2fs_cpu_to_be64(x) ((__force __be64)ext2fs_swab64((__u64)(x)))
+#define ext2fs_be64_to_cpu(x) ext2fs_swab64((__force __u64)(__be64)(x))
+#define ext2fs_cpu_to_be32(x) ((__force __be32)ext2fs_swab32((__u32)(x)))
+#define ext2fs_be32_to_cpu(x) ext2fs_swab32((__force __u32)(__be32)(x))
+#define ext2fs_cpu_to_be16(x) ((__force __be16)ext2fs_swab16((__u16)(x)))
+#define ext2fs_be16_to_cpu(x) ext2fs_swab16((__force __u16)(__be16)(x))
 #endif
 
 /*
diff --git a/lib/ext2fs/ext2_types.h.in b/lib/ext2fs/ext2_types.h.in
index 1320431..a00ed7f 100644
--- a/lib/ext2fs/ext2_types.h.in
+++ b/lib/ext2fs/ext2_types.h.in
@@ -164,6 +164,21 @@ typedef long		__s64;
 #undef __S64_TYPEDEF
 #undef __U64_TYPEDEF
 
+#ifdef __CHECKER__
+#define __bitwise		__attribute__((bitwise))
+#define __force			__attribute__((force))
+#else
+#define __bitwise
+#define __force
+#endif
+
+typedef __u16	__bitwise	__le16;
+typedef __u32	__bitwise	__le32;
+typedef __u64	__bitwise	__le64;
+typedef __u16	__bitwise	__be16;
+typedef __u32	__bitwise	__be32;
+typedef __u64	__bitwise	__be64;
+
 #endif /* _*_TYPES_H */
 
 @PUBLIC_CONFIG_HEADER@
diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
index fef6910..506d43b 100644
--- a/lib/ext2fs/ext2fs.h
+++ b/lib/ext2fs/ext2fs.h
@@ -66,12 +66,6 @@ extern "C" {
 #include <ext2fs/ext3_extents.h>
 #endif /* EXT2_FLAT_INCLUDES */
 
-#ifdef __CHECK_ENDIAN__
-#define __bitwise __attribute__((bitwise))
-#else
-#define __bitwise
-#endif

[PATCH 2/6] e2fsprogs: fix endian handling of ext3_extent_header

[Eric Sandeen]

This turned up when trying to resize a filesystem containing
a file with many extents on PPC64.

Fix all locations where ext3_extent_header members aren't
handled in an endian-safe manner.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 lib/ext2fs/ext3_extents.h |   15 ++++++++++-----
 lib/ext2fs/inline_data.c  |    2 +-
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/lib/ext2fs/ext3_extents.h b/lib/ext2fs/ext3_extents.h
index 4163436..a18d705 100644
--- a/lib/ext2fs/ext3_extents.h
+++ b/lib/ext2fs/ext3_extents.h
@@ -106,15 +106,20 @@ struct ext3_ext_path {
 	((struct ext3_extent_idx *) (((char *) (__hdr__)) +	\
 				     sizeof(struct ext3_extent_header)))
 #define EXT_HAS_FREE_INDEX(__path__) \
-	((__path__)->p_hdr->eh_entries < (__path__)->p_hdr->eh_max)
+	(ext2fs_le16_to_cpu((__path__)->p_hdr->eh_entries) < \
+	 ext2fs_le16_to_cpu((__path__)->p_hdr->eh_max))
 #define EXT_LAST_EXTENT(__hdr__) \
-	(EXT_FIRST_EXTENT((__hdr__)) + (__hdr__)->eh_entries - 1)
+	(EXT_FIRST_EXTENT((__hdr__)) + \
+	ext2fs_le16_to_cpu((__hdr__)->eh_entries) - 1)
 #define EXT_LAST_INDEX(__hdr__) \
-	(EXT_FIRST_INDEX((__hdr__)) + (__hdr__)->eh_entries - 1)
+	(EXT_FIRST_INDEX((__hdr__)) + \
+	ext2fs_le16_to_cpu((__hdr__)->eh_entries) - 1)
 #define EXT_MAX_EXTENT(__hdr__) \
-	(EXT_FIRST_EXTENT((__hdr__)) + (__hdr__)->eh_max - 1)
+	(EXT_FIRST_EXTENT((__hdr__)) + \
+	ext2fs_le16_to_cpu((__hdr__)->eh_max) - 1)
 #define EXT_MAX_INDEX(__hdr__) \
-	(EXT_FIRST_INDEX((__hdr__)) + (__hdr__)->eh_max - 1)
+	(EXT_FIRST_INDEX((__hdr__)) + \
+	ext2fs_le16_to_cpu((__hdr__)->eh_max) - 1)
 
 #endif /* _LINUX_EXT3_EXTENTS */
 
diff --git a/lib/ext2fs/inline_data.c b/lib/ext2fs/inline_data.c
index 7eb8b94..8167b76 100644
--- a/lib/ext2fs/inline_data.c
+++ b/lib/ext2fs/inline_data.c
@@ -420,7 +420,7 @@ ext2fs_inline_data_file_expand(ext2_filsys fs, ext2_ino_t ino,
 		eh = (struct ext3_extent_header *) &inode->i_block[0];
 		eh->eh_depth = 0;
 		eh->eh_entries = 0;
-		eh->eh_magic = EXT3_EXT_MAGIC;
+		eh->eh_magic = ext2fs_cpu_to_le16(EXT3_EXT_MAGIC);
 		i = (sizeof(inode->i_block) - sizeof(*eh)) /
 			sizeof(struct ext3_extent);
 		eh->eh_max = ext2fs_cpu_to_le16(i);
-- 1.7.1

Re: [PATCH 2/6] e2fsprogs: fix endian handling of ext3_extent_header

[Darrick J. Wong]

On Thu, Oct 23, 2014 at 04:27:32PM -0500, Eric Sandeen wrote:
> 
> 
> This turned up when trying to resize a filesystem containing
> a file with many extents on PPC64.
> 
> Fix all locations where ext3_extent_header members aren't
> handled in an endian-safe manner.

Looks ok to me (and fixed resize2fs on ${BE}64), so
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  lib/ext2fs/ext3_extents.h |   15 ++++++++++-----
>  lib/ext2fs/inline_data.c  |    2 +-
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/ext2fs/ext3_extents.h b/lib/ext2fs/ext3_extents.h
> index 4163436..a18d705 100644
> --- a/lib/ext2fs/ext3_extents.h
> +++ b/lib/ext2fs/ext3_extents.h
> @@ -106,15 +106,20 @@ struct ext3_ext_path {
>  	((struct ext3_extent_idx *) (((char *) (__hdr__)) +	\
>  				     sizeof(struct ext3_extent_header)))
>  #define EXT_HAS_FREE_INDEX(__path__) \
> -	((__path__)->p_hdr->eh_entries < (__path__)->p_hdr->eh_max)
> +	(ext2fs_le16_to_cpu((__path__)->p_hdr->eh_entries) < \
> +	 ext2fs_le16_to_cpu((__path__)->p_hdr->eh_max))
>  #define EXT_LAST_EXTENT(__hdr__) \
> -	(EXT_FIRST_EXTENT((__hdr__)) + (__hdr__)->eh_entries - 1)
> +	(EXT_FIRST_EXTENT((__hdr__)) + \
> +	ext2fs_le16_to_cpu((__hdr__)->eh_entries) - 1)
>  #define EXT_LAST_INDEX(__hdr__) \
> -	(EXT_FIRST_INDEX((__hdr__)) + (__hdr__)->eh_entries - 1)
> +	(EXT_FIRST_INDEX((__hdr__)) + \
> +	ext2fs_le16_to_cpu((__hdr__)->eh_entries) - 1)
>  #define EXT_MAX_EXTENT(__hdr__) \
> -	(EXT_FIRST_EXTENT((__hdr__)) + (__hdr__)->eh_max - 1)
> +	(EXT_FIRST_EXTENT((__hdr__)) + \
> +	ext2fs_le16_to_cpu((__hdr__)->eh_max) - 1)
>  #define EXT_MAX_INDEX(__hdr__) \
> -	(EXT_FIRST_INDEX((__hdr__)) + (__hdr__)->eh_max - 1)
> +	(EXT_FIRST_INDEX((__hdr__)) + \
> +	ext2fs_le16_to_cpu((__hdr__)->eh_max) - 1)
>  
>  #endif /* _LINUX_EXT3_EXTENTS */
>  
> diff --git a/lib/ext2fs/inline_data.c b/lib/ext2fs/inline_data.c
> index 7eb8b94..8167b76 100644
> --- a/lib/ext2fs/inline_data.c
> +++ b/lib/ext2fs/inline_data.c
> @@ -420,7 +420,7 @@ ext2fs_inline_data_file_expand(ext2_filsys fs, ext2_ino_t ino,
>  		eh = (struct ext3_extent_header *) &inode->i_block[0];
>  		eh->eh_depth = 0;
>  		eh->eh_entries = 0;
> -		eh->eh_magic = EXT3_EXT_MAGIC;
> +		eh->eh_magic = ext2fs_cpu_to_le16(EXT3_EXT_MAGIC);
>  		i = (sizeof(inode->i_block) - sizeof(*eh)) /
>  			sizeof(struct ext3_extent);
>  		eh->eh_max = ext2fs_cpu_to_le16(i);
> -- 1.7.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] e2fsprogs: Endian-annotate most on-disk structures

[Eric Sandeen]

This annotates most on-disk structures for endianness;
however it does not annotate some, like the superblock, inodes,
mmp, etc, as these are swapped in-place at this point.  This is
a little inconsistent, but should help catch some endian mistakes,
at least.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 lib/ext2fs/ext2_fs.h      |   13 +++++++------
 lib/ext2fs/ext3_extents.h |   28 ++++++++++++++--------------
 lib/ext2fs/kernel-jbd.h   |    4 ++--
 lib/quota/quotaio.c       |    4 ++--
 lib/quota/quotaio_tree.h  |   10 +++++-----
 lib/quota/quotaio_v2.h    |   36 ++++++++++++++++++------------------
 6 files changed, 48 insertions(+), 47 deletions(-)

diff --git a/lib/ext2fs/ext2_fs.h b/lib/ext2fs/ext2_fs.h
index f9a4bdb..2b24080 100644
--- a/lib/ext2fs/ext2_fs.h
+++ b/lib/ext2fs/ext2_fs.h
@@ -233,21 +233,21 @@ struct ext2_dx_root_info {
 #define EXT2_HASH_FLAG_INCOMPAT	0x1
 
 struct ext2_dx_entry {
-	__u32 hash;
-	__u32 block;
+	__le32 hash;
+	__le32 block;
 };
 
 struct ext2_dx_countlimit {
-	__u16 limit;
-	__u16 count;
+	__le16 limit;
+	__le16 count;
 };
 
 /*
  * This goes at the end of each htree block.
  */
 struct ext2_dx_tail {
-	__u32 dt_reserved;
-	__u32 dt_checksum;	/* crc32c(uuid+inum+dxblock) */
+	__le32 dt_reserved;
+	__le32 dt_checksum;	/* crc32c(uuid+inum+dxblock) */
 };
 
 /*
@@ -894,6 +894,7 @@ struct ext2_dir_entry_tail {
 #define EXT4_MMP_SEQ_FSCK  0xE24D4D50U /* mmp_seq value when being fscked */
 #define EXT4_MMP_SEQ_MAX   0xE24D4D4FU /* maximum valid mmp_seq value */
 
+/* Not endian-annotated; it's swapped at read/write time */
 struct mmp_struct {
 	__u32	mmp_magic;		/* Magic number for MMP */
 	__u32	mmp_seq;		/* Sequence no. updated periodically */
diff --git a/lib/ext2fs/ext3_extents.h b/lib/ext2fs/ext3_extents.h
index a18d705..66c23fd 100644
--- a/lib/ext2fs/ext3_extents.h
+++ b/lib/ext2fs/ext3_extents.h
@@ -26,7 +26,7 @@
  * crammed into the end of the block without having to rebalance the tree.
  */
 struct ext3_extent_tail {
-	__u32	et_checksum;	/* crc32c(uuid+inum+extent_block) */
+	__le32	et_checksum;	/* crc32c(uuid+inum+extent_block) */
 };
 
 /*
@@ -34,10 +34,10 @@ struct ext3_extent_tail {
  * it's used at the bottom of the tree
  */
 struct ext3_extent {
-	__u32	ee_block;	/* first logical block extent covers */
-	__u16	ee_len;		/* number of blocks covered by extent */
-	__u16	ee_start_hi;	/* high 16 bits of physical block */
-	__u32	ee_start;	/* low 32 bigs of physical block */
+	__le32	ee_block;	/* first logical block extent covers */
+	__le16	ee_len;		/* number of blocks covered by extent */
+	__le16	ee_start_hi;	/* high 16 bits of physical block */
+	__le32	ee_start;	/* low 32 bigs of physical block */
 };
 
 /*
@@ -45,22 +45,22 @@ struct ext3_extent {
  * it's used at all the levels, but the bottom
  */
 struct ext3_extent_idx {
-	__u32	ei_block;	/* index covers logical blocks from 'block' */
-	__u32	ei_leaf;	/* pointer to the physical block of the next *
+	__le32	ei_block;	/* index covers logical blocks from 'block' */
+	__le32	ei_leaf;	/* pointer to the physical block of the next *
 				 * level. leaf or next index could bet here */
-	__u16	ei_leaf_hi;	/* high 16 bits of physical block */
-	__u16	ei_unused;
+	__le16	ei_leaf_hi;	/* high 16 bits of physical block */
+	__le16	ei_unused;
 };
 
 /*
  * each block (leaves and indexes), even inode-stored has header
  */
 struct ext3_extent_header {
-	__u16	eh_magic;	/* probably will support different formats */
-	__u16	eh_entries;	/* number of valid entries */
-	__u16	eh_max;		/* capacity of store in entries */
-	__u16	eh_depth;	/* has tree real underlaying blocks? */
-	__u32	eh_generation;	/* generation of the tree */
+	__le16	eh_magic;	/* probably will support different formats */
+	__le16	eh_entries;	/* number of valid entries */
+	__le16	eh_max;		/* capacity of store in entries */
+	__le16	eh_depth;	/* has tree real underlaying blocks? */
+	__le32	eh_generation;	/* generation of the tree */
 };
 
 #define EXT3_EXT_MAGIC		0xf30a
diff --git a/lib/ext2fs/kernel-jbd.h b/lib/ext2fs/kernel-jbd.h
index 4020429..56efe21 100644
--- a/lib/ext2fs/kernel-jbd.h
+++ b/lib/ext2fs/kernel-jbd.h
@@ -170,7 +170,7 @@ typedef struct journal_block_tag_s
 
 /* Tail of descriptor block, for checksumming */
 struct journal_block_tail {
-	__u32		t_checksum;
+	__be32		t_checksum;
 };
 
 /*
@@ -185,7 +185,7 @@ typedef struct journal_revoke_header_s
 
 /* Tail of revoke block, for checksumming */
 struct journal_revoke_tail {
-	__u32		r_checksum;
+	__be32		r_checksum;
 };
 
 /* Definitions for the journal tag flags word: */
diff --git a/lib/quota/quotaio.c b/lib/quota/quotaio.c
index 65fccaa..c7e5f87 100644
--- a/lib/quota/quotaio.c
+++ b/lib/quota/quotaio.c
@@ -30,8 +30,8 @@ static const char * const basenames[] = {
 
 /* Header in all newer quotafiles */
 struct disk_dqheader {
-	__u32 dqh_magic;
-	__u32 dqh_version;
+	__le32 dqh_magic;
+	__le32 dqh_version;
 } __attribute__ ((packed));
 
 /**
diff --git a/lib/quota/quotaio_tree.h b/lib/quota/quotaio_tree.h
index be34edb..0db0ca1 100644
--- a/lib/quota/quotaio_tree.h
+++ b/lib/quota/quotaio_tree.h
@@ -20,13 +20,13 @@ typedef __u32 qid_t;        /* Type in which we store ids in memory */
  *  so there will be space for exactly 21 quota-entries in a block
  */
 struct qt_disk_dqdbheader {
-	__u32 dqdh_next_free;	/* Number of next block with free
+	__le32 dqdh_next_free;	/* Number of next block with free
 					 * entry */
-	__u32 dqdh_prev_free; /* Number of previous block with free
+	__le32 dqdh_prev_free; /* Number of previous block with free
 				   * entry */
-	__u16 dqdh_entries; /* Number of valid entries in block */
-	__u16 dqdh_pad1;
-	__u32 dqdh_pad2;
+	__le16 dqdh_entries; /* Number of valid entries in block */
+	__le16 dqdh_pad1;
+	__le32 dqdh_pad2;
 } __attribute__ ((packed));
 
 struct dquot;
diff --git a/lib/quota/quotaio_v2.h b/lib/quota/quotaio_v2.h
index 646c698..de2db27 100644
--- a/lib/quota/quotaio_v2.h
+++ b/lib/quota/quotaio_v2.h
@@ -16,8 +16,8 @@
 #define V2_VERSION 1
 
 struct v2_disk_dqheader {
-	__u32 dqh_magic;	/* Magic number identifying file */
-	__u32 dqh_version;	/* File version */
+	__le32 dqh_magic;	/* Magic number identifying file */
+	__le32 dqh_version;	/* File version */
 } __attribute__ ((packed));
 
 /* Flags for version specific files */
@@ -25,30 +25,30 @@ struct v2_disk_dqheader {
 
 /* Header with type and version specific information */
 struct v2_disk_dqinfo {
-	__u32 dqi_bgrace;	/* Time before block soft limit becomes
+	__le32 dqi_bgrace;	/* Time before block soft limit becomes
 				 * hard limit */
-	__u32 dqi_igrace;	/* Time before inode soft limit becomes
+	__le32 dqi_igrace;	/* Time before inode soft limit becomes
 				 * hard limit */
-	__u32 dqi_flags;	/* Flags for quotafile (DQF_*) */
-	__u32 dqi_blocks;	/* Number of blocks in file */
-	__u32 dqi_free_blk;	/* Number of first free block in the list */
-	__u32 dqi_free_entry;	/* Number of block with at least one
+	__le32 dqi_flags;	/* Flags for quotafile (DQF_*) */
+	__le32 dqi_blocks;	/* Number of blocks in file */
+	__le32 dqi_free_blk;	/* Number of first free block in the list */
+	__le32 dqi_free_entry;	/* Number of block with at least one
 					 * free entry */
 } __attribute__ ((packed));
 
 struct v2r1_disk_dqblk {
-	__u32 dqb_id;	/* id this quota applies to */
-	__u32 dqb_pad;
-	__u64 dqb_ihardlimit;	/* absolute limit on allocated inodes */
-	__u64 dqb_isoftlimit;	/* preferred inode limit */
-	__u64 dqb_curinodes;	/* current # allocated inodes */
-	__u64 dqb_bhardlimit;	/* absolute limit on disk space
+	__le32 dqb_id;	/* id this quota applies to */
+	__le32 dqb_pad;
+	__le64 dqb_ihardlimit;	/* absolute limit on allocated inodes */
+	__le64 dqb_isoftlimit;	/* preferred inode limit */
+	__le64 dqb_curinodes;	/* current # allocated inodes */
+	__le64 dqb_bhardlimit;	/* absolute limit on disk space
 					 * (in QUOTABLOCK_SIZE) */
-	__u64 dqb_bsoftlimit;	/* preferred limit on disk space
+	__le64 dqb_bsoftlimit;	/* preferred limit on disk space
 					 * (in QUOTABLOCK_SIZE) */
-	__u64 dqb_curspace;	/* current space occupied (in bytes) */
-	__u64 dqb_btime;	/* time limit for excessive disk use */
-	__u64 dqb_itime;	/* time limit for excessive inode use */
+	__le64 dqb_curspace;	/* current space occupied (in bytes) */
+	__le64 dqb_btime;	/* time limit for excessive disk use */
+	__le64 dqb_itime;	/* time limit for excessive inode use */
 } __attribute__ ((packed));
 
 #endif
-- 1.7.1

[PATCH 4/6] debugfs: don't swap htree nodes in-place

[Eric Sandeen]

htree_dump_int_node() was swapping htree nodes in-place,
something not done elsewhere, so it made the sparse checker
unhappy.  Just use dedicated variables to hold the 2 needed
(endian-swapped) values.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 debugfs/htree.c |   35 ++++++++++++++++++-----------------
 1 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/debugfs/htree.c b/debugfs/htree.c
index 4f0118d..866d1c6 100644
--- a/debugfs/htree.c
+++ b/debugfs/htree.c
@@ -138,21 +138,20 @@ static void htree_dump_int_node(ext2_filsys fs, ext2_ino_t ino,
 				struct ext2_dx_entry *ent,
 				char *buf, int level)
 {
-	struct ext2_dx_countlimit	limit;
-	struct ext2_dx_entry		e;
+	struct ext2_dx_countlimit	dx_countlimit;
 	struct ext2_dx_tail		*tail;
 	int				hash, i;
+	int				limit, count;
 	int				remainder;
 
-	limit = *((struct ext2_dx_countlimit *) ent);
-	limit.count = ext2fs_le16_to_cpu(limit.count);
-	limit.limit = ext2fs_le16_to_cpu(limit.limit);
+	dx_countlimit = *((struct ext2_dx_countlimit *) ent);
+	count = ext2fs_le16_to_cpu(dx_countlimit.count);
+	limit = ext2fs_le16_to_cpu(dx_countlimit.limit);
 
-	fprintf(pager, "Number of entries (count): %d\n", limit.count);
-	fprintf(pager, "Number of entries (limit): %d\n", limit.limit);
+	fprintf(pager, "Number of entries (count): %d\n", count);
+	fprintf(pager, "Number of entries (limit): %d\n", limit);
 
-	remainder = fs->blocksize - (limit.limit *
-				     sizeof(struct ext2_dx_entry));
+	remainder = fs->blocksize - (limit * sizeof(struct ext2_dx_entry));
 	if (ent == (struct ext2_dx_entry *)(rootnode + 1))
 		remainder -= sizeof(struct ext2_dx_root_info) + 24;
 	else
@@ -160,12 +159,12 @@ static void htree_dump_int_node(ext2_filsys fs, ext2_ino_t ino,
 	if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
 				       EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) &&
 	    remainder == sizeof(struct ext2_dx_tail)) {
-		tail = (struct ext2_dx_tail *)(ent + limit.limit);
+		tail = (struct ext2_dx_tail *)(ent + limit);
 		fprintf(pager, "Checksum: 0x%08x\n",
 			ext2fs_le32_to_cpu(tail->dt_checksum));
 	}
 
-	for (i=0; i < limit.count; i++) {
+	for (i=0; i < count; i++) {
 		hash = i ? ext2fs_le32_to_cpu(ent[i].hash) : 0;
 		fprintf(pager, "Entry #%d: Hash 0x%08x%s, block %u\n", i,
 			hash, (hash & 1) ? " (**)" : "",
@@ -174,17 +173,19 @@ static void htree_dump_int_node(ext2_filsys fs, ext2_ino_t ino,
 
 	fprintf(pager, "\n");
 
-	for (i=0; i < limit.count; i++) {
-		e.hash = ext2fs_le32_to_cpu(ent[i].hash);
-		e.block = ext2fs_le32_to_cpu(ent[i].block);
+	for (i=0; i < count; i++) {
+		int hash, block;
+
+		hash = ext2fs_le32_to_cpu(ent[i].hash);
+		block = ext2fs_le32_to_cpu(ent[i].block);
 		fprintf(pager, "Entry #%d: Hash 0x%08x, block %u\n", i,
-		       i ? e.hash : 0, e.block);
+		       i ? hash : 0, block);
 		if (level)
 			htree_dump_int_block(fs, ino, inode, rootnode,
-					     e.block, buf, level-1);
+					     block, buf, level-1);
 		else
 			htree_dump_leaf_node(fs, ino, inode, rootnode,
-					     e.block, buf);
+					     block, buf);
 	}
 
 	fprintf(pager, "---------------------\n");
-- 1.7.1

[PATCH 5/6] libext2: minor sparse endian checker fixup

[Eric Sandeen]

The sparse checker treats 0 assignments as special, but
doesn't catch a = b = 0; separate them to make it quieter.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 lib/ext2fs/extent.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
index c9ef701..ca5b78b 100644
--- a/lib/ext2fs/extent.c
+++ b/lib/ext2fs/extent.c
@@ -1651,8 +1651,10 @@ errcode_t ext2fs_extent_delete(ext2_extent_handle_t handle, int flags)
 	} else {
 		eh = (struct ext3_extent_header *) path->buf;
 		eh->eh_entries = ext2fs_cpu_to_le16(path->entries);
-		if ((path->entries == 0) && (handle->level == 0))
-			eh->eh_depth = handle->max_depth = 0;
+		if ((path->entries == 0) && (handle->level == 0)) {
+			eh->eh_depth = 0;
+			handle->max_depth = 0;
+		}
 		retval = update_path(handle);
 	}
 	return retval;
-- 1.7.1

[PATCH 6/6] quotaio: annotate & fix up for sparse endian checker

[Eric Sandeen]

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 lib/quota/quotaio_tree.c |   10 +++++-----
 lib/quota/quotaio_v2.c   |    7 ++++---
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/lib/quota/quotaio_tree.c b/lib/quota/quotaio_tree.c
index 4d7a9ab..e7f3e95 100644
--- a/lib/quota/quotaio_tree.c
+++ b/lib/quota/quotaio_tree.c
@@ -254,7 +254,7 @@ static int do_insert_tree(struct quota_handle *h, struct dquot *dquot,
 {
 	dqbuf_t buf;
 	int newson = 0, newact = 0;
-	__u32 *ref;
+	__le32 *ref;
 	unsigned int newblk;
 	int ret = 0;
 
@@ -274,7 +274,7 @@ static int do_insert_tree(struct quota_handle *h, struct dquot *dquot,
 		read_blk(h, *treeblk, buf);
 	}
 
-	ref = (__u32 *) buf;
+	ref = (__le32 *) buf;
 	newblk = ext2fs_le32_to_cpu(ref[get_index(dquot->dq_id, depth)]);
 	if (!newblk)
 		newson = 1;
@@ -397,7 +397,7 @@ static void remove_tree(struct quota_handle *h, struct dquot *dquot,
 {
 	dqbuf_t buf = getdqbuf();
 	unsigned int newblk;
-	__u32 *ref = (__u32 *) buf;
+	__le32 *ref = (__le32 *) buf;
 
 	if (!buf)
 		return;
@@ -473,7 +473,7 @@ static ext2_loff_t find_tree_dqentry(struct quota_handle *h,
 {
 	dqbuf_t buf = getdqbuf();
 	ext2_loff_t ret = 0;
-	__u32 *ref = (__u32 *) buf;
+	__le32 *ref = (__le32 *) buf;
 
 	if (!buf)
 		return -ENOMEM;
@@ -597,7 +597,7 @@ static int report_tree(struct dquot *dquot, unsigned int blk, int depth,
 {
 	int entries = 0, i;
 	dqbuf_t buf = getdqbuf();
-	__u32 *ref = (__u32 *) buf;
+	__le32 *ref = (__le32 *) buf;
 
 	if (!buf)
 		return 0;
diff --git a/lib/quota/quotaio_v2.c b/lib/quota/quotaio_v2.c
index e7bf29c..504b3ea 100644
--- a/lib/quota/quotaio_v2.c
+++ b/lib/quota/quotaio_v2.c
@@ -151,6 +151,7 @@ static int v2_check_file(struct quota_handle *h, int type, int fmt)
 {
 	struct v2_disk_dqheader dqh;
 	int file_magics[] = INITQMAGICS;
+	int be_magic;
 
 	if (fmt != QFMT_VFS_V1)
 		return 0;
@@ -158,9 +159,9 @@ static int v2_check_file(struct quota_handle *h, int type, int fmt)
 	if (!v2_read_header(h, &dqh))
 		return 0;
 
-	if (ext2fs_le32_to_cpu(dqh.dqh_magic) != file_magics[type]) {
-		if (ext2fs_be32_to_cpu(dqh.dqh_magic) == file_magics[type])
-			log_err("Your quota file is stored in wrong endianity");
+	be_magic = ext2fs_be32_to_cpu((__force __be32)dqh.dqh_magic);
+	if (be_magic == file_magics[type]) {
+		log_err("Your quota file is stored in wrong endianity");
 		return 0;
 	}
 	if (V2_VERSION != ext2fs_le32_to_cpu(dqh.dqh_version))
-- 1.7.1

Re: [PATCH 0/6] RFC: (partially) endian-annotate e2fsprogs

[Andreas Dilger]

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

On Oct 23, 2014, at 3:26 PM, Eric Sandeen <sandeen@redhat.com> wrote:
> This is really only partial, and in the end didn't spot any
> actual problems.  And things are a bit odd and tricky, because
> some structures (superblocks, inodes, etc) are swapped in-place
> in the same structure (so they can't be easily annotated - 
> if we wish to, we should define separate on-disk and in-memory
> structures).

Yeah, and this was a source of bugs in the past...  I wonder if
it would make sense to declare a different inode struct for use
in case EXT4_EXTENTS_FL is set so that it can declare i_block
in terms of extent structures?  Sadly, I recall that ext3_extent
and ext3_extent_idx do not have their fields aligned in the same
way, which makes swabbing sad...

> Further, i_block in the inode is sometimes swapped on read, and
> sometimes not (!), depending on whether it's indirect blocks,
> extents, or inline data.  So that's still messy too.
> 
> So this is really just kind of an RFC; I did it on a whim, and
> things aren't yet totally sparse-check clean, but figured I'd send
> it out and see what people think, whether it's worth merging,
> or working on cleaning up the above issues to make it all tidier.

I definitely think this is worthwhile.  I'd guess most people
contributing to e2fsprogs are already used to this from the kernel,
so it won't slow them down, and we almost certainly don't get
enough big-endian testing and this is at least a good way to catch
likely problems.

> (sparse is pretty good at looking for casts in and out of blk64_t
> too, though I haven't looked much at those.)
> 
> Thanks,
> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/6] RFC: (partially) endian-annotate e2fsprogs

[Theodore Ts'o]

On Thu, Oct 23, 2014 at 04:26:46PM -0500, Eric Sandeen wrote:
> This is really only partial, and in the end didn't spot any
> actual problems.  And things are a bit odd and tricky, because
> some structures (superblocks, inodes, etc) are swapped in-place
> in the same structure (so they can't be easily annotated - 
> if we wish to, we should define separate on-disk and in-memory
> structures).
> 
> Further, i_block in the inode is sometimes swapped on read, and
> sometimes not (!), depending on whether it's indirect blocks,
> extents, or inline data.  So that's still messy too.
> 
> So this is really just kind of an RFC; I did it on a whim, and
> things aren't yet totally sparse-check clean, but figured I'd send
> it out and see what people think, whether it's worth merging,
> or working on cleaning up the above issues to make it all tidier.
> 
> (sparse is pretty good at looking for casts in and out of blk64_t
> too, though I haven't looked much at those.)

I've applied all of these patches, thanks.  I'm not sure how much we
can clean up some of the rest of the bits without breaking the library
ABI, and we're not all that sparse-clean to start, so I think it's
worth merging now.  We can always do more clean ups (both with sparse,
or gcc-wall, clang, etc.) as people have time.

Thanks!!

						- Ted

Re: [PATCH 0/6] RFC: (partially) endian-annotate e2fsprogs

[Darrick J. Wong]

On Tue, Nov 04, 2014 at 11:34:07AM -0500, Theodore Ts'o wrote:
> On Thu, Oct 23, 2014 at 04:26:46PM -0500, Eric Sandeen wrote:
> > This is really only partial, and in the end didn't spot any
> > actual problems.  And things are a bit odd and tricky, because
> > some structures (superblocks, inodes, etc) are swapped in-place
> > in the same structure (so they can't be easily annotated - 
> > if we wish to, we should define separate on-disk and in-memory
> > structures).
> > 
> > Further, i_block in the inode is sometimes swapped on read, and
> > sometimes not (!), depending on whether it's indirect blocks,
> > extents, or inline data.  So that's still messy too.
> > 
> > So this is really just kind of an RFC; I did it on a whim, and
> > things aren't yet totally sparse-check clean, but figured I'd send
> > it out and see what people think, whether it's worth merging,
> > or working on cleaning up the above issues to make it all tidier.
> > 
> > (sparse is pretty good at looking for casts in and out of blk64_t
> > too, though I haven't looked much at those.)
> 
> I've applied all of these patches, thanks.  I'm not sure how much we
> can clean up some of the rest of the bits without breaking the library
> ABI, and we're not all that sparse-clean to start, so I think it's
> worth merging now.  We can always do more clean ups (both with sparse,
> or gcc-wall, clang, etc.) as people have time.

Uh... it breaks my ppc64/arm64 cross builds, though moving the __CHECKER__ and
typedef bits out of that horrible three-way #if fixes it.  Will send patch.

--D

> 
> Thanks!!
> 
> 						- Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html