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

[Geert Uytterhoeven]

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