Richard,
this patch works well. but i found some trivial mistakes.
On Thu, Feb 09, 2017 at 10:28:35PM +0100, Richard Weinberger wrote:
> When removing an encrypted file with a long anem and without having
> the key we have to be able to locate and remove the directory entry
> via a double hash. This corner case was simply forgotten.
>
> Reported-by: David Oberhollenzer <[email protected]>
> Signed-off-by: Richard Weinberger <[email protected]>
> ---
> fs/ubifs/journal.c | 10 ++++-
> fs/ubifs/tnc.c | 129 ++++++++++++++++++++++++++++++++++++++++++++---------
> fs/ubifs/ubifs.h | 2 +
> 3 files changed, 117 insertions(+), 24 deletions(-)
>
> diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
> index f3b620cbdda4..7aef413ea2a9 100644
> --- a/fs/ubifs/journal.c
> +++ b/fs/ubifs/journal.c
> @@ -585,7 +585,10 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct inode *dir,
>
> if (!xent) {
> dent->ch.node_type = UBIFS_DENT_NODE;
> - dent_key_init(c, &dent_key, dir->i_ino, nm);
> + if (nm->hash)
> + dent_key_init_hash(c, &dent_key, dir->i_ino, nm->hash);
> + else
> + dent_key_init(c, &dent_key, dir->i_ino, nm);
> } else {
> dent->ch.node_type = UBIFS_XENT_NODE;
> xent_key_init(c, &dent_key, dir->i_ino, nm);
> @@ -629,7 +632,10 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct inode *dir,
> kfree(dent);
>
> if (deletion) {
> - err = ubifs_tnc_remove_nm(c, &dent_key, nm);
> + if (nm->hash)
> + err = ubifs_tnc_remove_dh(c, &dent_key, nm->minor_hash);
> + else
> + err = ubifs_tnc_remove_nm(c, &dent_key, nm);
> if (err)
> goto out_ro;
> err = ubifs_add_dirt(c, lnum, dlen);
> diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c
> index 709aa098dd46..d84f4ba467a3 100644
> --- a/fs/ubifs/tnc.c
> +++ b/fs/ubifs/tnc.c
> @@ -1880,48 +1880,65 @@ int ubifs_tnc_lookup_nm(struct ubifs_info *c, const union ubifs_key *key,
> return do_lookup_nm(c, key, node, nm);
> }
>
> -static int do_lookup_dh(struct ubifs_info *c, const union ubifs_key *key,
> - struct ubifs_dent_node *dent, uint32_t cookie)
> +static int search_dh_cookie(struct ubifs_info *c, const union ubifs_key *key,
> + struct ubifs_dent_node *dent, uint32_t cookie,
> + struct ubifs_znode **zn, int *n)
> {
> - int n, err, type = key_type(c, key);
> - struct ubifs_znode *znode;
> + int err;
i guess that err should be initialized with -ENOENT to avoid the first call of
tnc_next(c, &znode, n).
> + struct ubifs_znode *znode = *zn;
> struct ubifs_zbranch *zbr;
> - union ubifs_key *dkey, start_key;
> -
> - ubifs_assert(is_hash_key(c, key));
> -
> - lowest_dent_key(c, &start_key, key_inum(c, key));
> -
> - mutex_lock(&c->tnc_mutex);
> - err = ubifs_lookup_level0(c, &start_key, &znode, &n);
> - if (unlikely(err < 0))
> - goto out_unlock;
> + union ubifs_key *dkey;
>
> for (;;) {
> if (!err) {
> - err = tnc_next(c, &znode, &n);
> + err = tnc_next(c, &znode, n);
> if (err)
> - goto out_unlock;
> + goto out;
> }
>
> - zbr = &znode->zbranch[n];
> + zbr = &znode->zbranch[*n];
> dkey = &zbr->key;
>
> if (key_inum(c, dkey) != key_inum(c, key) ||
> - key_type(c, dkey) != type) {
> + key_type(c, dkey) != key_type(c, key)) {
> err = -ENOENT;
> - goto out_unlock;
> + goto out;
> }
>
> err = tnc_read_hashed_node(c, zbr, dent);
> if (err)
> - goto out_unlock;
> + goto out;
>
> if (key_hash(c, key) == key_hash(c, dkey) &&
> - le32_to_cpu(dent->cookie) == cookie)
> - goto out_unlock;
> + le32_to_cpu(dent->cookie) == cookie) {
> + *zn = znode;
> + goto out;
> + }
> }
>
> +out:
> +
> + return err;
> +}
> +
> +static int do_lookup_dh(struct ubifs_info *c, const union ubifs_key *key,
> + struct ubifs_dent_node *dent, uint32_t cookie)
> +{
> + int n, err;
> + struct ubifs_znode *znode;
> + union ubifs_key start_key;
> +
> + ubifs_assert(is_hash_key(c, key));
> +
> + lowest_dent_key(c, &start_key, key_inum(c, key));
> +
> + mutex_lock(&c->tnc_mutex);
> + err = ubifs_lookup_level0(c, &start_key, &znode, &n);
> + if (unlikely(err < 0))
> + goto out_unlock;
> +
> + err = search_dh_cookie(c, key, dent, cookie, &znode, &n);
> +
> out_unlock:
> mutex_unlock(&c->tnc_mutex);
> return err;
> @@ -2663,6 +2680,74 @@ int ubifs_tnc_remove_nm(struct ubifs_info *c, const union ubifs_key *key,
> }
>
> /**
> + * 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;
> +
> + zbr = &znode->zbranch[n];
> + dent = kmalloc(UBIFS_MAX_DENT_NODE_SZ, GFP_NOFS);
> + if (!dent) {
> + err = -ENOMEM;
> + goto out_unlock;
> + }
> +
> + 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.
> +
> + 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.
> + }
> + }
> + err = tnc_delete(c, znode, n);
> +
> +out_free:
> + kfree(dent);
> +out_unlock:
> + if (!err)
> + err = dbg_check_tnc(c, 0);
> + mutex_unlock(&c->tnc_mutex);
> + return err;
> +}
> +
> +/**
> * key_in_range - determine if a key falls within a range of keys.
> * @c: UBIFS file-system description object
> * @key: key to check
> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> index ca72382ce6cc..36df4613b803 100644
> --- a/fs/ubifs/ubifs.h
> +++ b/fs/ubifs/ubifs.h
> @@ -1589,6 +1589,8 @@ int ubifs_tnc_add_nm(struct ubifs_info *c, const union ubifs_key *key,
> int ubifs_tnc_remove(struct ubifs_info *c, const union ubifs_key *key);
> int ubifs_tnc_remove_nm(struct ubifs_info *c, const union ubifs_key *key,
> const struct fscrypt_name *nm);
> +int ubifs_tnc_remove_dh(struct ubifs_info *c, const union ubifs_key *key,
> + uint32_t cookie);
> int ubifs_tnc_remove_range(struct ubifs_info *c, union ubifs_key *from_key,
> union ubifs_key *to_key);
> int ubifs_tnc_remove_ino(struct ubifs_info *c, ino_t inum);
> --
> 2.10.2
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
thanks,
Hyunchul
Hyunchul,
Am 09.03.2017 um 08:04 schrieb Hyunchul Lee:
>> - int n, err, type = key_type(c, key);
>> - struct ubifs_znode *znode;
>> + int err;
>
> 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.
[...]
>> /**
>> + * 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;
>> +
>> + zbr = &znode->zbranch[n];
>> + dent = kmalloc(UBIFS_MAX_DENT_NODE_SZ, GFP_NOFS);
>> + if (!dent) {
>> + err = -ENOMEM;
>> + goto out_unlock;
>> + }
>> +
>> + 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!
Thanks,
//richard
Hi Richard,
On Sun, Mar 19, 2017 at 9:46 PM, Richard Weinberger <[email protected]> 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 -- [email protected]
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
Geert,
Am 16.07.2017 um 14:12 schrieb Geert Uytterhoeven:
> Hi Richard,
>
> On Sun, Mar 19, 2017 at 9:46 PM, Richard Weinberger <[email protected]> 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")?
Oh, that was not indented.
Maybe I've selected the wrong patch from patchwork.
Will sort out.
Thanks for pointing out,
//richard
Am 16.07.2017 um 14:17 schrieb Richard Weinberger:
> Geert,
>
> Am 16.07.2017 um 14:12 schrieb Geert Uytterhoeven:
>> Hi Richard,
>>
>> On Sun, Mar 19, 2017 at 9:46 PM, Richard Weinberger <[email protected]> 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")?
>
> Oh, that was not indented.
*intentional ;)