On Wed, Nov 22, 2017 at 01:37:54PM +0100, Heinrich Schuchardt wrote: > On 11/21/2017 11:40 PM, Ladislav Michl wrote: > > On Tue, Nov 21, 2017 at 11:06:35PM +0100, Heinrich Schuchardt wrote: > > > If 'file' cannot be allocated due to an out of memory > > > situation, NULL is dereferenced. > > > > > > Variables file and dentry are not needed at all. > > > So let's eliminate them. > > > > > > When debugging this patch also avoids a misleading message > > > "cannot find next direntry, error %d" in case of an out of > > > memory situation. It is sufficent to write > > > "%s: Error, no memory for malloc!\n" in this case. > > > > > > Reported-by: Ladislav Michl <la...@linux-mips.org> > > > Reported-by: Alex Sadovsky <nable.mainin...@googlemail.com> > > > Signed-off-by: Heinrich Schuchardt <xypron.g...@gmx.de> > > > --- > > > fs/ubifs/ubifs.c | 25 ++----------------------- > > > 1 file changed, 2 insertions(+), 23 deletions(-) > > > > > > diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c > > > index 4465523d5f..f3d190c763 100644 > > > --- a/fs/ubifs/ubifs.c > > > +++ b/fs/ubifs/ubifs.c > > > @@ -393,29 +393,18 @@ static int ubifs_finddir(struct super_block *sb, > > > char *dirname, > > > 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) { > > > + if (!dir) { > > > printf("%s: Error, no memory for malloc!\n", __func__); > > > - err = -ENOMEM; > > > - goto out; > > > + goto out_free; > > > } > > > 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); > > > - > > > /* Find the first entry in TNC and save it */ > > > lowest_dent_key(c, &key, dir->i_ino); > > > nm.name = NULL; > > > @@ -425,9 +414,6 @@ static int ubifs_finddir(struct super_block *sb, char > > > *dirname, > > > goto out; > > > } > > > - file->f_pos = key_hash_flash(c, &dent->key); > > > - file->private_data = dent; > > > - > > > while (1) { > > > dbg_gen("feed '%s', ino %llu, new f_pos %#x", > > > dent->name, (unsigned long > > > long)le64_to_cpu(dent->inum), > > > @@ -450,10 +436,6 @@ static int ubifs_finddir(struct super_block *sb, > > > char *dirname, > > > err = PTR_ERR(dent); > > > goto out; > > > } > > > - > > > - kfree(file->private_data); > > > > We still need to kfree allocated 'dent' as it was previously allocated: > > dent = kmalloc(zbr->len, GFP_NOFS); > > in ubifs_tnc_next_ent. > > I agree that there is a memory leak. But we should put fixing that into a > separate patch so that we can test both modifications separately.
There was no such memory leak before above patch. > It is not enough to kfree(dent). > ubifs_tnc_next_ent may return ERR_PTR(err) and we do not want to pass this > value to kfree. Nobody is claiming otherwise. > As Wolfgang wrote we should pass error codes to the calling chain of > ubifs_finddir(), i.e. ubifs_findfile(), ubifs_size(), ubifs_read, > ubifs_exists(), ubifs_ls(), ... > > The code also lacks support for the driver model. > > So a lot of other patches needed. Yes, but fix should not add another bug. > If you think this patch fixes what it promises to fix, please, add your > review comment. What about (untested)? diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c index 4465523d5f..64188d9f2d 100644 --- a/fs/ubifs/ubifs.c +++ b/fs/ubifs/ubifs.c @@ -388,47 +388,33 @@ out: static int ubifs_finddir(struct super_block *sb, char *dirname, unsigned long root_inum, unsigned long *inum) { - int err; + int err = 0; 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) { + if (!dir) { printf("%s: Error, no memory for malloc!\n", __func__); - err = -ENOMEM; - goto out; + return -ENOMEM; } 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); - /* Find the first entry in TNC and save it */ lowest_dent_key(c, &key, dir->i_ino); 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 (!err) { + dent = ubifs_tnc_next_ent(c, &key, &nm); + if (IS_ERR(dent)) { + err = PTR_ERR(dent); + break; + } - while (1) { 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)); @@ -438,36 +424,23 @@ 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; - } - - /* 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; + err = 1; + } else { + /* Switch to the next entry */ + key_read(c, &dent->key, &key); + nm.name = (char *)dent->name; } + kfree(dent); - kfree(file->private_data); - file->f_pos = key_hash_flash(c, &dent->key); - file->private_data = dent; cond_resched(); } -out: - if (err != -ENOENT) + if (err != -ENOENT && err != 1) dbg_gen("cannot find next direntry, error %d", err); -out_free: - kfree(file->private_data); - free(file); - free(dentry); - free(dir); + kfree(dir); - return ret; + return err; } static unsigned long ubifs_findfile(struct super_block *sb, char *filename) @@ -508,7 +481,7 @@ static unsigned long ubifs_findfile(struct super_block *sb, char *filename) } ret = ubifs_finddir(sb, name, root_inum, &inum); - if (!ret) + if (ret != 1) return 0; inode = ubifs_iget(sb, inum); _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot