Skip to contentSkip to conversation

Make cramfs little endian only

PATCH Superseded Andi Drebes 5 participants 20 messages Dec 4, 2007 – Dec 6, 2007

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover Make cramfs little endian only

The following patchset makes cramfs little endian only and updates the documentation.

The changes were tested on the following types of machines:
An i386 compatible box (little endian)
UltraSparc IIi (big endian)

#1 lore raw

PATCH 1/2 Make cramfs little endian only

The following patch makes cramfs little endian only. When trying to mount a big endian image, an error message is produced.

The changes were tested on the following types of machines:
An i386 compatible box (little endian)
UltraSparc IIi (big endian)

---
 inode.c |  163 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 136 insertions(+), 27 deletions(-)
fs/cramfs/inode.c +136 -27
index 350680f..3fbf567 100644
@@ -4,6 +4,10 @@
  * Copyright (C) 1999 Linus Torvalds.
  *
  * This file is released under the GPL.
+ *
+ * Changelog:
+ *	11/07 - Andi Drebes <[email protected]>
+ *	Made cramfs little endian only.
  */
 
 /*
@@ -40,6 +44,95 @@ static DEFINE_MUTEX(read_mutex);
 #define CRAMINO(x)	(((x)->offset && (x)->size)?(x)->offset<<2:1)
 #define OFFSET(x)	((x)->i_ino)
 
+#ifdef __BIG_ENDIAN
+/* Converts a cramfs_info from little endian to big endian. */
+static inline void cramfs_convert_info_letobe(struct cramfs_info* info)
+{
+	 info->crc = swab32(info->crc);
+	 info->edition = swab32(info->edition);
+	 info->blocks = swab32(info->blocks);
+	 info->files = swab32(info->files);
+}
+
+/* Converts a cramfs_info from little endian to big endian. */
+static inline void cramfs_convert_inode_letobe(struct cramfs_inode* inode)
+{
+	u8* inode_bytes = (u8*)inode;
+	u8 old_nloffs[4];
+
+	inode->mode = swab16(inode->mode);
+	inode->uid = swab16(inode->uid);
+	inode->size = (inode_bytes[6] << 16) | (inode_bytes[5] << 8) | (inode_bytes[4]);
+
+	/* Save the old values of the namelength and the offset */
+	memcpy(old_nloffs, inode_bytes+8, 4);
+
+	/* Convert the namelength and the offset */
+	inode_bytes[8] = ((old_nloffs[0] & 0x3f) << 2) | ((old_nloffs[3] & 0xc0) >> 6);
+	inode_bytes[9] = ((old_nloffs[3] & 0x3f) << 2) | ((old_nloffs[2] & 0xc0) >> 6);
+	inode_bytes[10] = ((old_nloffs[2] & 0x3f) << 2) | ((old_nloffs[1] & 0xc0) >> 6);
+	inode_bytes[11] = ((old_nloffs[1] & 0x3f) << 2) | ((old_nloffs[0] & 0xc0) >> 6);
+}
+
+/* Converts a cramfs superblock from little endian to big endian. */
+static inline void cramfs_convert_super_letobe(struct cramfs_super* super)
+{
+	super->magic = swab32(super->magic);
+	super->size = swab32(super->size);
+	super->flags = swab32(super->flags);
+	super->future = swab32(super->future);
+	cramfs_convert_info_letobe(&super->fsid);
+	cramfs_convert_inode_letobe(&super->root);
+}
+
+/* Converts a 32 bit integer from little endian to big endian */
+static inline u32 cramfs_convert_u32_letobe(u32 val)
+{
+	return swab32(val);
+}
+
+static inline void cramfs_info_to_host(struct cramfs_info *info)
+{
+	cramfs_convert_info_letobe(info);
+}
+
+static inline void cramfs_inode_to_host(struct cramfs_inode *inode)
+{
+	cramfs_convert_inode_letobe(inode);
+}
+
+static inline void cramfs_super_to_host(struct cramfs_super *super)
+{
+	cramfs_convert_super_letobe(super);
+}
+
+static inline u32 cramfs_u32_to_host(u32 val)
+{
+	return cramfs_convert_u32_letobe(val);
+}
+
+#elif defined(__LITTLE_ENDIAN)
+
+static inline void cramfs_info_to_host(struct cramfs_info *info)
+{
+}
+
+static inline void cramfs_inode_to_host(struct cramfs_inode *inode)
+{
+}
+
+static inline void cramfs_super_to_host(struct cramfs_super *super)
+{
+}
+
+static inline u32 cramfs_u32_to_host(u32 val)
+{
+	return val;
+}
+
+#else
+#error "Neither __BIG_ENDIAN nor __LITTLE_ENDIAN defined."
+#endif
 
 static int cramfs_iget5_test(struct inode *inode, void *opaque)
 {
@@ -253,29 +346,35 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 
 	/* Read the first block and get the superblock from it */
 	memcpy(&super, cramfs_read(sb, 0, sizeof(super)), sizeof(super));
+	cramfs_super_to_host(&super);
 	mutex_unlock(&read_mutex);
 
 	/* Do sanity checks on the superblock */
-	if (super.magic != CRAMFS_MAGIC) {
-		/* check for wrong endianess */
-		if (super.magic == CRAMFS_MAGIC_WEND) {
-			if (!silent)
-				printk(KERN_ERR "cramfs: wrong endianess\n");
-			goto out;
-		}
-
+	if (super.magic != CRAMFS_MAGIC && super.magic != CRAMFS_MAGIC_WEND) {
 		/* check at 512 byte offset */
 		mutex_lock(&read_mutex);
 		memcpy(&super, cramfs_read(sb, 512, sizeof(super)), sizeof(super));
+		cramfs_super_to_host(&super);
 		mutex_unlock(&read_mutex);
-		if (super.magic != CRAMFS_MAGIC) {
-			if (super.magic == CRAMFS_MAGIC_WEND && !silent)
-				printk(KERN_ERR "cramfs: wrong endianess\n");
-			else if (!silent)
+
+		if (super.magic == CRAMFS_MAGIC_WEND) {
+			goto other_endian;
+		}
+		else if (super.magic != CRAMFS_MAGIC) {
+			if (!silent)
 				printk(KERN_ERR "cramfs: wrong magic\n");
+
 			goto out;
 		}
 	}
+	/* check for wrong endianess */
+	else if (super.magic == CRAMFS_MAGIC_WEND) {
+other_endian:
+		if (!silent)
+			printk(KERN_ERR "cramfs: filesystems in big endian format are not supported any longer.\n");
+
+		goto out;
+	}
 
 	/* get feature flags first */
 	if (super.flags & ~CRAMFS_SUPPORTED_FLAGS) {
@@ -367,7 +466,8 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 
 	copied = 0;
 	while (offset < inode->i_size) {
-		struct cramfs_inode *de;
+		void *inode_data;
+		struct cramfs_inode de;
 		unsigned long nextoffset;
 		char *name;
 		ino_t ino;
@@ -375,20 +475,22 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 		int namelen, error;
 
 		mutex_lock(&read_mutex);
-		de = cramfs_read(sb, OFFSET(inode) + offset, sizeof(*de)+CRAMFS_MAXPATHLEN);
-		name = (char *)(de+1);
+		inode_data = cramfs_read(sb, OFFSET(inode) + offset, sizeof(de)+CRAMFS_MAXPATHLEN);
+		memcpy(&de, inode_data, sizeof(de));
+		name = inode_data+sizeof(de);
+		cramfs_inode_to_host(&de);
 
 		/*
 		 * Namelengths on disk are shifted by two
 		 * and the name padded out to 4-byte boundaries
 		 * with zeroes.
 		 */
-		namelen = de->namelen << 2;
+		namelen = de.namelen << 2;
 		memcpy(buf, name, namelen);
-		ino = CRAMINO(de);
-		mode = de->mode;
+		ino = CRAMINO(&de);
+		mode = de.mode;
 		mutex_unlock(&read_mutex);
-		nextoffset = offset + sizeof(*de) + namelen;
+		nextoffset = offset + sizeof(de) + namelen;
 		for (;;) {
 			if (!namelen) {
 				kfree(buf);
@@ -421,19 +523,22 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 	mutex_lock(&read_mutex);
 	sorted = CRAMFS_SB(dir->i_sb)->flags & CRAMFS_FLAG_SORTED_DIRS;
 	while (offset < dir->i_size) {
-		struct cramfs_inode *de;
+		void* inode_data;
+		struct cramfs_inode de;
 		char *name;
 		int namelen, retval;
 
-		de = cramfs_read(dir->i_sb, OFFSET(dir) + offset, sizeof(*de)+CRAMFS_MAXPATHLEN);
-		name = (char *)(de+1);
+		inode_data = cramfs_read(dir->i_sb, OFFSET(dir) + offset, sizeof(de)+CRAMFS_MAXPATHLEN);
+		memcpy(&de, inode_data, sizeof(de));
+		name = (char *)(inode_data+sizeof(de));
+		cramfs_inode_to_host(&de);
 
 		/* Try to take advantage of sorted directories */
 		if (sorted && (dentry->d_name.name[0] < name[0]))
 			break;
 
-		namelen = de->namelen << 2;
-		offset += sizeof(*de) + namelen;
+		namelen = de.namelen << 2;
+		offset += sizeof(de) + namelen;
 
 		/* Quick check that the name is roughly the right length */
 		if (((dentry->d_name.len + 3) & ~3) != namelen)
@@ -454,7 +559,7 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 		if (retval > 0)
 			continue;
 		if (!retval) {
-			struct cramfs_inode entry = *de;
+			struct cramfs_inode entry = de;
 			mutex_unlock(&read_mutex);
 			d_add(dentry, get_cramfs_inode(dir->i_sb, &entry));
 			return NULL;
@@ -483,9 +588,13 @@ static int cramfs_readpage(struct file *file, struct page * page)
 
 		start_offset = OFFSET(inode) + maxblock*4;
 		mutex_lock(&read_mutex);
-		if (page->index)
+		if (page->index) {
 			start_offset = *(u32 *) cramfs_read(sb, blkptr_offset-4, 4);
-		compr_len = (*(u32 *) cramfs_read(sb, blkptr_offset, 4) - start_offset);
+			start_offset = cramfs_u32_to_host(start_offset);
+		}
+
+		compr_len = cramfs_u32_to_host(*(u32 *) cramfs_read(sb, blkptr_offset, 4)) - start_offset;
+
 		mutex_unlock(&read_mutex);
 		pgdata = kmap(page);
 		if (compr_len == 0)
#2 lore raw

fs/cramfs/inode.c static DEFINE_MUTEX(read_mutex); · line 55

Quoted 10 lines · Andi Drebes
fs/cramfs/inode.c +9 -0
+#ifdef __BIG_ENDIAN
+/* Converts a cramfs_info from little endian to big endian. */
+static inline void cramfs_convert_info_letobe(struct cramfs_info* info)
+{
+	 info->crc = swab32(info->crc);
+	 info->edition = swab32(info->edition);
+	 info->blocks = swab32(info->blocks);
+	 info->files = swab32(info->files);
+}

Can you remove the #ifdef and use le32_to_cpu() directly?

Jörn

Signature · 7 lines
Mundie uses a textbook tactic of manipulation: start with some
reasonable talk, and lead the audience to an unreasonable conclusion.
-- Bruce Perens
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#3 lore raw
Quoted 11 lines

Review

Quoted 9 lines
(quoted diff) +9 -0
+#ifdef __BIG_ENDIAN
+/* Converts a cramfs_info from little endian to big endian. */
+static inline void cramfs_convert_info_letobe(struct cramfs_info* info)
+{
+	 info->crc = swab32(info->crc);
+	 info->edition = swab32(info->edition);
+	 info->blocks = swab32(info->blocks);
+	 info->files = swab32(info->files);
+}

Can you remove the #ifdef and use le32_to_cpu() directly?

Sure. This saves some definitions (and lines of code)...
Here's the new patch (tested on the same machines mentioned in the first message). I tried to move as many lines as possible out of the endian dependent section.

---
 fs/cramfs/inode.c |  135 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 108 insertions(+), 27 deletions(-)
fs/cramfs/inode.c +108 -27
index 350680f..ab05fd5 100644
@@ -4,6 +4,10 @@
  * Copyright (C) 1999 Linus Torvalds.
  *
  * This file is released under the GPL.
+ *
+ * Changelog:
+ *	11/07 - Andi Drebes <[email protected]>
+ *	Made cramfs little endian only.
  */
 
 /*
@@ -40,6 +44,67 @@ static DEFINE_MUTEX(read_mutex);
 #define CRAMINO(x)	(((x)->offset && (x)->size)?(x)->offset<<2:1)
 #define OFFSET(x)	((x)->i_ino)
 
+/* Converts a cramfs_info from little endian to host endian. */
+static inline void cramfs_info_to_host(struct cramfs_info* info)
+{
+	 info->crc = le32_to_cpu(info->crc);
+	 info->edition = le32_to_cpu(info->edition);
+	 info->blocks = le32_to_cpu(info->blocks);
+	 info->files = le32_to_cpu(info->files);
+}
+
+/* Converts a 32 bit integer from little endian to host endian */
+static inline u32 cramfs_u32_to_host(u32 val)
+{
+	return le32_to_cpu(val);
+}
+
+#ifdef __BIG_ENDIAN
+/* Converts a cramfs_inode from little endian to big endian. */
+static inline void cramfs_convert_inode_letobe(struct cramfs_inode* inode)
+{
+	u8* inode_bytes = (u8*)inode;
+	u8 old_nloffs[4];
+
+	inode->mode = swab16(inode->mode);
+	inode->uid = swab16(inode->uid);
+	inode->size = (inode_bytes[6] << 16) | (inode_bytes[5] << 8) | (inode_bytes[4]);
+
+	/* Save the old values of the namelength and the offset */
+	memcpy(old_nloffs, inode_bytes+8, 4);
+
+	/* Convert the namelength and the offset */
+	inode_bytes[8] = ((old_nloffs[0] & 0x3f) << 2) | ((old_nloffs[3] & 0xc0) >> 6);
+	inode_bytes[9] = ((old_nloffs[3] & 0x3f) << 2) | ((old_nloffs[2] & 0xc0) >> 6);
+	inode_bytes[10] = ((old_nloffs[2] & 0x3f) << 2) | ((old_nloffs[1] & 0xc0) >> 6);
+	inode_bytes[11] = ((old_nloffs[1] & 0x3f) << 2) | ((old_nloffs[0] & 0xc0) >> 6);
+}
+
+static inline void cramfs_inode_to_host(struct cramfs_inode *inode)
+{
+	cramfs_convert_inode_letobe(inode);
+}
+
+#elif defined(__LITTLE_ENDIAN)
+
+static inline void cramfs_inode_to_host(struct cramfs_inode *inode)
+{
+}
+
+#else
+#error "Neither __BIG_ENDIAN nor __LITTLE_ENDIAN defined."
+#endif
+
+/* Converts a cramfs superblock from little endian to host endian. */
+static inline void cramfs_super_to_host(struct cramfs_super* super)
+{
+	super->magic = le32_to_cpu(super->magic);
+	super->size = le32_to_cpu(super->size);
+	super->flags = le32_to_cpu(super->flags);
+	super->future = le32_to_cpu(super->future);
+	cramfs_info_to_host(&super->fsid);
+	cramfs_inode_to_host(&super->root);
+}
 
 static int cramfs_iget5_test(struct inode *inode, void *opaque)
 {
@@ -253,29 +318,35 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 
 	/* Read the first block and get the superblock from it */
 	memcpy(&super, cramfs_read(sb, 0, sizeof(super)), sizeof(super));
+	cramfs_super_to_host(&super);
 	mutex_unlock(&read_mutex);
 
 	/* Do sanity checks on the superblock */
-	if (super.magic != CRAMFS_MAGIC) {
-		/* check for wrong endianess */
-		if (super.magic == CRAMFS_MAGIC_WEND) {
-			if (!silent)
-				printk(KERN_ERR "cramfs: wrong endianess\n");
-			goto out;
-		}
-
+	if (super.magic != CRAMFS_MAGIC && super.magic != CRAMFS_MAGIC_WEND) {
 		/* check at 512 byte offset */
 		mutex_lock(&read_mutex);
 		memcpy(&super, cramfs_read(sb, 512, sizeof(super)), sizeof(super));
+		cramfs_super_to_host(&super);
 		mutex_unlock(&read_mutex);
-		if (super.magic != CRAMFS_MAGIC) {
-			if (super.magic == CRAMFS_MAGIC_WEND && !silent)
-				printk(KERN_ERR "cramfs: wrong endianess\n");
-			else if (!silent)
+
+		if (super.magic == CRAMFS_MAGIC_WEND) {
+			goto other_endian;
+		}
+		else if (super.magic != CRAMFS_MAGIC) {
+			if (!silent)
 				printk(KERN_ERR "cramfs: wrong magic\n");
+
 			goto out;
 		}
 	}
+	/* check for wrong endianess */
+	else if (super.magic == CRAMFS_MAGIC_WEND) {
+other_endian:
+		if (!silent)
+			printk(KERN_ERR "cramfs: filesystems in big endian format are not supported any longer.\n");
+
+		goto out;
+	}
 
 	/* get feature flags first */
 	if (super.flags & ~CRAMFS_SUPPORTED_FLAGS) {
@@ -367,7 +438,8 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 
 	copied = 0;
 	while (offset < inode->i_size) {
-		struct cramfs_inode *de;
+		void *inode_data;
+		struct cramfs_inode de;
 		unsigned long nextoffset;
 		char *name;
 		ino_t ino;
@@ -375,20 +447,22 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 		int namelen, error;
 
 		mutex_lock(&read_mutex);
-		de = cramfs_read(sb, OFFSET(inode) + offset, sizeof(*de)+CRAMFS_MAXPATHLEN);
-		name = (char *)(de+1);
+		inode_data = cramfs_read(sb, OFFSET(inode) + offset, sizeof(de)+CRAMFS_MAXPATHLEN);
+		memcpy(&de, inode_data, sizeof(de));
+		name = inode_data+sizeof(de);
+		cramfs_inode_to_host(&de);
 
 		/*
 		 * Namelengths on disk are shifted by two
 		 * and the name padded out to 4-byte boundaries
 		 * with zeroes.
 		 */
-		namelen = de->namelen << 2;
+		namelen = de.namelen << 2;
 		memcpy(buf, name, namelen);
-		ino = CRAMINO(de);
-		mode = de->mode;
+		ino = CRAMINO(&de);
+		mode = de.mode;
 		mutex_unlock(&read_mutex);
-		nextoffset = offset + sizeof(*de) + namelen;
+		nextoffset = offset + sizeof(de) + namelen;
 		for (;;) {
 			if (!namelen) {
 				kfree(buf);
@@ -421,19 +495,22 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 	mutex_lock(&read_mutex);
 	sorted = CRAMFS_SB(dir->i_sb)->flags & CRAMFS_FLAG_SORTED_DIRS;
 	while (offset < dir->i_size) {
-		struct cramfs_inode *de;
+		void* inode_data;
+		struct cramfs_inode de;
 		char *name;
 		int namelen, retval;
 
-		de = cramfs_read(dir->i_sb, OFFSET(dir) + offset, sizeof(*de)+CRAMFS_MAXPATHLEN);
-		name = (char *)(de+1);
+		inode_data = cramfs_read(dir->i_sb, OFFSET(dir) + offset, sizeof(de)+CRAMFS_MAXPATHLEN);
+		memcpy(&de, inode_data, sizeof(de));
+		name = (char *)(inode_data+sizeof(de));
+		cramfs_inode_to_host(&de);
 
 		/* Try to take advantage of sorted directories */
 		if (sorted && (dentry->d_name.name[0] < name[0]))
 			break;
 
-		namelen = de->namelen << 2;
-		offset += sizeof(*de) + namelen;
+		namelen = de.namelen << 2;
+		offset += sizeof(de) + namelen;
 
 		/* Quick check that the name is roughly the right length */
 		if (((dentry->d_name.len + 3) & ~3) != namelen)
@@ -454,7 +531,7 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 		if (retval > 0)
 			continue;
 		if (!retval) {
-			struct cramfs_inode entry = *de;
+			struct cramfs_inode entry = de;
 			mutex_unlock(&read_mutex);
 			d_add(dentry, get_cramfs_inode(dir->i_sb, &entry));
 			return NULL;
@@ -483,9 +560,13 @@ static int cramfs_readpage(struct file *file, struct page * page)
 
 		start_offset = OFFSET(inode) + maxblock*4;
 		mutex_lock(&read_mutex);
-		if (page->index)
+		if (page->index) {
 			start_offset = *(u32 *) cramfs_read(sb, blkptr_offset-4, 4);
-		compr_len = (*(u32 *) cramfs_read(sb, blkptr_offset, 4) - start_offset);
+			start_offset = cramfs_u32_to_host(start_offset);
+		}
+
+		compr_len = cramfs_u32_to_host(*(u32 *) cramfs_read(sb, blkptr_offset, 4)) - start_offset;
+
 		mutex_unlock(&read_mutex);
 		pgdata = kmap(page);
 		if (compr_len == 0)
#4 lore raw
Quoted 4 lines · Andi Drebes

Sure. This saves some definitions (and lines of code)...
Here's the new patch (tested on the same machines mentioned in the first message). I tried to move as many lines as possible out of the endian dependent section.

This really is the totally wrong way to do this.

You should *not* convert inodes to CPU-endian mode at all. You should *keep* them in the native mode, and then just use "le32_to_cpu()" when actually using them.

Basically, if you ever have a

	#ifdef __BIG_ENDIAN

or similar in the source code, you're simply doing something wrong.

Btw, sparse can be a big help for things like this, by just marking the actual disk data structures as being the right type (ie "__le32" and friends), and then you can verify that any users will use "le32_to_cpu()" as required, because sparse will warn about bad endianness.

So don't copy things around and change them from one mode to the other. Keep all the data structures in native LE ordering, and only when you really need to use them for some real data do you need to do the conversion.

Now, for example, your code has two different "struct cramfs_inode" that you use: the unconverted and the converted one. That's confusing, but it's also error-prone. If you just have one type - the unconverted little-endian one - you have none of those issues.

Linus

#5 lore raw
Quoted 9 lines
Quoted 3 lines

Sure. This saves some definitions (and lines of code)...
Here's the new patch (tested on the same machines mentioned in the first message). I tried to move as many lines as possible out of the endian dependent section.

This really is the totally wrong way to do this.

You should *not* convert inodes to CPU-endian mode at all. You should *keep* them in the native mode, and then just use "le32_to_cpu()" when actually using them.

OK. I'll prepare a new patch and send it to the list (not today, it's already too late in the evening here).

Quoted 5 lines

Basically, if you ever have a

	#ifdef __BIG_ENDIAN

or similar in the source code, you're simply doing something wrong.

Perhaps I'm missing somehting, but I think for cramfs, unfortunately, there has to be this statement. The bitfields in the cramfs_inode structure cause some problems. You can't simply call le32_to_cpu() on them. Especially the namelength and offset fields are weird. There has to be at least one routine for each kind of machine that converts those values (or not -- depending ont the machine's endianness).

Quoted 4 lines

Btw, sparse can be a big help for things like this, by just marking the actual disk data structures as being the right type (ie "__le32" and friends), and then you can verify that any users will use "le32_to_cpu()" as required, because sparse will warn about bad endianness.

Ok, thanks for your advice. But what about the problems mentioned above?

Andi

#6 lore raw
Quoted 4 lines · Andi Drebes

Perhaps I'm missing somehting, but I think for cramfs, unfortunately, there has to be this statement. The bitfields in the cramfs_inode structure cause some problems.

I agree that bitfields can be painful, but they should likely be just rewritten to be accesses using actual masks and shifts. The thing is, bitfields aren't actually endianness safe *anyway*, in that a compiler may end up using a *different* bit order than the byte order. So you cannot really use bitfields reliably on things like that (although Linux has a notion of a "__[BIG|LITTLE]_ENDIAN_BITFIELD", if you really want to).

Linus

#7 lore raw
Quoted 7 lines · Linus Torvalds
Quoted 3 lines · Andi Drebes

Perhaps I'm missing somehting, but I think for cramfs, unfortunately, there has to be this statement. The bitfields in the cramfs_inode structure cause some problems.

I agree that bitfields can be painful, but they should likely be just rewritten to be accesses using actual masks and shifts. The thing is, bitfields aren't actually endianness safe *anyway*, in that a compiler may end up using a *different* bit order than the byte order. So you cannot really use bitfields reliably on things like that (although Linux has a notion of a "__[BIG|LITTLE]_ENDIAN_BITFIELD", if you really want to).

Bitfields also generate lower-quality assembly than masks&shifts (typically more instructions using additional temporaries to accomplish the same thing), based on my own informal gcc testing.

You would think gcc would be advanced enough to turn bitfield use into masks and shifts under the hood, but for whatever reason that often is not the case in kernel code.

Due to the way they're used, bitfields make more difficult the common code pattern of setting several flags at once:

	(assuming 'foo', 'bar' and 'baz' are bitfields in a struct)
	pdev->foo = 1;
	pdev->bar = 0;
	pdev->baz = 1;

    versus

	flag_foo = (1 << 0);
	flag_bar = (1 << 1);
	flag_baz = (1 << 2);
	...
	pdev->flags = flag_foo | flag_bar;

</pet peeve>

And getting back on topic, I think "pdev->flags = cpu_to_le32(flag1|flag2)" is nicer than dealing with bitfields, when your data structures are fixed-endian.

Jeff

#8 lore raw
Quoted 7 lines · Jeff Garzik

Bitfields also generate lower-quality assembly than masks&shifts (typically more instructions using additional temporaries to accomplish the same thing), based on my own informal gcc testing.

You would think gcc would be advanced enough to turn bitfield use into masks and shifts under the hood, but for whatever reason that often is not the case in kernel code.

OK. I'll modify the cramfs structures and replace the bitfields so that the data can be accessed using masks and shifts. This requires updating the userspace tools aswell. There will be a new patch avaliable soon.

I worked on the copy-and-convert-problem today and created a new patch that avoids the negative things pointed out by Linus earlier. I hope I got that in the right way. However, there are still some functions whose implementations depend on the machine's endianness. I hope that they will be obsolete as soon as the new patch is available.

The changes were tested on the following types of machines:
An i386 compatible box (little endian)
UltraSparc IIi (big endian)

---
 fs/cramfs/inode.c |  131 +++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 96 insertions(+), 35 deletions(-)
fs/cramfs/inode.c +96 -35
index 350680f..ceb2c6e 100644
@@ -4,6 +4,10 @@
  * Copyright (C) 1999 Linus Torvalds.
  *
  * This file is released under the GPL.
+ *
+ * Changelog:
+ *	11/07 - Andi Drebes <[email protected]>
+ *	Made cramfs little endian only.
  */
 
 /*
@@ -37,9 +41,62 @@ static DEFINE_MUTEX(read_mutex);
 
 /* These two macros may change in future, to provide better st_ino
    semantics. */
-#define CRAMINO(x)	(((x)->offset && (x)->size)?(x)->offset<<2:1)
+#define CRAMINO(x)	((cramfs_offset_to_cpu(x) && cramfs_size_to_cpu(x)) ? \
+			 cramfs_offset_to_cpu(x) << 2 : 1)
 #define OFFSET(x)	((x)->i_ino)
 
+#ifdef __BIG_ENDIAN
+
+/* Converts a cramfs_inode's offset field
+   from little endianto cpu endian */
+static u32 cramfs_offset_to_cpu(struct cramfs_inode *inode)
+{
+	u8 *inode_bytes = (u8 *)inode;
+	return ((inode_bytes[8] & 0xc0) >> 6) | (inode_bytes[9] << 2) |
+		(inode_bytes[10] << 10) | (inode_bytes[11] << 18);
+}
+
+/* Converts a cramfs_inode's namelength field
+   from little endian to cpu endian */
+static u32 cramfs_namelen_to_cpu(struct cramfs_inode *inode)
+{
+	u8 *inode_bytes = (u8 *)inode;
+	return inode_bytes[8] & 0x3f;
+}
+
+/* Converts a cramfs_inode's size field
+   from little endian to cpu endian */
+static u32 cramfs_size_to_cpu(struct cramfs_inode *inode)
+{
+	return ((inode->size & 0xff0000) >> 16) | (inode->size & 0x00ff00) |
+		((inode->size & 0x0000ff) << 16);
+}
+
+#elif defined(__LITTLE_ENDIAN)
+
+/* Converts a cramfs_inode's offset field
+   from little endian to cpu endian */
+static u32 cramfs_offset_to_cpu(struct cramfs_inode *inode)
+{
+	return inode->offset;
+}
+
+/* Converts a cramfs_inode's namelength field
+   from little endian to cpu endian */
+static u32 cramfs_namelen_to_cpu(struct cramfs_inode *inode)
+{
+	return inode->namelen;
+}
+
+/* Converts a cramfs_inode's size field
+   from little endian to cpu endian */
+static u32 cramfs_size_to_cpu(struct cramfs_inode *inode)
+{
+	return inode->size;
+}
+#else
+#error "Neither __BIG_ENDIAN nor __LITTLE_ENDIAN defined."
+#endif
 
 static int cramfs_iget5_test(struct inode *inode, void *opaque)
 {
@@ -53,13 +110,13 @@ static int cramfs_iget5_test(struct inode *inode, void *opaque)
 
 	/* all empty directories, char, block, pipe, and sock, share inode #1 */
 
-	if ((inode->i_mode != cramfs_inode->mode) ||
+	if ((inode->i_mode != le16_to_cpu(cramfs_inode->mode)) ||
 	    (inode->i_gid != cramfs_inode->gid) ||
-	    (inode->i_uid != cramfs_inode->uid))
+	    (inode->i_uid != le16_to_cpu(cramfs_inode->uid)))
 		return 0; /* does not match */
 
 	if ((S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) &&
-	    (inode->i_rdev != old_decode_dev(cramfs_inode->size)))
+	    (inode->i_rdev != old_decode_dev(cramfs_size_to_cpu(cramfs_inode))))
 		return 0; /* does not match */
 
 	return 1; /* matches */
@@ -69,10 +126,10 @@ static int cramfs_iget5_set(struct inode *inode, void *opaque)
 {
 	static struct timespec zerotime;
 	struct cramfs_inode *cramfs_inode = opaque;
-	inode->i_mode = cramfs_inode->mode;
-	inode->i_uid = cramfs_inode->uid;
-	inode->i_size = cramfs_inode->size;
-	inode->i_blocks = (cramfs_inode->size - 1) / 512 + 1;
+	inode->i_mode = le16_to_cpu(cramfs_inode->mode);
+	inode->i_uid = le16_to_cpu(cramfs_inode->uid);
+	inode->i_size = cramfs_size_to_cpu(cramfs_inode);
+	inode->i_blocks = (cramfs_size_to_cpu(cramfs_inode) - 1) / 512 + 1;
 	inode->i_gid = cramfs_inode->gid;
 	/* Struct copy intentional */
 	inode->i_mtime = inode->i_atime = inode->i_ctime = zerotime;
@@ -94,7 +151,7 @@ static int cramfs_iget5_set(struct inode *inode, void *opaque)
 		inode->i_size = 0;
 		inode->i_blocks = 0;
 		init_special_inode(inode, inode->i_mode,
-			old_decode_dev(cramfs_inode->size));
+			old_decode_dev(cramfs_size_to_cpu(cramfs_inode)));
 	}
 	return 0;
 }
@@ -256,53 +313,57 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 	mutex_unlock(&read_mutex);
 
 	/* Do sanity checks on the superblock */
-	if (super.magic != CRAMFS_MAGIC) {
-		/* check for wrong endianess */
-		if (super.magic == CRAMFS_MAGIC_WEND) {
-			if (!silent)
-				printk(KERN_ERR "cramfs: wrong endianess\n");
-			goto out;
-		}
-
+	if (le32_to_cpu(super.magic) != CRAMFS_MAGIC &&
+	    le32_to_cpu(super.magic) != CRAMFS_MAGIC_WEND) {
 		/* check at 512 byte offset */
 		mutex_lock(&read_mutex);
 		memcpy(&super, cramfs_read(sb, 512, sizeof(super)), sizeof(super));
 		mutex_unlock(&read_mutex);
-		if (super.magic != CRAMFS_MAGIC) {
-			if (super.magic == CRAMFS_MAGIC_WEND && !silent)
-				printk(KERN_ERR "cramfs: wrong endianess\n");
-			else if (!silent)
+
+		if (le32_to_cpu(super.magic) == CRAMFS_MAGIC_WEND)
+			goto other_endian;
+		else if (le32_to_cpu(super.magic) != CRAMFS_MAGIC) {
+			if (!silent)
 				printk(KERN_ERR "cramfs: wrong magic\n");
+
 			goto out;
 		}
 	}
+	/* check for wrong endianess */
+	else if (le32_to_cpu(super.magic) == CRAMFS_MAGIC_WEND) {
+other_endian:
+		if (!silent)
+			printk(KERN_ERR "cramfs: filesystems in big endian format are not supported any longer.\n");
+
+		goto out;
+	}
 
 	/* get feature flags first */
-	if (super.flags & ~CRAMFS_SUPPORTED_FLAGS) {
+	if (le32_to_cpu(super.flags) & ~CRAMFS_SUPPORTED_FLAGS) {
 		printk(KERN_ERR "cramfs: unsupported filesystem features\n");
 		goto out;
 	}
 
 	/* Check that the root inode is in a sane state */
-	if (!S_ISDIR(super.root.mode)) {
+	if (!S_ISDIR(le16_to_cpu(super.root.mode))) {
 		printk(KERN_ERR "cramfs: root is not a directory\n");
 		goto out;
 	}
-	root_offset = super.root.offset << 2;
+	root_offset = cramfs_offset_to_cpu(&super.root) << 2;
 	if (super.flags & CRAMFS_FLAG_FSID_VERSION_2) {
-		sbi->size=super.size;
-		sbi->blocks=super.fsid.blocks;
-		sbi->files=super.fsid.files;
+		sbi->size = le32_to_cpu(super.size);
+		sbi->blocks = le32_to_cpu(super.fsid.blocks);
+		sbi->files = le32_to_cpu(super.fsid.files);
 	} else {
 		sbi->size=1<<28;
 		sbi->blocks=0;
 		sbi->files=0;
 	}
-	sbi->magic=super.magic;
-	sbi->flags=super.flags;
+	sbi->magic = le32_to_cpu(super.magic);
+	sbi->flags = le32_to_cpu(super.flags);
 	if (root_offset == 0)
 		printk(KERN_INFO "cramfs: empty filesystem");
-	else if (!(super.flags & CRAMFS_FLAG_SHIFTED_ROOT_OFFSET) &&
+	else if (!(le32_to_cpu(super.flags) & CRAMFS_FLAG_SHIFTED_ROOT_OFFSET) &&
 		 ((root_offset != sizeof(struct cramfs_super)) &&
 		  (root_offset != 512 + sizeof(struct cramfs_super))))
 	{
@@ -383,10 +444,10 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 		 * and the name padded out to 4-byte boundaries
 		 * with zeroes.
 		 */
-		namelen = de->namelen << 2;
+		namelen = cramfs_namelen_to_cpu(de) << 2;
 		memcpy(buf, name, namelen);
 		ino = CRAMINO(de);
-		mode = de->mode;
+		mode = le16_to_cpu(de->mode);
 		mutex_unlock(&read_mutex);
 		nextoffset = offset + sizeof(*de) + namelen;
 		for (;;) {
@@ -432,7 +493,7 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 		if (sorted && (dentry->d_name.name[0] < name[0]))
 			break;
 
-		namelen = de->namelen << 2;
+		namelen = cramfs_namelen_to_cpu(de) << 2;
 		offset += sizeof(*de) + namelen;
 
 		/* Quick check that the name is roughly the right length */
@@ -484,8 +545,8 @@ static int cramfs_readpage(struct file *file, struct page * page)
 		start_offset = OFFSET(inode) + maxblock*4;
 		mutex_lock(&read_mutex);
 		if (page->index)
-			start_offset = *(u32 *) cramfs_read(sb, blkptr_offset-4, 4);
-		compr_len = (*(u32 *) cramfs_read(sb, blkptr_offset, 4) - start_offset);
+			start_offset = le32_to_cpu(*(u32 *) cramfs_read(sb, blkptr_offset-4, 4));
+		compr_len = le32_to_cpu(*(u32 *) cramfs_read(sb, blkptr_offset, 4)) - start_offset;
 		mutex_unlock(&read_mutex);
 		pgdata = kmap(page);
 		if (compr_len == 0)
#9 lore raw
Quoted 11 lines · Andi Drebes
fs/cramfs/inode.c +10 -0
+#ifdef __BIG_ENDIAN
+
+/* Converts a cramfs_inode's offset field
+   from little endianto cpu endian */
+static u32 cramfs_offset_to_cpu(struct cramfs_inode *inode)
+{
+	u8 *inode_bytes = (u8 *)inode;
+	return ((inode_bytes[8] & 0xc0) >> 6) | (inode_bytes[9] << 2) |
+		(inode_bytes[10] << 10) | (inode_bytes[11] << 18);
+}

...

fs/cramfs/inode.c static DEFINE_MUTEX(read_mutex); · line 82

Quoted 8 lines
fs/cramfs/inode.c +8 -0
+#elif defined(__LITTLE_ENDIAN)
+
+/* Converts a cramfs_inode's offset field
+   from little endian to cpu endian */
+static u32 cramfs_offset_to_cpu(struct cramfs_inode *inode)
+{
+	return inode->offset;
+}

No, no, what I meant about not having any #ifdef __LITTLE_ENDIAN was to do the code do the same thing *regardless* of endianness. In other words, a simple:

	struct cramfs_inode {
		__le32 mode_uid;	/* CRAMFS_MODE_WIDTH:CRAMFS_UID_WIDTH */
		__le32 size_gid;	/* CRAMFS_SIZE_WIDTH:CRAMFS_GID_WIDTH */
		__le32 namelen_offset;	/* CRAMFS_NAMELEN_WIDTH:CRAMFS_OFFSET_WIDTH */
	};

	#define CRAMFS_UID_MASK ((1ul << CRAMFS_UID_WIDTH)-1)
	#define CRAMFS_GID_MASK ((1ul << CRAMFS_GID_WIDTH)-1)
	#define CRAMFS_NAMELEN_MASK ((1ul << CRAMFS_NAMELEN_WIDTH)-1)

	static inline u32 cramfs_mode(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->mode_uid) >> CRAMFS_UID_WIDTH;
	}

	static inline u32 cramfs_uid(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->mode_uid) & CRAMFS_UID_MASK;
	}

	static inline u32 cramfs_size(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->size_gid) >> CRAMFS_GID_WIDTH;
	}

	static inline u32 cramfs_gid(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->size_gid) & CRAMFS_GID_MASK;
	}

	static inline u32 cramfs_offset(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->namelen_offset) >> CRAMFS_NAMELEN_WIDTH;
	}

	static inline u32 cramfs_namelen(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->namelen_offset) & CRAMFS_NAMELEN_MASK;
	}

See? No #ifdef's required, no different code-paths, and the code generated will be pretty much optimal too.

(No, the above is not tested in any way, shape, or form, and no, I didn't double-check that I actually extracted the right bits, but you get the idea).

Linus

#10 lore raw
Quoted 10 lines · Linus Torvalds

No, no, what I meant about not having any #ifdef __LITTLE_ENDIAN was to do the code do the same thing *regardless* of endianness. In other words, a simple:

	struct cramfs_inode {
		__le32 mode_uid;	/* CRAMFS_MODE_WIDTH:CRAMFS_UID_WIDTH */
		__le32 size_gid;	/* CRAMFS_SIZE_WIDTH:CRAMFS_GID_WIDTH */
		__le32 namelen_offset;	/* CRAMFS_NAMELEN_WIDTH:CRAMFS_OFFSET_WIDTH */
	};

Mode and uid are both 16bit. So maybe we can change the structure:

	struct cramfs_inode {
		__le16 uid;
		__le16 mode;
		__le32 size_gid;	/* CRAMFS_SIZE_WIDTH:CRAMFS_GID_WIDTH */
		__le32 namelen_offset;	/* CRAMFS_NAMELEN_WIDTH:CRAMFS_OFFSET_WIDTH */
	};

That way masks and shifts are only needed for the remaining two shared variables.

Quoted 3 lines

(No, the above is not tested in any way, shape, or form, and no, I didn't double-check that I actually extracted the right bits, but you get the idea).

Same warning applies to my code as well.

Jörn

Signature · 8 lines
Rules of Optimization:
Rule 1: Don't do it.
Rule 2 (for experts only): Don't do it yet.
-- M.A. Jackson
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#11 lore raw
Quoted 8 lines

Mode and uid are both 16bit. So maybe we can change the structure:

	struct cramfs_inode {
		__le16 uid;
		__le16 mode;
		__le32 size_gid;	/* CRAMFS_SIZE_WIDTH:CRAMFS_GID_WIDTH */
		__le32 namelen_offset;	/* CRAMFS_NAMELEN_WIDTH:CRAMFS_OFFSET_WIDTH */
	};

Yep, looks better...

Andi

#12 lore raw
In reply to Linus Torvalds

<snip>

Quoted 14 lines

No, no, what I meant about not having any #ifdef __LITTLE_ENDIAN was to do the code do the same thing *regardless* of endianness. In other words, a simple:

	struct cramfs_inode {
		__le32 mode_uid;	/* CRAMFS_MODE_WIDTH:CRAMFS_UID_WIDTH */
		__le32 size_gid;	/* CRAMFS_SIZE_WIDTH:CRAMFS_GID_WIDTH */
		__le32 namelen_offset;	/* CRAMFS_NAMELEN_WIDTH:CRAMFS_OFFSET_WIDTH */
	};

	#define CRAMFS_UID_MASK ((1ul << CRAMFS_UID_WIDTH)-1)
	#define CRAMFS_GID_MASK ((1ul << CRAMFS_GID_WIDTH)-1)
	#define CRAMFS_NAMELEN_MASK ((1ul << CRAMFS_NAMELEN_WIDTH)-1)

[...]

Quoted 4 lines
	static inline u32 cramfs_offset(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->namelen_offset) >> CRAMFS_NAMELEN_WIDTH;
	}

This requires changing the on-disk-structure (even the current "little endian only" one). The problem is caused by the way GCC (and perhaps other compilers aswell) arranges the 6 bits bits for the namelength and the 26 bits for the offset within the 32 bits. I spent quite some time on figuring out how this is actually done. For little endian machines, the data arranged in the following way:

|o02.o01.n06.n05.n04.n03.n02.n01|o10.o09.o08.o07.o06.o05.o04.o03|
|o18.o17.o16.o15.o14.o13.o12.o11|o26.o25.o24.o23.o22.o21.o20.o19| 

(where oXX means offset bit XX, nXX means namelength bit XX, | marks the beginning / end of a byte and . is a bit separator)

On big endian machines, the data is arranged differently:

|n06.n05.n04.n03.n02.n01.o26.o25|o24.o23.o22.o21.o20.o19.o18.o17|
|o16.o15.o14.o13.o12.o11.o10.o09|o08.o07.o06.o05.o04.o03.o02.o01|

So masking and then shifting *the whole* masked data doesn't solve the problem.

Quoted 7 lines
	static inline u32 cramfs_namelen(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->namelen_offset) & CRAMFS_NAMELEN_MASK;
	}

See? No #ifdef's required, no different code-paths, and the code generated will be pretty much optimal too.

See above.

Quoted 1 line

([...]but you get the idea).

Yes. And I agree with you. But making this consistent for all fields of cramfs structures requires changing the on-disk-structure. This would mean that we don't only break support for big endian images, but also for current little endian images.
However, I'm not totally against this solution. But one should keep in mind what I pointed out above.

Andi

#13 lore raw
Quoted 5 lines · Andi Drebes

[...]

Quoted 4 lines
	static inline u32 cramfs_offset(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->namelen_offset) >> CRAMFS_NAMELEN_WIDTH;
	}
Quoted 8 lines

This requires changing the on-disk-structure (even the current "little endian only" one). The problem is caused by the way GCC (and perhaps other compilers aswell) arranges the 6 bits bits for the namelength and the 26 bits for the offset within the 32 bits. I spent quite some time on figuring out how this is actually done. For little endian machines, the data arranged in the following way:

|o02.o01.n06.n05.n04.n03.n02.n01|o10.o09.o08.o07.o06.o05.o04.o03|
|o18.o17.o16.o15.o14.o13.o12.o11|o26.o25.o24.o23.o22.o21.o20.o19| 

How about shifting and masking _before_ converting to host endianness?

	static inline u32 cramfs_offset(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->namelen_offset >> CRAMFS_NAMELEN_WIDTH);
	}

Jörn

Signature · 7 lines
With a PC, I always felt limited by the software available. On Unix,
I am limited only by my knowledge.
-- Peter J. Schoenster
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#14 lore raw
Quoted 7 lines · Jörn Engel

How about shifting and masking _before_ converting to host endianness?

	static inline u32 cramfs_offset(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->namelen_offset >> CRAMFS_NAMELEN_WIDTH);
	}

Stop this idiocy. The code I sent out was correct. You're just making it worse.

"le32_to_cpu()" is a no-op on little-endian machines. So your change won't make any difference there. But the point is, that if we want the disk layout to be the *same* for both big-endian and little-endian, we need to switch the word as it is loaded from memory, and not do *any* operations on it before we've done that equalization.

This is not something unusual. It's bog-standard procedure for a lot of filesystems. And sparse will help you (and would have complained about your code that tries to shift a little-endian entity before changing it into a CPU-endian one).

Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#15 lore raw
In reply to Andi Drebes
Quoted 2 lines · Andi Drebes

This requires changing the on-disk-structure (even the current "little endian only" one).

That makes no sense.

Your whole patch is about making it little-endian only.

If you do that, then big-endian is screwed. I'm ok with that, and support it.

But then you just make sure that the little-endian bits are the same, and now you're *done*.

Quoted 4 lines

For little endian machines, the data arranged in the following way:

|o02.o01.n06.n05.n04.n03.n02.n01|o10.o09.o08.o07.o06.o05.o04.o03|
|o18.o17.o16.o15.o14.o13.o12.o11|o26.o25.o24.o23.o22.o21.o20.o19| 

That's a singularly confused way or looking at it. The point is, the first byte in a little-endian machine is the lowest bits, so the *correct* way of looking at it is to see the above as one 32-bit word, and then it looks like this:

bit-in-word: 31 .. 6 5 .. 0
bit-in-bitfield o25 .. o0 n5 .. n0

and my code works *perfectly*. When you do

	static inline u32 cramfs_offset(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->namelen_offset) >> CRAMFS_NAMELEN_WIDTH;
	}

you are getting *exactly* the bits "o25..o0" that you want (ie the offset).

Quoted 1 line

So masking and then shifting *the whole* masked data doesn't solve the problem.

Yes it does. Try it.

Linus

#16 lore raw
Quoted 2 lines

But then you just make sure that the little-endian bits are the same, and now you're *done*.

Yes, you're perfectly right. Sorry for not getting it. I hope the next time I will understand faster.

Quoted 6 lines
Quoted 4 lines

For little endian machines, the data arranged in the following way:

|o02.o01.n06.n05.n04.n03.n02.n01|o10.o09.o08.o07.o06.o05.o04.o03|
|o18.o17.o16.o15.o14.o13.o12.o11|o26.o25.o24.o23.o22.o21.o20.o19| 

That's a singularly confused way or looking at it.

Yep. I saw everything with big-endian eyes and confused myself. Shouldn't have happened.

Quoted 1 line

Yes it does. Try it.

I tried it. It does it. Here's the new patch (hopefully the last one for this issue)...

Checked with sparse using C=2 and CF="-D__CHECK_ENDIAN__". Tested on an i386 box.
Tested on an Ultrasparc IIi box.

---
 fs/cramfs/inode.c         |  119 ++++++++++++++++++++++++++++++---------------
 include/linux/cramfs_fs.h |   22 ++++----
 2 files changed, 91 insertions(+), 50 deletions(-)
fs/cramfs/inode.c +80 -39
index 350680f..30dc640 100644
@@ -4,6 +4,10 @@
  * Copyright (C) 1999 Linus Torvalds.
  *
  * This file is released under the GPL.
+ *
+ * Changelog:
+ *	11/07 - Andi Drebes <[email protected]>
+ *	Made cramfs little endian only.
  */
 
 /*
@@ -34,13 +38,46 @@ static const struct address_space_operations cramfs_aops;
 
 static DEFINE_MUTEX(read_mutex);
 
+#define CRAMFS_NAMELEN_MASK ((1ul << CRAMFS_NAMELEN_WIDTH)-1)
+#define CRAMFS_MODE_MASK ((1ul << CRAMFS_MODE_WIDTH)-1)
+#define CRAMFS_SIZE_MASK ((1ul << CRAMFS_SIZE_WIDTH)-1)
+
+static inline u32 cramfs_mode(struct cramfs_inode *inode)
+{
+	return le32_to_cpu(inode->mode_uid) & CRAMFS_MODE_MASK;
+}
+
+static inline u32 cramfs_uid(struct cramfs_inode *inode)
+{
+	return le32_to_cpu(inode->mode_uid) >> CRAMFS_MODE_WIDTH;
+}
+
+static inline u32 cramfs_size(struct cramfs_inode *inode)
+{
+	return le32_to_cpu(inode->size_gid) & CRAMFS_SIZE_MASK;
+}
+
+static inline u32 cramfs_gid(struct cramfs_inode *inode)
+{
+	return le32_to_cpu(inode->size_gid) >> CRAMFS_SIZE_WIDTH;
+}
+
+static inline u32 cramfs_offset(struct cramfs_inode *inode)
+{
+	return le32_to_cpu(inode->namelen_offset) >> CRAMFS_NAMELEN_WIDTH;
+}
+
+static inline u32 cramfs_namelen(struct cramfs_inode *inode)
+{
+	return le32_to_cpu(inode->namelen_offset) & CRAMFS_NAMELEN_MASK;
+}
 
 /* These two macros may change in future, to provide better st_ino
    semantics. */
-#define CRAMINO(x)	(((x)->offset && (x)->size)?(x)->offset<<2:1)
+#define CRAMINO(x)	((cramfs_offset(x) && cramfs_size(x)) ? \
+			 cramfs_offset(x) << 2 : 1)
 #define OFFSET(x)	((x)->i_ino)
 
-
 static int cramfs_iget5_test(struct inode *inode, void *opaque)
 {
 	struct cramfs_inode *cramfs_inode = opaque;
@@ -53,13 +90,13 @@ static int cramfs_iget5_test(struct inode *inode, void *opaque)
 
 	/* all empty directories, char, block, pipe, and sock, share inode #1 */
 
-	if ((inode->i_mode != cramfs_inode->mode) ||
-	    (inode->i_gid != cramfs_inode->gid) ||
-	    (inode->i_uid != cramfs_inode->uid))
+	if ((inode->i_mode != cramfs_mode(cramfs_inode)) ||
+	    (inode->i_gid != cramfs_gid(cramfs_inode)) ||
+	    (inode->i_uid != cramfs_uid(cramfs_inode)))
 		return 0; /* does not match */
 
 	if ((S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) &&
-	    (inode->i_rdev != old_decode_dev(cramfs_inode->size)))
+	    (inode->i_rdev != old_decode_dev(cramfs_size(cramfs_inode))))
 		return 0; /* does not match */
 
 	return 1; /* matches */
@@ -69,11 +106,11 @@ static int cramfs_iget5_set(struct inode *inode, void *opaque)
 {
 	static struct timespec zerotime;
 	struct cramfs_inode *cramfs_inode = opaque;
-	inode->i_mode = cramfs_inode->mode;
-	inode->i_uid = cramfs_inode->uid;
-	inode->i_size = cramfs_inode->size;
-	inode->i_blocks = (cramfs_inode->size - 1) / 512 + 1;
-	inode->i_gid = cramfs_inode->gid;
+	inode->i_mode = cramfs_mode(cramfs_inode);
+	inode->i_uid = cramfs_uid(cramfs_inode);
+	inode->i_size = cramfs_size(cramfs_inode);
+	inode->i_blocks = (cramfs_size(cramfs_inode) - 1) / 512 + 1;
+	inode->i_gid = cramfs_gid(cramfs_inode);
 	/* Struct copy intentional */
 	inode->i_mtime = inode->i_atime = inode->i_ctime = zerotime;
 	inode->i_ino = CRAMINO(cramfs_inode);
@@ -94,7 +131,7 @@ static int cramfs_iget5_set(struct inode *inode, void *opaque)
 		inode->i_size = 0;
 		inode->i_blocks = 0;
 		init_special_inode(inode, inode->i_mode,
-			old_decode_dev(cramfs_inode->size));
+			old_decode_dev(cramfs_size(cramfs_inode)));
 	}
 	return 0;
 }
@@ -256,53 +293,57 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 	mutex_unlock(&read_mutex);
 
 	/* Do sanity checks on the superblock */
-	if (super.magic != CRAMFS_MAGIC) {
-		/* check for wrong endianess */
-		if (super.magic == CRAMFS_MAGIC_WEND) {
-			if (!silent)
-				printk(KERN_ERR "cramfs: wrong endianess\n");
-			goto out;
-		}
-
+	if (le32_to_cpu(super.magic) != CRAMFS_MAGIC &&
+	    le32_to_cpu(super.magic) != CRAMFS_MAGIC_WEND) {
 		/* check at 512 byte offset */
 		mutex_lock(&read_mutex);
 		memcpy(&super, cramfs_read(sb, 512, sizeof(super)), sizeof(super));
 		mutex_unlock(&read_mutex);
-		if (super.magic != CRAMFS_MAGIC) {
-			if (super.magic == CRAMFS_MAGIC_WEND && !silent)
-				printk(KERN_ERR "cramfs: wrong endianess\n");
-			else if (!silent)
+
+		if (le32_to_cpu(super.magic) == CRAMFS_MAGIC_WEND)
+			goto other_endian;
+		else if (le32_to_cpu(super.magic) != CRAMFS_MAGIC) {
+			if (!silent)
 				printk(KERN_ERR "cramfs: wrong magic\n");
+
 			goto out;
 		}
 	}
+	/* check for wrong endianess */
+	else if (le32_to_cpu(super.magic) == CRAMFS_MAGIC_WEND) {
+other_endian:
+		if (!silent)
+			printk(KERN_ERR "cramfs: filesystems in big endian format are not supported any longer.\n");
+
+		goto out;
+	}
 
 	/* get feature flags first */
-	if (super.flags & ~CRAMFS_SUPPORTED_FLAGS) {
+	if (le32_to_cpu(super.flags) & ~CRAMFS_SUPPORTED_FLAGS) {
 		printk(KERN_ERR "cramfs: unsupported filesystem features\n");
 		goto out;
 	}
 
 	/* Check that the root inode is in a sane state */
-	if (!S_ISDIR(super.root.mode)) {
+	if (!S_ISDIR(cramfs_mode(&super.root))) {
 		printk(KERN_ERR "cramfs: root is not a directory\n");
 		goto out;
 	}
-	root_offset = super.root.offset << 2;
-	if (super.flags & CRAMFS_FLAG_FSID_VERSION_2) {
-		sbi->size=super.size;
-		sbi->blocks=super.fsid.blocks;
-		sbi->files=super.fsid.files;
+	root_offset = cramfs_offset(&super.root) << 2;
+	if (__le32_to_cpu(super.flags) & CRAMFS_FLAG_FSID_VERSION_2) {
+		sbi->size = le32_to_cpu(super.size);
+		sbi->blocks = le32_to_cpu(super.fsid.blocks);
+		sbi->files = le32_to_cpu(super.fsid.files);
 	} else {
 		sbi->size=1<<28;
 		sbi->blocks=0;
 		sbi->files=0;
 	}
-	sbi->magic=super.magic;
-	sbi->flags=super.flags;
+	sbi->magic = le32_to_cpu(super.magic);
+	sbi->flags = le32_to_cpu(super.flags);
 	if (root_offset == 0)
 		printk(KERN_INFO "cramfs: empty filesystem");
-	else if (!(super.flags & CRAMFS_FLAG_SHIFTED_ROOT_OFFSET) &&
+	else if (!(le32_to_cpu(super.flags) & CRAMFS_FLAG_SHIFTED_ROOT_OFFSET) &&
 		 ((root_offset != sizeof(struct cramfs_super)) &&
 		  (root_offset != 512 + sizeof(struct cramfs_super))))
 	{
@@ -383,10 +424,10 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 		 * and the name padded out to 4-byte boundaries
 		 * with zeroes.
 		 */
-		namelen = de->namelen << 2;
+		namelen = cramfs_namelen(de) << 2;
 		memcpy(buf, name, namelen);
 		ino = CRAMINO(de);
-		mode = de->mode;
+		mode = cramfs_mode(de);
 		mutex_unlock(&read_mutex);
 		nextoffset = offset + sizeof(*de) + namelen;
 		for (;;) {
@@ -432,7 +473,7 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 		if (sorted && (dentry->d_name.name[0] < name[0]))
 			break;
 
-		namelen = de->namelen << 2;
+		namelen = cramfs_namelen(de) << 2;
 		offset += sizeof(*de) + namelen;
 
 		/* Quick check that the name is roughly the right length */
@@ -484,8 +525,8 @@ static int cramfs_readpage(struct file *file, struct page * page)
 		start_offset = OFFSET(inode) + maxblock*4;
 		mutex_lock(&read_mutex);
 		if (page->index)
-			start_offset = *(u32 *) cramfs_read(sb, blkptr_offset-4, 4);
-		compr_len = (*(u32 *) cramfs_read(sb, blkptr_offset, 4) - start_offset);
+			start_offset = le32_to_cpu(*(__le32 *) cramfs_read(sb, blkptr_offset-4, 4));
+		compr_len = le32_to_cpu(*(__le32 *) cramfs_read(sb, blkptr_offset, 4)) - start_offset;
 		mutex_unlock(&read_mutex);
 		pgdata = kmap(page);
 		if (compr_len == 0)
include/linux/cramfs_fs.h +11 -11
index 3be4e5a..66aba06 100644
@@ -28,9 +28,9 @@
  * Reasonably terse representation of the inode data.
  */
 struct cramfs_inode {
-	__u32 mode:CRAMFS_MODE_WIDTH, uid:CRAMFS_UID_WIDTH;
+	__le32 mode_uid;
 	/* SIZE for device files is i_rdev */
-	__u32 size:CRAMFS_SIZE_WIDTH, gid:CRAMFS_GID_WIDTH;
+	__le32 size_gid;
 	/* NAMELEN is the length of the file name, divided by 4 and
            rounded up.  (cramfs doesn't support hard links.) */
 	/* OFFSET: For symlinks and non-empty regular files, this
@@ -39,24 +39,24 @@ struct cramfs_inode {
 	   see README).  For non-empty directories it is the offset
 	   (divided by 4) of the inode of the first file in that
 	   directory.  For anything else, offset is zero. */
-	__u32 namelen:CRAMFS_NAMELEN_WIDTH, offset:CRAMFS_OFFSET_WIDTH;
+	__le32 namelen_offset;
 };
 
 struct cramfs_info {
-	__u32 crc;
-	__u32 edition;
-	__u32 blocks;
-	__u32 files;
+	__le32 crc;
+	__le32 edition;
+	__le32 blocks;
+	__le32 files;
 };
 
 /*
  * Superblock information at the beginning of the FS.
  */
 struct cramfs_super {
-	__u32 magic;			/* 0x28cd3d45 - random number */
-	__u32 size;			/* length in bytes */
-	__u32 flags;			/* feature flags */
-	__u32 future;			/* reserved for future use */
+	__le32 magic;			/* 0x28cd3d45 - random number */
+	__le32 size;			/* length in bytes */
+	__le32 flags;			/* feature flags */
+	__le32 future;			/* reserved for future use */
 	__u8 signature[16];		/* "Compressed ROMFS" */
 	struct cramfs_info fsid;	/* unique filesystem info */
 	__u8 name[16];			/* user-defined name */
#17 lore raw
In reply to Andi Drebes
Quoted 40 lines · On Tue, 4 Dec 2007 13:01:26 +0100 Andi Drebes…

The following patch makes cramfs little endian only. When trying to mount a big endian image, an error message is produced.

The changes were tested on the following types of machines:
An i386 compatible box (little endian)
UltraSparc IIi (big endian)

---
 inode.c |  163 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 136 insertions(+), 27 deletions(-)
fs/cramfs/inode.c +13 -0
index 350680f..3fbf567 100644
@@ -4,6 +4,10 @@
  * Copyright (C) 1999 Linus Torvalds.
  *
  * This file is released under the GPL.
+ *
+ * Changelog:
+ *	11/07 - Andi Drebes <[email protected]>
+ *	Made cramfs little endian only.
  */
 
 /*
@@ -40,6 +44,95 @@ static DEFINE_MUTEX(read_mutex);
 #define CRAMINO(x)	(((x)->offset && (x)->size)?(x)->offset<<2:1)
 #define OFFSET(x)	((x)->i_ino)
 
+#ifdef __BIG_ENDIAN
+/* Converts a cramfs_info from little endian to big endian. */
+static inline void cramfs_convert_info_letobe(struct cramfs_info* info)
+{
+	 info->crc = swab32(info->crc);
+	 info->edition = swab32(info->edition);
+	 info->blocks = swab32(info->blocks);
+	 info->files = swab32(info->files);
+}

fs/cramfs/inode.c static DEFINE_MUTEX(read_mutex); · line 65

Quoted 10 lines
fs/cramfs/inode.c +10 -0
+
+/* Converts a cramfs_info from little endian to big endian. */
+static inline void cramfs_convert_inode_letobe(struct cramfs_inode* inode)
+{
+	u8* inode_bytes = (u8*)inode;
+	u8 old_nloffs[4];
+
+	inode->mode = swab16(inode->mode);
+	inode->uid = swab16(inode->uid);
+	inode->size = (inode_bytes[6] << 16) | (inode_bytes[5] << 8) | (inode_bytes[4]);

eww. Is there a nicer way of doing that?

Might be a bit tricky given the weird way in which struct cramfs_inode was defined.

Quoted 11 lines
fs/cramfs/inode.c +11 -0
+
+	/* Save the old values of the namelength and the offset */
+	memcpy(old_nloffs, inode_bytes+8, 4);
+
+	/* Convert the namelength and the offset */
+	inode_bytes[8] = ((old_nloffs[0] & 0x3f) << 2) | ((old_nloffs[3] & 0xc0) >> 6);
+	inode_bytes[9] = ((old_nloffs[3] & 0x3f) << 2) | ((old_nloffs[2] & 0xc0) >> 6);
+	inode_bytes[10] = ((old_nloffs[2] & 0x3f) << 2) | ((old_nloffs[1] & 0xc0) >> 6);
+	inode_bytes[11] = ((old_nloffs[1] & 0x3f) << 2) | ((old_nloffs[0] & 0xc0) >> 6);
+}
+

fs/cramfs/inode.c static DEFINE_MUTEX(read_mutex); · line 86

Quoted 10 lines
fs/cramfs/inode.c +10 -0
+/* Converts a cramfs superblock from little endian to big endian. */
+static inline void cramfs_convert_super_letobe(struct cramfs_super* super)
+{
+	super->magic = swab32(super->magic);
+	super->size = swab32(super->size);
+	super->flags = swab32(super->flags);
+	super->future = swab32(super->future);
+	cramfs_convert_info_letobe(&super->fsid);
+	cramfs_convert_inode_letobe(&super->root);
+}

These inlines are not sane. Just removing those three takes the sparc64 fs/cramfs/inode.o from 6856 bytes of text down to 5668, which is rather a large difference.

The patch has a number of trivial coding-style errors. scripts/checkpatch.pl finds them.

#18 lore raw
Quoted 11 lines

fs/cramfs/inode.c static DEFINE_MUTEX(read_mutex); · line 65

Quoted 9 lines
fs/cramfs/inode.c +9 -0
+/* Converts a cramfs_info from little endian to big endian. */
+static inline void cramfs_convert_inode_letobe(struct cramfs_inode* inode)
+{
+	u8* inode_bytes = (u8*)inode;
+	u8 old_nloffs[4];
+
+	inode->mode = swab16(inode->mode);
+	inode->uid = swab16(inode->uid);
+	inode->size = (inode_bytes[6] << 16) | (inode_bytes[5] << 8) | (inode_bytes[4]);

eww. Is there a nicer way of doing that?

Yes, I know it's ugly. I searched for a swab24 function. The file fs/jfs/endian24.h defines such a function. Including this file in fs/cramfs/inode.c would be even more ugly than the code above. What about moving this to a header file in include/linux/byteorder? (just a suggestion, perhaps it isn't worth the effort...)

Quoted 20 lines

Might be a bit tricky given the weird way in which struct cramfs_inode was defined.

Quoted 17 lines
fs/cramfs/inode.c +17 -0
+
+	/* Save the old values of the namelength and the offset */
+	memcpy(old_nloffs, inode_bytes+8, 4);
+
+	/* Convert the namelength and the offset */
+	inode_bytes[8] = ((old_nloffs[0] & 0x3f) << 2) | ((old_nloffs[3] & 0xc0) >> 6);
+	inode_bytes[9] = ((old_nloffs[3] & 0x3f) << 2) | ((old_nloffs[2] & 0xc0) >> 6);
+	inode_bytes[10] = ((old_nloffs[2] & 0x3f) << 2) | ((old_nloffs[1] & 0xc0) >> 6);
+	inode_bytes[11] = ((old_nloffs[1] & 0x3f) << 2) | ((old_nloffs[0] & 0xc0) >> 6);
+}
+
+/* Converts a cramfs superblock from little endian to big endian. */
+static inline void cramfs_convert_super_letobe(struct cramfs_super* super)
+{
+	super->magic = swab32(super->magic);
+	super->size = swab32(super->size);
+	super->flags = swab32(super->flags);
Quoted 8 lines

fs/cramfs/inode.c static DEFINE_MUTEX(read_mutex); · line 86

Quoted 4 lines
fs/cramfs/inode.c +4 -0
+	super->future = swab32(super->future);
+	cramfs_convert_info_letobe(&super->fsid);
+	cramfs_convert_inode_letobe(&super->root);
+}

These inlines are not sane. Just removing those three takes the sparc64 fs/cramfs/inode.o from 6856 bytes of text down to 5668, which is rather a large difference.

OK. I'll remove the inline keywords in my copies aswell. Should I prepare a new patch and send it to the mailinglist?

Quoted 2 lines

The patch has a number of trivial coding-style errors. scripts/checkpatch.pl finds them.

Oh, I'm sorry. I should have checked that.

Andi

#19 lore raw

PATCH 2/2 Update documentation

In reply to Andi Drebes

The following patch updates the cramfs documentation according to the changes in PATCH 1/2.

The changes were tested on the following types of machines:
An i386 compatible box (little endian)
UltraSparc IIi (big endian)

---
 README |   19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)
fs/cramfs/README +5 -14
index 445d1c2..91efe77 100644
@@ -6,8 +6,11 @@ a bit looser, e.g. it doesn't care if the <file_data> items are
 swapped around (though it does care that directory entries (inodes) in
 a given directory are contiguous, as this is used by readdir).
 
-All data is currently in host-endian format; neither mkcramfs nor the
-kernel ever do swabbing.  (See section `Block Size' below.)
+All data is now in little endian format. Before, it was in host endian
+format. In order to make filesystem images more shareable between machines
+with a different byte order, cramfs' specification ("this README file")
+was updated. There will be no support for big endian filesystems in the
+future.
 
 <filesystem>:
 	<superblock>
@@ -108,18 +111,6 @@ kernels, not even necessarily kernels of the same architecture if
 PAGE_CACHE_SIZE is subject to change between kernel versions
 (currently possible with arm and ia64).
 
-The remaining options try to make cramfs more sharable.
-
-One part of that is addressing endianness.  The two options here are
-`always use little-endian' (like ext2fs) or `writer chooses
-endianness; kernel adapts at runtime'.  Little-endian wins because of
-code simplicity and little CPU overhead even on big-endian machines.
-
-The cost of swabbing is changing the code to use the le32_to_cpu
-etc. macros as used by ext2fs.  We don't need to swab the compressed
-data, only the superblock, inodes and block pointers.
-
-
 The other part of making cramfs more sharable is choosing a block
 size.  The options are: