Re: [U-Boot] [PATCH v1] ubifs: avoid assert failed in ubifs.c
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
> > >> Signed-off-by: Patrice Chotard
> > >
> > > 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 *
Re: [U-Boot] [PATCH v1] ubifs: avoid assert failed in ubifs.c
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 > >> Signed-off-by: Patrice Chotard > > > > 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 mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v1] ubifs: avoid assert failed in ubifs.c
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
>> Signed-off-by: Patrice Chotard
>
> 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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v1] ubifs: avoid assert failed in ubifs.c
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
> Signed-off-by: Patrice Chotard
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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v1] ubifs: avoid assert failed in ubifs.c
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 Signed-off-by: Patrice Chotard --- 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: h...@denx.de ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
