[PATCH 1/4] ubifs: Fix data node size for truncating uncompressed nodes

[PATCH 1/4] ubifs: Fix data node size for truncating uncompressed nodes

[Richard Weinberger]

From: David Oberhollenzer <david.oberhollenzer@sigma-star.at>

Currently, the function truncate_data_node only updates the
destination data node size if compression is used. For
uncompressed nodes, the old length is incorrectly retained.

This patch makes sure that the length is correctly set when
compression is disabled.

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/journal.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index 294519b98874..f3b620cbdda4 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -1298,7 +1298,9 @@ static int truncate_data_node(const struct ubifs_info *c, const struct inode *in
 			goto out;
 	}
 
-	if (compr_type != UBIFS_COMPR_NONE) {
+	if (compr_type == UBIFS_COMPR_NONE) {
+		out_len = *new_len;
+	} else {
 		err = ubifs_decompress(c, &dn->data, dlen, buf, &out_len, compr_type);
 		if (err)
 			goto out;
-- 
2.10.2

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

[Richard Weinberger]

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@sigma-star.at>
Signed-off-by: Richard Weinberger <richard@nod.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;
+	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;
+
+		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;
+		}
+	}
+	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

[PATCH 3/4] ubifs: Add assert to dent_key_init()

[Richard Weinberger]

...to make sure that we don't use it for double hashed lookups
instead of dent_key_init_hash().

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/key.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ubifs/key.h b/fs/ubifs/key.h
index 7547be512db2..b1f7c0caa3ac 100644
--- a/fs/ubifs/key.h
+++ b/fs/ubifs/key.h
@@ -162,6 +162,7 @@ static inline void dent_key_init(const struct ubifs_info *c,
 	uint32_t hash = c->key_hash(fname_name(nm), fname_len(nm));
 
 	ubifs_assert(!(hash & ~UBIFS_S_KEY_HASH_MASK));
+	ubifs_assert(!nm->hash && !nm->minor_hash);
 	key->u32[0] = inum;
 	key->u32[1] = hash | (UBIFS_DENT_KEY << UBIFS_S_KEY_HASH_BITS);
 }
-- 
2.10.2

[PATCH 4/4] ubifs: Massage debug prints wrt. fscrypt

[Richard Weinberger]

If file names are encrypted we can no longer print them.
That's why we have to change these prints or remove them completely.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/journal.c | 10 ----------
 fs/ubifs/tnc.c     |  9 ++++-----
 2 files changed, 4 insertions(+), 15 deletions(-)

diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index 7aef413ea2a9..419c79ff377e 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -549,8 +549,6 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct inode *dir,
 	struct ubifs_ino_node *ino;
 	union ubifs_key dent_key, ino_key;
 
-	//dbg_jnl("ino %lu, dent '%.*s', data len %d in dir ino %lu",
-	//	inode->i_ino, nm->len, nm->name, ui->data_len, dir->i_ino);
 	ubifs_assert(mutex_is_locked(&host_ui->ui_mutex));
 
 	dlen = UBIFS_DENT_NODE_SZ + fname_len(nm) + 1;
@@ -956,9 +954,6 @@ int ubifs_jnl_xrename(struct ubifs_info *c, const struct inode *fst_dir,
 	int twoparents = (fst_dir != snd_dir);
 	void *p;
 
-	//dbg_jnl("dent '%pd' in dir ino %lu between dent '%pd' in dir ino %lu",
-	//	fst_dentry, fst_dir->i_ino, snd_dentry, snd_dir->i_ino);
-
 	ubifs_assert(ubifs_inode(fst_dir)->data_len == 0);
 	ubifs_assert(ubifs_inode(snd_dir)->data_len == 0);
 	ubifs_assert(mutex_is_locked(&ubifs_inode(fst_dir)->ui_mutex));
@@ -1100,8 +1095,6 @@ int ubifs_jnl_rename(struct ubifs_info *c, const struct inode *old_dir,
 	int move = (old_dir != new_dir);
 	struct ubifs_inode *uninitialized_var(new_ui);
 
-	//dbg_jnl("dent '%pd' in dir ino %lu to dent '%pd' in dir ino %lu",
-	//	old_dentry, old_dir->i_ino, new_dentry, new_dir->i_ino);
 	ubifs_assert(ubifs_inode(old_dir)->data_len == 0);
 	ubifs_assert(ubifs_inode(new_dir)->data_len == 0);
 	ubifs_assert(mutex_is_locked(&ubifs_inode(old_dir)->ui_mutex));
@@ -1493,9 +1486,6 @@ int ubifs_jnl_delete_xattr(struct ubifs_info *c, const struct inode *host,
 	int sync = IS_DIRSYNC(host);
 	struct ubifs_inode *host_ui = ubifs_inode(host);
 
-	//dbg_jnl("host %lu, xattr ino %lu, name '%s', data len %d",
-	//	host->i_ino, inode->i_ino, nm->name,
-	//	ubifs_inode(inode)->data_len);
 	ubifs_assert(inode->i_nlink == 0);
 	ubifs_assert(mutex_is_locked(&host_ui->ui_mutex));
 
diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c
index d84f4ba467a3..85ee824241fc 100644
--- a/fs/ubifs/tnc.c
+++ b/fs/ubifs/tnc.c
@@ -1812,7 +1812,7 @@ static int do_lookup_nm(struct ubifs_info *c, const union ubifs_key *key,
 	int found, n, err;
 	struct ubifs_znode *znode;
 
-	//dbg_tnck(key, "name '%.*s' key ", nm->len, nm->name);
+	dbg_tnck(key, "key ");
 	mutex_lock(&c->tnc_mutex);
 	found = ubifs_lookup_level0(c, key, &znode, &n);
 	if (!found) {
@@ -2410,8 +2410,7 @@ int ubifs_tnc_add_nm(struct ubifs_info *c, const union ubifs_key *key,
 	struct ubifs_znode *znode;
 
 	mutex_lock(&c->tnc_mutex);
-	//dbg_tnck(key, "LEB %d:%d, name '%.*s', key ",
-	//	 lnum, offs, nm->len, nm->name);
+	dbg_tnck(key, "LEB %d:%d, key ", lnum, offs);
 	found = lookup_level0_dirty(c, key, &znode, &n);
 	if (found < 0) {
 		err = found;
@@ -2645,7 +2644,7 @@ int ubifs_tnc_remove_nm(struct ubifs_info *c, const union ubifs_key *key,
 	struct ubifs_znode *znode;
 
 	mutex_lock(&c->tnc_mutex);
-	//dbg_tnck(key, "%.*s, key ", nm->len, nm->name);
+	dbg_tnck(key, "key ");
 	err = lookup_level0_dirty(c, key, &znode, &n);
 	if (err < 0)
 		goto out_unlock;
@@ -2948,7 +2947,7 @@ struct ubifs_dent_node *ubifs_tnc_next_ent(struct ubifs_info *c,
 	struct ubifs_zbranch *zbr;
 	union ubifs_key *dkey;
 
-	//dbg_tnck(key, "%s ", nm->name ? (char *)nm->name : "(lowest)");
+	dbg_tnck(key, "key ");
 	ubifs_assert(is_hash_key(c, key));
 
 	mutex_lock(&c->tnc_mutex);
-- 
2.10.2

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

[Hyunchul Lee]

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

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

[Richard Weinberger]

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

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@nod.at> 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@linux-m68k.org

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

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

[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 <richard@nod.at> 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

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

[Richard Weinberger]

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 <richard@nod.at> 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 ;)