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

From: Hyunchul Lee
Date: Thu Mar 09 2017 - 02:34:56 EST

Next message: Omar Sandoval: "Re: kexec regression since 4.9 caused by efi"
Previous message: Inki Dae: "Re: linux-4.11-rc1/drivers/gpu/drm/exynos/exynos5433_drm_decon.c:681: suspicious mask ?"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 <david.oberhollenzer@xxxxxxxxxxxxx>
> Signed-off-by: Richard Weinberger <richard@xxxxxx>
> ---
> 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

Next message: Omar Sandoval: "Re: kexec regression since 4.9 caused by efi"
Previous message: Inki Dae: "Re: linux-4.11-rc1/drivers/gpu/drm/exynos/exynos5433_drm_decon.c:681: suspicious mask ?"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]