[U-Boot] [PATCH] ubifs: avoid memory corruption during ubifsmount

[U-Boot] [PATCH] ubifs: avoid memory corruption during ubifsmount

[Patrice Chotard]

Sometimes, at boot time, following issue appears:
Error reading superblock on volume 'ubi0:boot' errno=-22!

This error is coming from wrong ubi_num and wrong ubi_id in the superblock.
(ubi_num = -1 and vol_id = -1).
It appears that following line in sget function:
hlist_add_head(&s->s_instances, &type->fs_supers);
corrupts the superblock structure.

By checking ubifs source code, s_instances parameter is not used anymore.
So, by setting this parameter and the associated source code under
__UBOOT__ compilation switch solves this issue.

Signed-off-by: Christophe Kerello <christophe.kerello@st.com>
Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

 fs/ubifs/super.c | 8 ++++----
 fs/ubifs/ubifs.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index abdef1e6ab8d..9603163d8a04 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -2360,7 +2360,9 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags)
 		return ERR_PTR(err);
 	}
 
+#ifndef __UBOOT__
 	INIT_HLIST_NODE(&s->s_instances);
+#endif
 	INIT_LIST_HEAD(&s->s_inodes);
 	s->s_time_gran = 1000000000;
 	s->s_flags = flags;
@@ -2429,14 +2431,12 @@ retry:
 #ifndef __UBOOT__
 	strlcpy(s->s_id, type->name, sizeof(s->s_id));
 	list_add_tail(&s->s_list, &super_blocks);
-#else
-	strncpy(s->s_id, type->name, sizeof(s->s_id));
-#endif
 	hlist_add_head(&s->s_instances, &type->fs_supers);
-#ifndef __UBOOT__
 	spin_unlock(&sb_lock);
 	get_filesystem(type);
 	register_shrinker(&s->s_shrink);
