ubifs: endian handling fixes and annotations

PATCH In review Harvey Harrison 2 participants 6 messages Oct 24, 2008 – Oct 26, 2008 UBI file system (UBIFS)

Share & tools

Harvey Harrison

author

2008-10-24 18:18 UTC #0 lore raw

PATCH 1/1 ubifs: endian handling fixes and annotations

Noticed by sparse:
fs/ubifs/file.c:75:2: warning: restricted __le64 degrades to integer fs/ubifs/file.c:629:4: warning: restricted __le64 degrades to integer fs/ubifs/dir.c:431:3: warning: restricted __le64 degrades to integer

This should be checked to ensure the ubifs_assert is working as intended, I've done the suggested annotation in this patch.

fs/ubifs/sb.c:298:6: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:298:6: expected int [signed] [assigned] tmp
fs/ubifs/sb.c:298:6: got restricted __le64 [usertype] <noident>
fs/ubifs/sb.c:299:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:299:19: expected restricted __le64 [usertype] atime_sec
fs/ubifs/sb.c:299:19: got int [signed] [assigned] tmp
fs/ubifs/sb.c:300:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:300:19: expected restricted __le64 [usertype] ctime_sec
fs/ubifs/sb.c:300:19: got int [signed] [assigned] tmp
fs/ubifs/sb.c:301:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:301:19: expected restricted __le64 [usertype] mtime_sec
fs/ubifs/sb.c:301:19: got int [signed] [assigned] tmp

This looks like a bugfix as your tmp was a u32 so there was truncation in the atime, mtime, ctime value, probably not intentional, add a tmp_le64 and use it here.

fs/ubifs/key.h:348:9: warning: cast to restricted __le32 fs/ubifs/key.h:348:9: warning: cast to restricted __le32 fs/ubifs/key.h:419:9: warning: cast to restricted __le32

Read from the annotated union member instead.

fs/ubifs/recovery.c:175:13: warning: incorrect type in assignment (different base types)
fs/ubifs/recovery.c:175:13: expected unsigned int [unsigned] [usertype] save_flags
fs/ubifs/recovery.c:175:13: got restricted __le32 [usertype] flags
fs/ubifs/recovery.c:186:13: warning: incorrect type in assignment (different base types)
fs/ubifs/recovery.c:186:13: expected restricted __le32 [usertype] flags
fs/ubifs/recovery.c:186:13: got unsigned int [unsigned] [usertype] save_flags

Do byteshifting at compile time of the flag value. Annotate the saved_flags as le32.

fs/ubifs/debug.c:368:10: warning: cast to restricted __le32 fs/ubifs/debug.c:368:10: warning: cast from restricted __le64

Should be checked if the truncation was intentional, I've changed the printk to print the full width.

Signed-off-by Harvey Harrison <[email protected]>

---
 fs/ubifs/debug.c    |    4 ++--
 fs/ubifs/dir.c      |    3 ++-
 fs/ubifs/file.c     |    4 ++--
 fs/ubifs/key.h      |    4 ++--
 fs/ubifs/recovery.c |    4 ++--
 fs/ubifs/sb.c       |    9 +++++----
 6 files changed, 15 insertions(+), 13 deletions(-)

fs/ubifs/debug.c +2 -2

index 7186400..f9deccb 100644
@@ -364,8 +364,8 @@ void dbg_dump_node(const struct ubifs_info *c, const void *node)
 		       le32_to_cpu(mst->ihead_lnum));
 		printk(KERN_DEBUG "\tihead_offs     %u\n",
 		       le32_to_cpu(mst->ihead_offs));
-		printk(KERN_DEBUG "\tindex_size     %u\n",
-		       le32_to_cpu(mst->index_size));
+		printk(KERN_DEBUG "\tindex_size     %llu\n",
+		       (unsigned long long)le64_to_cpu(mst->index_size));
 		printk(KERN_DEBUG "\tlpt_lnum       %u\n",
 		       le32_to_cpu(mst->lpt_lnum));
 		printk(KERN_DEBUG "\tlpt_offs       %u\n",

fs/ubifs/dir.c +2 -1

index 8561890..198cf3e 100644
@@ -428,7 +428,8 @@ static int ubifs_readdir(struct file *file, void *dirent, filldir_t filldir)
 		dbg_gen("feed '%s', ino %llu, new f_pos %#x",
 			dent->name, (unsigned long long)le64_to_cpu(dent->inum),
 			key_hash_flash(c, &dent->key));
-		ubifs_assert(dent->ch.sqnum > ubifs_inode(dir)->creat_sqnum);
+		ubifs_assert(le64_to_cpu(dent->ch.sqnum) >
+			     ubifs_inode(dir)->creat_sqnum);
 
 		nm.len = le16_to_cpu(dent->nlen);
 		over = filldir(dirent, dent->name, nm.len, file->f_pos,

fs/ubifs/file.c +2 -2

index 51cf511..9124eee 100644
@@ -72,7 +72,7 @@ static int read_block(struct inode *inode, void *addr, unsigned int block,
 		return err;
 	}
 
-	ubifs_assert(dn->ch.sqnum > ubifs_inode(inode)->creat_sqnum);
+	ubifs_assert(le64_to_cpu(dn->ch.sqnum) > ubifs_inode(inode)->creat_sqnum);
 
 	len = le32_to_cpu(dn->size);
 	if (len <= 0 || len > UBIFS_BLOCK_SIZE)

@@ -626,7 +626,7 @@ static int populate_page(struct ubifs_info *c, struct page *page,
 
 			dn = bu->buf + (bu->zbranch[nn].offs - offs);
 
-			ubifs_assert(dn->ch.sqnum >
+			ubifs_assert(le64_to_cpu(dn->ch.sqnum) >
 				     ubifs_inode(inode)->creat_sqnum);
 
 			len = le32_to_cpu(dn->size);

fs/ubifs/key.h +2 -2

index 9ee6508..3f1f16b 100644
@@ -345,7 +345,7 @@ static inline int key_type_flash(const struct ubifs_info *c, const void *k)
 {
 	const union ubifs_key *key = k;
 
-	return le32_to_cpu(key->u32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
+	return le32_to_cpu(key->j32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
 }
 
 /**

@@ -416,7 +416,7 @@ static inline unsigned int key_block_flash(const struct ubifs_info *c,
 {
 	const union ubifs_key *key = k;
 
-	return le32_to_cpu(key->u32[1]) & UBIFS_S_KEY_BLOCK_MASK;
+	return le32_to_cpu(key->j32[1]) & UBIFS_S_KEY_BLOCK_MASK;
 }
 
 /**

fs/ubifs/recovery.c +2 -2

index 77d26c1..bed9742 100644
@@ -168,12 +168,12 @@ static int write_rcvrd_mst_node(struct ubifs_info *c,
 				struct ubifs_mst_node *mst)
 {
 	int err = 0, lnum = UBIFS_MST_LNUM, sz = c->mst_node_alsz;
-	uint32_t save_flags;
+	__le32 save_flags;
 
 	dbg_rcvry("recovery");
 
 	save_flags = mst->flags;
-	mst->flags = cpu_to_le32(le32_to_cpu(mst->flags) | UBIFS_MST_RCVRY);
+	mst->flags |= cpu_to_le32(UBIFS_MST_RCVRY);
 
 	ubifs_prepare_node(c, mst, UBIFS_MST_NODE_SZ, 1);
 	err = ubi_leb_change(c->ubi, lnum, mst, sz, UBI_SHORTTERM);

fs/ubifs/sb.c +5 -4

index 2bf753b..0f39235 100644
@@ -81,6 +81,7 @@ static int create_default_filesystem(struct ubifs_info *c)
 	int lpt_lebs, lpt_first, orph_lebs, big_lpt, ino_waste, sup_flags = 0;
 	int min_leb_cnt = UBIFS_MIN_LEB_CNT;
 	uint64_t tmp64, main_bytes;
+	__le64 tmp_le64;
 
 	/* Some functions called from here depend on the @c->key_len filed */
 	c->key_len = UBIFS_SK_LEN;

@@ -295,10 +296,10 @@ static int create_default_filesystem(struct ubifs_info *c)
 	ino->ch.node_type = UBIFS_INO_NODE;
 	ino->creat_sqnum = cpu_to_le64(++c->max_sqnum);
 	ino->nlink = cpu_to_le32(2);
-	tmp = cpu_to_le64(CURRENT_TIME_SEC.tv_sec);
-	ino->atime_sec   = tmp;
-	ino->ctime_sec   = tmp;
-	ino->mtime_sec   = tmp;
+	tmp_le64 = cpu_to_le64(CURRENT_TIME_SEC.tv_sec);
+	ino->atime_sec   = tmp_le64;
+	ino->ctime_sec   = tmp_le64;
+	ino->mtime_sec   = tmp_le64;
 	ino->atime_nsec  = 0;
 	ino->ctime_nsec  = 0;
 	ino->mtime_nsec  = 0;

Signature · 1 line

1.6.0.3.723.g757e

Artem Bityutskiy

2008-10-25 10:59 UTC #1 lore raw

Harvey,

Quoted 8 lines · Harvey Harrison

Noticed by sparse:
fs/ubifs/file.c:75:2: warning: restricted __le64 degrades to integer fs/ubifs/file.c:629:4: warning: restricted __le64 degrades to integer fs/ubifs/dir.c:431:3: warning: restricted __le64 degrades to integer
This should be checked to ensure the ubifs_assert is working as intended, I've done the suggested annotation in this patch.

Quoted 12 lines

fs/ubifs/sb.c:298:6: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:298:6: expected int [signed] [assigned] tmp
fs/ubifs/sb.c:298:6: got restricted __le64 [usertype] <noident>
fs/ubifs/sb.c:299:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:299:19: expected restricted __le64 [usertype] atime_sec
fs/ubifs/sb.c:299:19: got int [signed] [assigned] tmp
fs/ubifs/sb.c:300:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:300:19: expected restricted __le64 [usertype] ctime_sec
fs/ubifs/sb.c:300:19: got int [signed] [assigned] tmp
fs/ubifs/sb.c:301:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:301:19: expected restricted __le64 [usertype] mtime_sec
fs/ubifs/sb.c:301:19: got int [signed] [assigned] tmp

... snip ...

thanks for the patch. It's shame we did not fix this ourselves. We did run sparse before submitting UBIFS and did not see these warnings. Probably sparse has been improved recently. Anyway, thank you, I'll look closer at your patch and apply it to ubifs-2.6.git.

Signature · 2 lines

Best regards,
Artem Bityutskiy (Битюцкий Артём)

Harvey Harrison

author

2008-10-25 18:53 UTC #2 lore raw

Quoted 24 lines · Artem Bityutskiy

Harvey,
Quoted 8 lines · Harvey Harrison
Noticed by sparse:
fs/ubifs/file.c:75:2: warning: restricted __le64 degrades to integer fs/ubifs/file.c:629:4: warning: restricted __le64 degrades to integer fs/ubifs/dir.c:431:3: warning: restricted __le64 degrades to integer
This should be checked to ensure the ubifs_assert is working as intended, I've done the suggested annotation in this patch.
Quoted 12 lines
fs/ubifs/sb.c:298:6: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:298:6: expected int [signed] [assigned] tmp
fs/ubifs/sb.c:298:6: got restricted __le64 [usertype] <noident>
fs/ubifs/sb.c:299:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:299:19: expected restricted __le64 [usertype] atime_sec
fs/ubifs/sb.c:299:19: got int [signed] [assigned] tmp
fs/ubifs/sb.c:300:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:300:19: expected restricted __le64 [usertype] ctime_sec
fs/ubifs/sb.c:300:19: got int [signed] [assigned] tmp
fs/ubifs/sb.c:301:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:301:19: expected restricted __le64 [usertype] mtime_sec
fs/ubifs/sb.c:301:19: got int [signed] [assigned] tmp

Quoted 7 lines

... snip ...
thanks for the patch. It's shame we did not fix this ourselves. We did run sparse before submitting UBIFS and did not see these warnings. Probably sparse has been improved recently. Anyway, thank you, I'll look closer at your patch and apply it to ubifs-2.6.git.

Run sparse with -D__CHECK_ENDIAN__ to see these warnings.

Harvey

Artem Bityutskiy

2008-10-26 10:14 UTC #3 lore raw

Quoted 1 line · Harvey Harrison

Run sparse with -D__CHECK_ENDIAN__ to see these warnings.

Thanks. Pushed your patch to ubifs-2.6.git.

Signature · 2 lines

Best regards,
Artem Bityutskiy (Битюцкий Артём)

Artem Bityutskiy

2008-10-26 13:24 UTC #4 lore raw

In reply to Harvey Harrison

Quoted 9 lines · Harvey Harrison

Quoted 9 lines
Quoted 6 lines
fs/ubifs/sb.c:300:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:300:19: expected restricted __le64 [usertype] ctime_sec
fs/ubifs/sb.c:300:19: got int [signed] [assigned] tmp
fs/ubifs/sb.c:301:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:301:19: expected restricted __le64 [usertype] mtime_sec
fs/ubifs/sb.c:301:19: got int [signed] [assigned] tmp
... snip ...

Quoted 7 lines

Quoted 5 lines
thanks for the patch. It's shame we did not fix this ourselves. We did run sparse before submitting UBIFS and did not see these warnings. Probably sparse has been improved recently. Anyway, thank you, I'll look closer at your patch and apply it to ubifs-2.6.git.
Run sparse with -D__CHECK_ENDIAN__ to see these warnings.

Any idea why this is not default?

Signature · 2 lines

Best regards,
Artem Bityutskiy (Битюцкий Артём)

Harvey Harrison

author

2008-10-26 18:44 UTC #5 lore raw

Quoted 16 lines · Artem Bityutskiy

Quoted 15 lines · Harvey Harrison
Quoted 14 lines
Quoted 6 lines
fs/ubifs/sb.c:300:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:300:19: expected restricted __le64 [usertype] ctime_sec
fs/ubifs/sb.c:300:19: got int [signed] [assigned] tmp
fs/ubifs/sb.c:301:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:301:19: expected restricted __le64 [usertype] mtime_sec
fs/ubifs/sb.c:301:19: got int [signed] [assigned] tmp
... snip ...
thanks for the patch. It's shame we did not fix this ourselves. We did run sparse before submitting UBIFS and did not see these warnings. Probably sparse has been improved recently. Anyway, thank you, I'll look closer at your patch and apply it to ubifs-2.6.git.

Quoted 4 lines

Quoted 1 line
Run sparse with -D__CHECK_ENDIAN__ to see these warnings.
Any idea why this is not default?

Currently the build gets a bit verbose with this turned on, I've been working to get the noise down a bit, but perhaps still a ways off from turning this on.

Harvey