Message-ID: <20170309070439.GA31469@sebu>
Date:   Thu, 9 Mar 2017 16:04:39 +0900
From:   Hyunchul Lee <hyc.lee@...il.com>
To:     Richard Weinberger <richard@....at>
CC:     <linux-mtd@...ts.infradead.org>,
        <david.oberhollenzer@...ma-star.at>,
        <linux-kernel@...r.kernel.org>, <dedekind1@...il.com>,
        <kernel-team@....com>
Subject: Re: [PATCH 2/4] ubifs: Fix unlink code wrt. double hash lookups

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@...ma-star.at>
> Signed-off-by: Richard Weinberger <richard@....at>
> ---
>  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

Powered by blists - more mailing lists