+#else
+	strncpy(s->s_id, type->name, sizeof(s->s_id));
 #endif
 	return s;
 }
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 78c3a68216e1..512fdaa1444d 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -316,8 +316,8 @@ struct super_block {
 	struct backing_dev_info *s_bdi;
 #endif
 	struct mtd_info		*s_mtd;
-	struct hlist_node	s_instances;
 #ifndef __UBOOT__
+	struct hlist_node	s_instances;
 	struct quota_info	s_dquot;	/* Diskquota specific options */
 #endif
 
-- 
1.9.1

[U-Boot] [PATCH] ubifs: avoid memory corruption during ubifsmount

[Richard Weinberger]

Patrice,

Am Montag, 25. Juni 2018, 13:54:12 CEST schrieb Patrice Chotard:
> Sometimes, at boot time, following issue appears:
> Error reading superblock on volume 'ubi0:boot' errno=-22!
> 
> This error is coming from wrong ubi_num and wrong ubi_id in the superblock.
> (ubi_num = -1 and vol_id = -1).
> It appears that following line in sget function:
> hlist_add_head(&s->s_instances, &type->fs_supers);
> corrupts the superblock structure.

Hmm, how can hlist_add_head() corrupt the structure?
This seems fishy to me, I fear that this is not the root cause of the problem
you are facing.

> By checking ubifs source code, s_instances parameter is not used anymore.
> So, by setting this parameter and the associated source code under
> __UBOOT__ compilation switch solves this issue.

Yes, we can clean up this. But as I said, we need to dig deeper to explain
the corruption you see.

Thanks,
//richard

[U-Boot] [PATCH] ubifs: avoid memory corruption during ubifsmount

[Patrice CHOTARD]

Hi Richard

On 06/25/2018 10:47 PM, Richard Weinberger wrote:
> Patrice,
> 
> Am Montag, 25. Juni 2018, 13:54:12 CEST schrieb Patrice Chotard:
>> Sometimes, at boot time, following issue appears:
>> Error reading superblock on volume 'ubi0:boot' errno=-22!
>>
>> This error is coming from wrong ubi_num and wrong ubi_id in the superblock.
>> (ubi_num = -1 and vol_id = -1).
>> It appears that following line in sget function:
>> hlist_add_head(&s->s_instances, &type->fs_supers);
>> corrupts the superblock structure.
> 
> Hmm, how can hlist_add_head() corrupt the structure?
> This seems fishy to me, I fear that this is not the root cause of the problem
> you are facing.

Following your remark, Christophe and i relaunched our test setup to go 
deeper in the analysis of the issue we saw. Unfortunately we can't 
reproduced it. We have now some doubt.

> 
>> By checking ubifs source code, s_instances parameter is not used anymore.
>> So, by setting this parameter and the associated source code under
>> __UBOOT__ compilation switch solves this issue.
> 
> Yes, we can clean up this. But as I said, we need to dig deeper to explain
> the corruption you see.

Nevertheless, do you think it's useful to clean this code and submit a 
new patch with clean-up purpose only ?

Patrice

> 
> Thanks,
> //richard
> 

[U-Boot] [PATCH] ubifs: avoid memory corruption during ubifsmount

[Richard Weinberger]

Patrice,

Am Dienstag, 26. Juni 2018, 15:01:14 CEST schrieb Patrice CHOTARD:
> Following your remark, Christophe and i relaunched our test setup to go 
> deeper in the analysis of the issue we saw. Unfortunately we can't 
> reproduced it. We have now some doubt.

Ok.
 
> > 
> >> By checking ubifs source code, s_instances parameter is not used anymore.
> >> So, by setting this parameter and the associated source code under
> >> __UBOOT__ compilation switch solves this issue.
> > 
> > Yes, we can clean up this. But as I said, we need to dig deeper to explain
> > the corruption you see.
> 
> Nevertheless, do you think it's useful to clean this code and submit a 
> new patch with clean-up purpose only ?

Yes, this makes sense.

Thanks,
//richard

[U-Boot] [PATCH] ubifs: avoid memory corruption during ubifsmount

[Patrice Chotard]

From: Christophe Kerello <christophe.kerello@st.com>

Sometimes, at boot time, following issue appears:
Error reading superblock on volume 'ubi0:boot' errno=-22!

This error is coming from wrong ubi_num and wrong ubi_id in the superblock.
(ubi_num = -1 and vol_id = -1).
It appears that following line in sget function:
hlist_add_head(&s->s_instances, &type->fs_supers);
corrupts the superblock structure.

By checking ubifs source code, s_instances parameter is not used anymore.
So, by setting this parameter and the associated source code under
__UBOOT__ compilation switch solves this issue.

Signed-off-by: Christophe Kerello <christophe.kerello@st.com>
Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

 fs/ubifs/super.c | 8 ++++----
 fs/ubifs/ubifs.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index abdef1e6ab8d..9603163d8a04 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -2360,7 +2360,9 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags)
 		return ERR_PTR(err);
 	}
 
+#ifndef __UBOOT__
 	INIT_HLIST_NODE(&s->s_instances);
+#endif
 	INIT_LIST_HEAD(&s->s_inodes);
 	s->s_time_gran = 1000000000;
 	s->s_flags = flags;
@@ -2429,14 +2431,12 @@ retry:
 #ifndef __UBOOT__
 	strlcpy(s->s_id, type->name, sizeof(s->s_id));
 	list_add_tail(&s->s_list, &super_blocks);
-#else
-	strncpy(s->s_id, type->name, sizeof(s->s_id));
-#endif
 	hlist_add_head(&s->s_instances, &type->fs_supers);
-#ifndef __UBOOT__
 	spin_unlock(&sb_lock);
 	get_filesystem(type);
 	register_shrinker(&s->s_shrink);
+#else
+	strncpy(s->s_id, type->name, sizeof(s->s_id));
 #endif
 	return s;
 }
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 78c3a68216e1..512fdaa1444d 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -316,8 +316,8 @@ struct super_block {
 	struct backing_dev_info *s_bdi;
 #endif
 	struct mtd_info		*s_mtd;
-	struct hlist_node	s_instances;
 #ifndef __UBOOT__
+	struct hlist_node	s_instances;
 	struct quota_info	s_dquot;	/* Diskquota specific options */
 #endif
 
-- 
1.9.1