Re: [PATCH 2/4] ubifs: Fix unlink code wrt. double hash lookups

From: Geert Uytterhoeven
Date: Sun Jul 16 2017 - 08:17:58 EST


Hi Richard,

On Sun, Mar 19, 2017 at 9:46 PM, Richard Weinberger <richard@xxxxxx> wrote:
> Am 09.03.2017 um 08:04 schrieb Hyunchul Lee:
>>> - int n, err, type = key_type(c, key);
>>> - struct ubifs_znode *znode;
>>> + int err;

fs/ubifs/tnc.c: In function âsearch_dh_cookieâ:
fs/ubifs/tnc.c:1893: warning: âerrâ is used uninitialized in this function

None of Hyunchul's review comments below ended up in commit 781f675e2d7ec120
("ubifs: Fix unlink code wrt. double hash lookups")?

>> i guess that err should be initialized with -ENOENT to avoid the first call of
>> tnc_next(c, &znode, n).
>
> Yes. err is used unitialized. Happened most likely while moving the code block around.

The initialization can easily be avoided by moving the first call of
tnc_next() to the end of the for-loop.

>>> /**
>>> + * ubifs_tnc_remove_dh - remove an index entry for a "double hashed" node.
>>> + * @c: UBIFS file-system description object
>>> + * @key: key of node
>>> + * @cookie: node cookie for collision resolution
>>> + *
>>> + * Returns %0 on success or negative error code on failure.
>>> + */
>>> +int ubifs_tnc_remove_dh(struct ubifs_info *c, const union ubifs_key *key,
>>> + uint32_t cookie)
>>> +{
>>> + int n, err;
>>> + struct ubifs_znode *znode;
>>> + struct ubifs_dent_node *dent;
>>> + struct ubifs_zbranch *zbr;
>>> +
>>> + if (!c->double_hash)
>>> + return -EOPNOTSUPP;
>>> +
>>> + mutex_lock(&c->tnc_mutex);
>>> + err = lookup_level0_dirty(c, key, &znode, &n);
>>> + if (err <= 0)
>>> + goto out_unlock;

break? :-)
Or return err?

>>> +
>>> + zbr = &znode->zbranch[n];
>>> + dent = kmalloc(UBIFS_MAX_DENT_NODE_SZ, GFP_NOFS);
>>> + if (!dent) {
>>> + err = -ENOMEM;
>>> + goto out_unlock;

break?
Or return err?

>>> + }
>>> +
>>> + err = tnc_read_hashed_node(c, zbr, dent);
>>> + if (err)
>>> + goto out_free;
>>> +
>>> + /* If the cookie does not match, we're facing a hash collision. */
>>> + if (le32_to_cpu(dent->cookie) != cookie) {
>>> + union ubifs_key start_key;
>>> +
>>> + lowest_dent_key(c, &start_key, key_inum(c, key));
>>> +
>>> + err = ubifs_lookup_level0(c, &start_key, &znode, &n);
>>> + if (unlikely(err < 0))
>>> + goto out_unlock;
>>
>> i guess that out_unlock should be replaced with out_free to free dent.
>>
>
> Ohhh, correct.
>
>>> +
>>> + err = search_dh_cookie(c, key, dent, cookie, &znode, &n);
>>> + if (err)
>>> + goto out_free;
>>> + }
>>> +
>>> + if (znode->cnext || !ubifs_zn_dirty(znode)) {
>>> + znode = dirty_cow_bottom_up(c, znode);
>>> + if (IS_ERR(znode)) {
>>> + err = PTR_ERR(znode);
>>> + goto out_unlock;
>>
>> this out_unlock should also be.
>
> Yep. Both are copy&paste errors. :-(
>
> Thanks a lot for pointing this out, your help is much appreciated!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds