[U-Boot] [PATCH v1] ubifs: avoid assert failed in ubifs.c

[U-Boot] [PATCH v1] ubifs: avoid assert failed in ubifs.c

[Patrice Chotard]

This patch solves assert failed displayed in the console during a boot.
The root cause is that the ubifs_inode is not already allocated when
ubifs_printdir and ubifs_finddir functions are called.

Trace showing the issue:
feed 'boot.scr.uimg', ino 94, new f_pos 0x17b40ece
dent->ch.sqnum '7132', creat_sqnum 3886945402880
UBIFS assert failed in ubifs_finddir at 436
INODE ALLOCATION: creat_sqnum '7129'
Found U-Boot script /boot.scr.uimg

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

 fs/ubifs/ubifs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
index 4465523d5fbe..a71c9400ef33 100644
--- a/fs/ubifs/ubifs.c
+++ b/fs/ubifs/ubifs.c
@@ -350,7 +350,9 @@ static int ubifs_printdir(struct file *file, void *dirent)
 		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));
+#ifndef __UBOOT__
 		ubifs_assert(le64_to_cpu(dent->ch.sqnum) > ubifs_inode(dir)->creat_sqnum);
+#endif
 
 		nm.len = le16_to_cpu(dent->nlen);
 		over = filldir(c, (char *)dent->name, nm.len,
@@ -432,7 +434,9 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
 		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));
+#ifndef __UBOOT__
 		ubifs_assert(le64_to_cpu(dent->ch.sqnum) > ubifs_inode(dir)->creat_sqnum);
+#endif
 
 		nm.len = le16_to_cpu(dent->nlen);
 		if ((strncmp(dirname, (char *)dent->name, nm.len) == 0) &&
-- 
1.9.1

[U-Boot] [PATCH v1] ubifs: avoid assert failed in ubifs.c

[Heiko Schocher]

Hello Patrice,

Am 27.04.2018 um 15:51 schrieb Patrice Chotard:
> This patch solves assert failed displayed in the console during a boot.
> The root cause is that the ubifs_inode is not already allocated when
> ubifs_printdir and ubifs_finddir functions are called.
> 
> Trace showing the issue:
> feed 'boot.scr.uimg', ino 94, new f_pos 0x17b40ece
> dent->ch.sqnum '7132', creat_sqnum 3886945402880
> UBIFS assert failed in ubifs_finddir at 436
> INODE ALLOCATION: creat_sqnum '7129'
> Found U-Boot script /boot.scr.uimg
> 
> Signed-off-by: Christophe Kerello <christophe.kerello@st.com>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
> 
>   fs/ubifs/ubifs.c | 4 ++++
>   1 file changed, 4 insertions(+)

Applied to u-boot-ubi.git master

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [PATCH v1] ubifs: avoid assert failed in ubifs.c

[Marek Vasut]

On 04/27/2018 03:51 PM, Patrice Chotard wrote:
> This patch solves assert failed displayed in the console during a boot.
> The root cause is that the ubifs_inode is not already allocated when
> ubifs_printdir and ubifs_finddir functions are called.
> 
> Trace showing the issue:
> feed 'boot.scr.uimg', ino 94, new f_pos 0x17b40ece
> dent->ch.sqnum '7132', creat_sqnum 3886945402880
> UBIFS assert failed in ubifs_finddir at 436
> INODE ALLOCATION: creat_sqnum '7129'
> Found U-Boot script /boot.scr.uimg
> 
> Signed-off-by: Christophe Kerello <christophe.kerello@st.com>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>

I ran into this too, but what I do not quite understand from the commit
message is how hiding the error actually solves the problem that the
assert points to.

Why does the assert trigger in the first place ?

What is the root cause of the issue that is being hidden by this patch?

> ---
> 
>  fs/ubifs/ubifs.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
> index 4465523d5fbe..a71c9400ef33 100644
> --- a/fs/ubifs/ubifs.c
> +++ b/fs/ubifs/ubifs.c
> @@ -350,7 +350,9 @@ static int ubifs_printdir(struct file *file, void *dirent)
>  		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));
> +#ifndef __UBOOT__
>  		ubifs_assert(le64_to_cpu(dent->ch.sqnum) > ubifs_inode(dir)->creat_sqnum);
> +#endif
>  
>  		nm.len = le16_to_cpu(dent->nlen);
>  		over = filldir(c, (char *)dent->name, nm.len,
> @@ -432,7 +434,9 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
>  		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));
> +#ifndef __UBOOT__
>  		ubifs_assert(le64_to_cpu(dent->ch.sqnum) > ubifs_inode(dir)->creat_sqnum);
> +#endif
>  
>  		nm.len = le16_to_cpu(dent->nlen);
>  		if ((strncmp(dirname, (char *)dent->name, nm.len) == 0) &&
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1] ubifs: avoid assert failed in ubifs.c

[Marek Vasut]

On 05/10/2018 10:57 PM, Marek Vasut wrote:
> On 04/27/2018 03:51 PM, Patrice Chotard wrote:
>> This patch solves assert failed displayed in the console during a boot.
>> The root cause is that the ubifs_inode is not already allocated when
>> ubifs_printdir and ubifs_finddir functions are called.
>>
>> Trace showing the issue:
>> feed 'boot.scr.uimg', ino 94, new f_pos 0x17b40ece
>> dent->ch.sqnum '7132', creat_sqnum 3886945402880
>> UBIFS assert failed in ubifs_finddir at 436
>> INODE ALLOCATION: creat_sqnum '7129'
>> Found U-Boot script /boot.scr.uimg
>>
>> Signed-off-by: Christophe Kerello <christophe.kerello@st.com>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> 
> I ran into this too, but what I do not quite understand from the commit
> message is how hiding the error actually solves the problem that the
> assert points to.
> 
> Why does the assert trigger in the first place ?
> 
> What is the root cause of the issue that is being hidden by this patch?

Bump?

>> ---
>>
>>  fs/ubifs/ubifs.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
>> index 4465523d5fbe..a71c9400ef33 100644
>> --- a/fs/ubifs/ubifs.c
>> +++ b/fs/ubifs/ubifs.c
>> @@ -350,7 +350,9 @@ static int ubifs_printdir(struct file *file, void *dirent)
>>  		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));
>> +#ifndef __UBOOT__
>>  		ubifs_assert(le64_to_cpu(dent->ch.sqnum) > ubifs_inode(dir)->creat_sqnum);
>> +#endif
>>  
>>  		nm.len = le16_to_cpu(dent->nlen);
>>  		over = filldir(c, (char *)dent->name, nm.len,
>> @@ -432,7 +434,9 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
>>  		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));
>> +#ifndef __UBOOT__
>>  		ubifs_assert(le64_to_cpu(dent->ch.sqnum) > ubifs_inode(dir)->creat_sqnum);
>> +#endif
>>  
>>  		nm.len = le16_to_cpu(dent->nlen);
>>  		if ((strncmp(dirname, (char *)dent->name, nm.len) == 0) &&
>>
> 
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v1] ubifs: avoid assert failed in ubifs.c

[Richard Weinberger]

Am Dienstag, 22. Mai 2018, 12:56:48 CEST schrieb Marek Vasut:
> On 05/10/2018 10:57 PM, Marek Vasut wrote:
> > On 04/27/2018 03:51 PM, Patrice Chotard wrote:
> >> This patch solves assert failed displayed in the console during a boot.
> >> The root cause is that the ubifs_inode is not already allocated when
> >> ubifs_printdir and ubifs_finddir functions are called.
> >>
> >> Trace showing the issue:
> >> feed 'boot.scr.uimg', ino 94, new f_pos 0x17b40ece
> >> dent->ch.sqnum '7132', creat_sqnum 3886945402880
> >> UBIFS assert failed in ubifs_finddir at 436
> >> INODE ALLOCATION: creat_sqnum '7129'
> >> Found U-Boot script /boot.scr.uimg
> >>
> >> Signed-off-by: Christophe Kerello <christophe.kerello@st.com>
> >> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> > 
> > I ran into this too, but what I do not quite understand from the commit
> > message is how hiding the error actually solves the problem that the
> > assert points to.
> > 
> > Why does the assert trigger in the first place ?
> > 
> > What is the root cause of the issue that is being hidden by this patch?
> 
> Bump?

I had a look, the bug is deeper, ubifs_finddir() allocates a vfs inode manually
and ignores UBIFS internals. ubifs_inode() will read beyond the allocated buffer.
In best case the assert triggers because ->creat_sqnum is garbage, in worst case, U-Boot will
just crash.

AFAICT, the correct solution is to use ubifs_iget().
Then we can keep the assert and it will check for the right thing.

Thanks,
//richard

[U-Boot] [PATCH v1] ubifs: avoid assert failed in ubifs.c

[Ladislav Michl]

On Tue, May 22, 2018 at 01:23:13PM +0200, Richard Weinberger wrote:
> Am Dienstag, 22. Mai 2018, 12:56:48 CEST schrieb Marek Vasut:
> > On 05/10/2018 10:57 PM, Marek Vasut wrote:
> > > On 04/27/2018 03:51 PM, Patrice Chotard wrote:
> > >> This patch solves assert failed displayed in the console during a boot.
> > >> The root cause is that the ubifs_inode is not already allocated when
> > >> ubifs_printdir and ubifs_finddir functions are called.
> > >>
> > >> Trace showing the issue:
> > >> feed 'boot.scr.uimg', ino 94, new f_pos 0x17b40ece
> > >> dent->ch.sqnum '7132', creat_sqnum 3886945402880
> > >> UBIFS assert failed in ubifs_finddir at 436
> > >> INODE ALLOCATION: creat_sqnum '7129'
> > >> Found U-Boot script /boot.scr.uimg
> > >>
> > >> Signed-off-by: Christophe Kerello <christophe.kerello@st.com>
> > >> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> > > 
> > > I ran into this too, but what I do not quite understand from the commit
> > > message is how hiding the error actually solves the problem that the
> > > assert points to.
> > > 
> > > Why does the assert trigger in the first place ?
> > > 
> > > What is the root cause of the issue that is being hidden by this patch?
> > 
> > Bump?
> 
> I had a look, the bug is deeper, ubifs_finddir() allocates a vfs inode manually
                                   ^^^^^^^^^^^^^^^

Hi Richard,

above triggered alert, so here's my old attempt to simplify ubifs_finddir(). Things
could be further simplified, but I got distracted and never had time to look here
again (which I'm affraid of).

Subject: [PATCH] Refactor ubifs_finddir

Just work in progress...

---
 fs/ubifs/ubifs.c | 67 +++++++-----------------------------------------
 1 file changed, 9 insertions(+), 58 deletions(-)

diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
index 47fa41ad1d..a876546a0e 100644
--- a/fs/ubifs/ubifs.c
+++ b/fs/ubifs/ubifs.c
@@ -389,47 +389,20 @@ out:
 static int ubifs_finddir(struct super_block *sb, char *dirname,
 			 unsigned long root_inum, unsigned long *inum)
 {
-	int err;
 	struct qstr nm;
 	union ubifs_key key;
 	struct ubifs_dent_node *dent;
-	struct ubifs_info *c;
-	struct file *file;
-	struct dentry *dentry;
-	struct inode *dir;
-	int ret = 0;
-
-	file = kzalloc(sizeof(struct file), 0);
-	dentry = kzalloc(sizeof(struct dentry), 0);
-	dir = kzalloc(sizeof(struct inode), 0);
-	if (!file || !dentry || !dir) {
-		printf("%s: Error, no memory for malloc!\n", __func__);
-		err = -ENOMEM;
-		goto out;
-	}
-
-	dir->i_sb = sb;
-	file->f_path.dentry = dentry;
-	file->f_path.dentry->d_parent = dentry;
-	file->f_path.dentry->d_inode = dir;
-	file->f_path.dentry->d_inode->i_ino = root_inum;
-	c = sb->s_fs_info;
-
-	dbg_gen("dir ino %lu, f_pos %#llx", dir->i_ino, file->f_pos);
+	struct ubifs_info *c = sb->s_fs_info;
 
 	/* Find the first entry in TNC and save it */
-	lowest_dent_key(c, &key, dir->i_ino);
+	lowest_dent_key(c, &key, root_inum);
 	nm.name = NULL;
-	dent = ubifs_tnc_next_ent(c, &key, &nm);
-	if (IS_ERR(dent)) {
-		err = PTR_ERR(dent);
-		goto out;
-	}
-
-	file->f_pos = key_hash_flash(c, &dent->key);
-	file->private_data = dent;
 
 	while (1) {
+		dent = ubifs_tnc_next_ent(c, &key, &nm);
+		if (IS_ERR(dent))
+			return PTR_ERR(dent);
+
 		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));
@@ -441,36 +414,14 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
 		if ((strncmp(dirname, (char *)dent->name, nm.len) == 0) &&
 		    (strlen(dirname) == nm.len)) {
 			*inum = le64_to_cpu(dent->inum);
-			ret = 1;
-			goto out_free;
+			kfree(dent);
+			return 0;
 		}
-
 		/* Switch to the next entry */
 		key_read(c, &dent->key, &key);
 		nm.name = (char *)dent->name;
-		dent = ubifs_tnc_next_ent(c, &key, &nm);
-		if (IS_ERR(dent)) {
-			err = PTR_ERR(dent);
-			goto out;
-		}
-
-		kfree(file->private_data);
-		file->f_pos = key_hash_flash(c, &dent->key);
-		file->private_data = dent;
-		cond_resched();
+		kfree(dent);
 	}
-
-out:
-	if (err != -ENOENT)
-		dbg_gen("cannot find next direntry, error %d", err);
-
-out_free:
-	kfree(file->private_data);
-	free(file);
-	free(dentry);
-	free(dir);
-
-	return ret;
 }
 
 static unsigned long ubifs_findfile(struct super_block *sb, char *filename)
-- 
2.17.0


> and ignores UBIFS internals. ubifs_inode() will read beyond the allocated buffer.
> In best case the assert triggers because ->creat_sqnum is garbage, in worst case, U-Boot will
> just crash.
> 
> AFAICT, the correct solution is to use ubifs_iget().
> Then we can keep the assert and it will check for the right thing.
> 
> Thanks,
> //richard
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot