[PATCH 1/2] hfsplus: add error checking for hfs_find_init()

[PATCH 1/2] hfsplus: add error checking for hfs_find_init()

[Alexey Khoroshilov]

hfs_find_init() may fail with ENOMEM, but there are places, where
the returned value is not checked. The consequences can be very
unpleasant, e.g. kfree uninitialized pointer and
inappropriate mutex unlocking.

The patch adds checks for errors in hfs_find_init().

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 fs/hfsplus/catalog.c |   18 +++++++++++++-----
 fs/hfsplus/dir.c     |   10 ++++++++--
 fs/hfsplus/extents.c |   25 +++++++++++++++++--------
 fs/hfsplus/inode.c   |   12 +++++++-----
 fs/hfsplus/super.c   |   16 ++++++++++------
 5 files changed, 55 insertions(+), 26 deletions(-)

diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
index b4ba1b3..6255de0 100644
--- a/fs/hfsplus/catalog.c
+++ b/fs/hfsplus/catalog.c
@@ -212,7 +212,9 @@ int hfsplus_create_cat(u32 cnid, struct inode *dir,
 
 	dprint(DBG_CAT_MOD, "create_cat: %s,%u(%d)\n",
 		str->name, cnid, inode->i_nlink);
-	hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	if (err)
+		goto err_init;
 
 	hfsplus_cat_build_key(sb, fd.search_key, cnid, NULL);
 	entry_size = hfsplus_fill_cat_thread(sb, &entry,
@@ -255,6 +257,7 @@ err1:
 		hfs_brec_remove(&fd);
 err2:
 	hfs_find_exit(&fd);
+err_init:
 	return err;
 }
 
@@ -269,7 +272,9 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, struct qstr *str)
 
 	dprint(DBG_CAT_MOD, "delete_cat: %s,%u\n",
 		str ? str->name : NULL, cnid);
-	hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	if (err)
+		goto fail_init;
 
 	if (!str) {
 		int len;
@@ -335,7 +340,7 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, struct qstr *str)
 	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
 out:
 	hfs_find_exit(&fd);
-
+fail_init:
 	return err;
 }
 
@@ -347,12 +352,14 @@ int hfsplus_rename_cat(u32 cnid,
 	struct hfs_find_data src_fd, dst_fd;
 	hfsplus_cat_entry entry;
 	int entry_size, type;
-	int err = 0;
+	int err;
 
 	dprint(DBG_CAT_MOD, "rename_cat: %u - %lu,%s - %lu,%s\n",
 		cnid, src_dir->i_ino, src_name->name,
 		dst_dir->i_ino, dst_name->name);
-	hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &src_fd);
+	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &src_fd);
+	if (err)
+		goto fail_init;
 	dst_fd = src_fd;
 
 	/* find the old dir entry and read the data */
@@ -417,5 +424,6 @@ int hfsplus_rename_cat(u32 cnid,
 out:
 	hfs_bnode_put(dst_fd.bnode);
 	hfs_find_exit(&src_fd);
+fail_init:
 	return err;
 }
diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index 4df5059..6c2ea98 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -38,7 +38,9 @@ static struct dentry *hfsplus_lookup(struct inode *dir, struct dentry *dentry,
 	sb = dir->i_sb;
 
 	dentry->d_fsdata = NULL;
-	hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	if (err)
+		goto fail_init;
 	hfsplus_cat_build_key(sb, fd.search_key, dir->i_ino, &dentry->d_name);
 again:
 	err = hfs_brec_read(&fd, &entry, sizeof(entry));
@@ -115,6 +117,7 @@ out:
 	return NULL;
 fail:
 	hfs_find_exit(&fd);
+fail_init:
 	return ERR_PTR(err);
 }
 
@@ -132,7 +135,9 @@ static int hfsplus_readdir(struct file *filp, void *dirent, filldir_t filldir)
 	if (filp->f_pos >= inode->i_size)
 		return 0;
 
-	hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	if (err)
+		goto fail_init;
 	hfsplus_cat_build_key(sb, fd.search_key, inode->i_ino, NULL);
 	err = hfs_brec_find(&fd);
 	if (err)
@@ -234,6 +239,7 @@ next:
 	memcpy(&rd->key, fd.key, sizeof(struct hfsplus_cat_key));
 out:
 	hfs_find_exit(&fd);
+fail_init:
 	return err;
 }
 
diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
index b1991a2..6db4586 100644
--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -124,9 +124,10 @@ static void hfsplus_ext_write_extent_locked(struct inode *inode)
 	if (HFSPLUS_I(inode)->extent_state & HFSPLUS_EXT_DIRTY) {
 		struct hfs_find_data fd;
 
-		hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd);
-		__hfsplus_ext_write_extent(inode, &fd);
-		hfs_find_exit(&fd);
+		if (!hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd)) {
+			__hfsplus_ext_write_extent(inode, &fd);
+			hfs_find_exit(&fd);
+		}
 	}
 }
 
@@ -194,9 +195,11 @@ static int hfsplus_ext_read_extent(struct inode *inode, u32 block)
 	    block < hip->cached_start + hip->cached_blocks)
 		return 0;
 
-	hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd);
-	res = __hfsplus_ext_cache_extent(&fd, inode, block);
-	hfs_find_exit(&fd);
+	res = hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd);
+	if (!res) {
+		res = __hfsplus_ext_cache_extent(&fd, inode, block);
+		hfs_find_exit(&fd);
+	}
 	return res;
 }
 
@@ -371,7 +374,9 @@ int hfsplus_free_fork(struct super_block *sb, u32 cnid,
 	if (total_blocks == blocks)
 		return 0;
 
-	hfs_find_init(HFSPLUS_SB(sb)->ext_tree, &fd);
+	res = hfs_find_init(HFSPLUS_SB(sb)->ext_tree, &fd);
+	if (res)
+		return res;
 	do {
 		res = __hfsplus_ext_read_extent(&fd, ext_entry, cnid,
 						total_blocks, type);
@@ -523,7 +528,11 @@ void hfsplus_file_truncate(struct inode *inode)
 		goto out;
 
 	mutex_lock(&hip->extents_lock);
-	hfs_find_init(HFSPLUS_SB(sb)->ext_tree, &fd);
+	res = hfs_find_init(HFSPLUS_SB(sb)->ext_tree, &fd);
+	if (res) {
+		mutex_unlock(&hip->extents_lock);
+		return;
+	}
 	while (1) {
 		if (alloc_cnt == hip->first_blocks) {
 			hfsplus_free_extents(sb, hip->first_extents,
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index b248a6c..010cd36 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -195,11 +195,13 @@ static struct dentry *hfsplus_file_lookup(struct inode *dir,
 	hip->flags = 0;
 	set_bit(HFSPLUS_I_RSRC, &hip->flags);
 
-	hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
-	err = hfsplus_find_cat(sb, dir->i_ino, &fd);
-	if (!err)
-		err = hfsplus_cat_read_inode(inode, &fd);
-	hfs_find_exit(&fd);
+	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	if (!err) {
+		err = hfsplus_find_cat(sb, dir->i_ino, &fd);
+		if (!err)
+			err = hfsplus_cat_read_inode(inode, &fd);
+		hfs_find_exit(&fd);
+	}
 	if (err) {
 		iput(inode);
 		return ERR_PTR(err);
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index b49b555..07a0502 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -73,11 +73,13 @@ struct inode *hfsplus_iget(struct super_block *sb, unsigned long ino)
 
 	if (inode->i_ino >= HFSPLUS_FIRSTUSER_CNID ||
 	    inode->i_ino == HFSPLUS_ROOT_CNID) {
-		hfs_find_init(HFSPLUS_SB(inode->i_sb)->cat_tree, &fd);
-		err = hfsplus_find_cat(inode->i_sb, inode->i_ino, &fd);
-		if (!err)
-			err = hfsplus_cat_read_inode(inode, &fd);
-		hfs_find_exit(&fd);
+		err = hfs_find_init(HFSPLUS_SB(inode->i_sb)->cat_tree, &fd);
+		if (!err) {
+			err = hfsplus_find_cat(inode->i_sb, inode->i_ino, &fd);
+			if (!err)
+				err = hfsplus_cat_read_inode(inode, &fd);
+			hfs_find_exit(&fd);
+		}
 	} else {
 		err = hfsplus_system_read_inode(inode);
 	}
@@ -447,7 +449,9 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 
 	str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1;
 	str.name = HFSP_HIDDENDIR_NAME;
-	hfs_find_init(sbi->cat_tree, &fd);
+	err = hfs_find_init(sbi->cat_tree, &fd);
+	if (err)
+		goto out_put_root;
 	hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
 	if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
 		hfs_find_exit(&fd);
-- 
1.7.4.1

[PATCH 2/2] hfsplus: Fix double iput of the same inode in hfsplus_fill_super()

[Alexey Khoroshilov]

There is a misprint in resource deallocation code on error path in
hfsplus_fill_super(): the sbi->alloc_file inode is iput twice,
while the root inode in not iput at all.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 fs/hfsplus/super.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 07a0502..8bfbd38 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -504,7 +504,7 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 out_put_hidden_dir:
 	iput(sbi->hidden_dir);
 out_put_root:
-	iput(sbi->alloc_file);
+	iput(root);
 out_put_alloc_file:
 	iput(sbi->alloc_file);
 out_close_cat_tree:
-- 
1.7.4.1

Re: [PATCH 1/2] hfsplus: add error checking for hfs_find_init()

[Christoph Hellwig]

On Fri, Jun 24, 2011 at 01:15:01AM +0400, Alexey Khoroshilov wrote:
> hfs_find_init() may fail with ENOMEM, but there are places, where
> the returned value is not checked. The consequences can be very
> unpleasant, e.g. kfree uninitialized pointer and
> inappropriate mutex unlocking.
> 
> The patch adds checks for errors in hfs_find_init().
> 
> Found by Linux Driver Verification project (linuxtesting.org).

What kind of testing did you do in detail?

> -	hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
> +	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
> +	if (err)
> +		goto err_init;
>  
>  	hfsplus_cat_build_key(sb, fd.search_key, cnid, NULL);
>  	entry_size = hfsplus_fill_cat_thread(sb, &entry,
> @@ -255,6 +257,7 @@ err1:
>  		hfs_brec_remove(&fd);
>  err2:
>  	hfs_find_exit(&fd);
> +err_init:

Please just return the error directly unless there's something to
unwind, both here and in other places.

> @@ -124,9 +124,10 @@ static void hfsplus_ext_write_extent_locked(struct inode *inode)
>  	if (HFSPLUS_I(inode)->extent_state & HFSPLUS_EXT_DIRTY) {
>  		struct hfs_find_data fd;
>  
> -		hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd);
> -		__hfsplus_ext_write_extent(inode, &fd);
> -		hfs_find_exit(&fd);
> +		if (!hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd)) {
> +			__hfsplus_ext_write_extent(inode, &fd);
> +			hfs_find_exit(&fd);
> +		}
>  	}
>  }

This one need to be propagated back through the callers.

> @@ -523,7 +528,11 @@ void hfsplus_file_truncate(struct inode *inode)
>  		goto out;
>  
>  	mutex_lock(&hip->extents_lock);
> -	hfs_find_init(HFSPLUS_SB(sb)->ext_tree, &fd);
> +	res = hfs_find_init(HFSPLUS_SB(sb)->ext_tree, &fd);
> +	if (res) {
> +		mutex_unlock(&hip->extents_lock);
> +		return;
> +	}

At least add an XXX comment about the lack of error handling here.
Once hfsplus gets converted to the new truncate sequence we'll be
able to handle to return it.

Re: [PATCH 2/2] hfsplus: Fix double iput of the same inode in hfsplus_fill_super()

[Christoph Hellwig]

On Fri, Jun 24, 2011 at 01:15:02AM +0400, Alexey Khoroshilov wrote:
> There is a misprint in resource deallocation code on error path in
> hfsplus_fill_super(): the sbi->alloc_file inode is iput twice,
> while the root inode in not iput at all.

Thanks, I'll put it in.

[PATCH v2 0/2] hfsplus: add error checking for hfs_find_init()

[Alexey Khoroshilov]

>> hfs_find_init() may fail with ENOMEM, but there are places, where
>> the returned value is not checked. The consequences can be very
>> unpleasant, e.g. kfree uninitialized pointer and
>> inappropriate mutex unlocking.
>> 
>> The patch adds checks for errors in hfs_find_init().
>> 
>> Found by Linux Driver Verification project (linuxtesting.org).
>
> What kind of testing did you do in detail?

Actually we work on two complementary approaches.

The first one is heavy-weight static analysis of drivers source code 
aimed to detect incorrect usage of kernel core APIs. It was a violation
of mutex usage rule detected by our tools that uncovers lack of error checking
of hfs_find_init() returned value. Yes, memory allocation failure is not 
an often event, but if it happens in inappropriate moment
(e.g. in hfs_find_init() in our case) consequencies can be very unplesant.
A benefit of static analysis approach is automatic evaluation of 
such seldom executed paths that can be hard to reproduce and that lead to 
hard to catch failures.

The second approach is implemented by KEDR toolset that aimed to facilitate 
runtime analysis of kernel modules. KEDR tools operate on the modules chosen 
by the user and can detect memory leaks, perform fault simulation according 
to user-defined scenarios and more. This approach often gives more sound results,
but it requires extra efforts to ensure good coverage and
it requires presence of actual hardware for device drivers verification.


>> -        hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
>> +        err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
>> +        if (err)
>> +                goto err_init;
>>  
>>          hfsplus_cat_build_key(sb, fd.search_key, cnid, NULL);
>>          entry_size = hfsplus_fill_cat_thread(sb, &entry,
>> @@ -255,6 +257,7 @@ err1:
>>                  hfs_brec_remove(&fd);
>>  err2:
>>          hfs_find_exit(&fd);
>> +err_init:
>
> Please just return the error directly unless there's something to
> unwind, both here and in other places.

Done.

>> @@ -124,9 +124,10 @@ static void hfsplus_ext_write_extent_locked(struct inode *inode)
>>          if (HFSPLUS_I(inode)->extent_state & HFSPLUS_EXT_DIRTY) {
>>                  struct hfs_find_data fd;
>>  
>> -                hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd);
>> -                __hfsplus_ext_write_extent(inode, &fd);
>> -                hfs_find_exit(&fd);
>> +                if (!hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd)) {
>> +                        __hfsplus_ext_write_extent(inode, &fd);
>> +                        hfs_find_exit(&fd);
>> +                }
>>          }
>>  }
>
> This one need to be propagated back through the callers.

Done.

>> @@ -523,7 +528,11 @@ void hfsplus_file_truncate(struct inode *inode)
>>                  goto out;
>>  
>>          mutex_lock(&hip->extents_lock);
>> -        hfs_find_init(HFSPLUS_SB(sb)->ext_tree, &fd);
>> +        res = hfs_find_init(HFSPLUS_SB(sb)->ext_tree, &fd);
>> +        if (res) {
>> +                mutex_unlock(&hip->extents_lock);
>> +                return;
>> +        }
>
> At least add an XXX comment about the lack of error handling here.
> Once hfsplus gets converted to the new truncate sequence we'll be
> able to handle to return it.

Done.

[PATCH v2 1/2] hfsplus: add error checking for hfs_find_init()

[Alexey Khoroshilov]

hfs_find_init() may fail with ENOMEM, but there are places, where
the returned value is not checked. The consequences can be very
unpleasant, e.g. kfree uninitialized pointer and
inappropriate mutex unlocking.

The patch adds checks for errors in hfs_find_init().

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 fs/hfsplus/catalog.c |   14 ++++++++++----
 fs/hfsplus/dir.c     |    8 ++++++--
 fs/hfsplus/extents.c |   27 ++++++++++++++++++---------
 fs/hfsplus/inode.c   |   12 +++++++-----
 fs/hfsplus/super.c   |   16 ++++++++++------
 5 files changed, 51 insertions(+), 26 deletions(-)

diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
index b4ba1b3..4dfbfec 100644
--- a/fs/hfsplus/catalog.c
+++ b/fs/hfsplus/catalog.c
@@ -212,7 +212,9 @@ int hfsplus_create_cat(u32 cnid, struct inode *dir,
 
 	dprint(DBG_CAT_MOD, "create_cat: %s,%u(%d)\n",
 		str->name, cnid, inode->i_nlink);
-	hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	if (err)
+		return err;
 
 	hfsplus_cat_build_key(sb, fd.search_key, cnid, NULL);
 	entry_size = hfsplus_fill_cat_thread(sb, &entry,
@@ -269,7 +271,9 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, struct qstr *str)
 
 	dprint(DBG_CAT_MOD, "delete_cat: %s,%u\n",
 		str ? str->name : NULL, cnid);
-	hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	if (err)
+		return err;
 
 	if (!str) {
 		int len;
@@ -347,12 +351,14 @@ int hfsplus_rename_cat(u32 cnid,
 	struct hfs_find_data src_fd, dst_fd;
 	hfsplus_cat_entry entry;
 	int entry_size, type;
-	int err = 0;
+	int err;
 
 	dprint(DBG_CAT_MOD, "rename_cat: %u - %lu,%s - %lu,%s\n",
 		cnid, src_dir->i_ino, src_name->name,
 		dst_dir->i_ino, dst_name->name);
-	hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &src_fd);
+	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &src_fd);
+	if (err)
+		return err;
 	dst_fd = src_fd;
 
 	/* find the old dir entry and read the data */
diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index 4df5059..25b2443 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -38,7 +38,9 @@ static struct dentry *hfsplus_lookup(struct inode *dir, struct dentry *dentry,
 	sb = dir->i_sb;
 
 	dentry->d_fsdata = NULL;
-	hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	if (err)
+		return ERR_PTR(err);
 	hfsplus_cat_build_key(sb, fd.search_key, dir->i_ino, &dentry->d_name);
 again:
 	err = hfs_brec_read(&fd, &entry, sizeof(entry));
@@ -132,7 +134,9 @@ static int hfsplus_readdir(struct file *filp, void *dirent, filldir_t filldir)
 	if (filp->f_pos >= inode->i_size)
 		return 0;
 
-	hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	if (err)
+		return err;
 	hfsplus_cat_build_key(sb, fd.search_key, inode->i_ino, NULL);
 	err = hfs_brec_find(&fd);
 	if (err)
diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
index b1991a2..59fa09f 100644
--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -124,9 +124,10 @@ static void hfsplus_ext_write_extent_locked(struct inode *inode)
 	if (HFSPLUS_I(inode)->extent_state & HFSPLUS_EXT_DIRTY) {
 		struct hfs_find_data fd;
 
-		hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd);
-		__hfsplus_ext_write_extent(inode, &fd);
-		hfs_find_exit(&fd);
+		if (!hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd)) {
+			__hfsplus_ext_write_extent(inode, &fd);
+			hfs_find_exit(&fd);
+		}
 	}
 }
 
@@ -194,9 +195,11 @@ static int hfsplus_ext_read_extent(struct inode *inode, u32 block)
 	    block < hip->cached_start + hip->cached_blocks)
 		return 0;
 
-	hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd);
-	res = __hfsplus_ext_cache_extent(&fd, inode, block);
-	hfs_find_exit(&fd);
+	res = hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd);
+	if (!res) {
+		res = __hfsplus_ext_cache_extent(&fd, inode, block);
+		hfs_find_exit(&fd);
+	}
 	return res;
 }
 
@@ -371,7 +374,9 @@ int hfsplus_free_fork(struct super_block *sb, u32 cnid,
 	if (total_blocks == blocks)
 		return 0;
 
-	hfs_find_init(HFSPLUS_SB(sb)->ext_tree, &fd);
+	res = hfs_find_init(HFSPLUS_SB(sb)->ext_tree, &fd);
+	if (res)
+		return res;
 	do {
 		res = __hfsplus_ext_read_extent(&fd, ext_entry, cnid,
 						total_blocks, type);
@@ -500,7 +505,6 @@ void hfsplus_file_truncate(struct inode *inode)
 		struct page *page;
 		void *fsdata;
 		u32 size = inode->i_size;
-		int res;
 
 		res = pagecache_write_begin(NULL, mapping, size, 0,
 						AOP_FLAG_UNINTERRUPTIBLE,
@@ -523,7 +527,12 @@ void hfsplus_file_truncate(struct inode *inode)
 		goto out;
 
 	mutex_lock(&hip->extents_lock);
-	hfs_find_init(HFSPLUS_SB(sb)->ext_tree, &fd);
+	res = hfs_find_init(HFSPLUS_SB(sb)->ext_tree, &fd);
+	if (res) {
+		mutex_unlock(&hip->extents_lock);
+		/* XXX: We lack error handling of hfsplus_file_truncate() */
+		return;
+	}
 	while (1) {
 		if (alloc_cnt == hip->first_blocks) {
 			hfsplus_free_extents(sb, hip->first_extents,
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index b248a6c..010cd36 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -195,11 +195,13 @@ static struct dentry *hfsplus_file_lookup(struct inode *dir,
 	hip->flags = 0;
 	set_bit(HFSPLUS_I_RSRC, &hip->flags);
 
-	hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
-	err = hfsplus_find_cat(sb, dir->i_ino, &fd);
-	if (!err)
-		err = hfsplus_cat_read_inode(inode, &fd);
-	hfs_find_exit(&fd);
+	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	if (!err) {
+		err = hfsplus_find_cat(sb, dir->i_ino, &fd);
+		if (!err)
+			err = hfsplus_cat_read_inode(inode, &fd);
+		hfs_find_exit(&fd);
+	}
 	if (err) {
 		iput(inode);
 		return ERR_PTR(err);
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index b49b555..07a0502 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -73,11 +73,13 @@ struct inode *hfsplus_iget(struct super_block *sb, unsigned long ino)
 
 	if (inode->i_ino >= HFSPLUS_FIRSTUSER_CNID ||
 	    inode->i_ino == HFSPLUS_ROOT_CNID) {
-		hfs_find_init(HFSPLUS_SB(inode->i_sb)->cat_tree, &fd);
-		err = hfsplus_find_cat(inode->i_sb, inode->i_ino, &fd);
-		if (!err)
-			err = hfsplus_cat_read_inode(inode, &fd);
-		hfs_find_exit(&fd);
+		err = hfs_find_init(HFSPLUS_SB(inode->i_sb)->cat_tree, &fd);
+		if (!err) {
+			err = hfsplus_find_cat(inode->i_sb, inode->i_ino, &fd);
+			if (!err)
+				err = hfsplus_cat_read_inode(inode, &fd);
+			hfs_find_exit(&fd);
+		}
 	} else {
 		err = hfsplus_system_read_inode(inode);
 	}
@@ -447,7 +449,9 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 
 	str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1;
 	str.name = HFSP_HIDDENDIR_NAME;
-	hfs_find_init(sbi->cat_tree, &fd);
+	err = hfs_find_init(sbi->cat_tree, &fd);
+	if (err)
+		goto out_put_root;
 	hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
 	if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
 		hfs_find_exit(&fd);
-- 
1.7.4.1

[PATCH v2 2/2] hfsplus: Add error propagation for hfsplus_ext_write_extent_locked

[Alexey Khoroshilov]

Implement error propagation through the callers of
hfsplus_ext_write_extent_locked().

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 fs/hfsplus/extents.c    |   26 ++++++++++++++++++--------
 fs/hfsplus/hfsplus_fs.h |    2 +-
 fs/hfsplus/super.c      |    6 +++++-
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
index 59fa09f..9235c41 100644
--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -119,23 +119,31 @@ static void __hfsplus_ext_write_extent(struct inode *inode,
 	set_bit(HFSPLUS_I_EXT_DIRTY, &hip->flags);
 }
 
-static void hfsplus_ext_write_extent_locked(struct inode *inode)
+static int hfsplus_ext_write_extent_locked(struct inode *inode)
 {
+	int res;
+
 	if (HFSPLUS_I(inode)->extent_state & HFSPLUS_EXT_DIRTY) {
 		struct hfs_find_data fd;
 
-		if (!hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd)) {
-			__hfsplus_ext_write_extent(inode, &fd);
-			hfs_find_exit(&fd);
-		}
+		res = hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd);
+		if (res)
+			return res;
+		__hfsplus_ext_write_extent(inode, &fd);
+		hfs_find_exit(&fd);
 	}
+	return 0;
 }
 
-void hfsplus_ext_write_extent(struct inode *inode)
+int hfsplus_ext_write_extent(struct inode *inode)
 {
+	int res;
+
 	mutex_lock(&HFSPLUS_I(inode)->extents_lock);
-	hfsplus_ext_write_extent_locked(inode);
+	res = hfsplus_ext_write_extent_locked(inode);
 	mutex_unlock(&HFSPLUS_I(inode)->extents_lock);
+
+	return res;
 }
 
 static inline int __hfsplus_ext_read_extent(struct hfs_find_data *fd,
@@ -474,7 +482,9 @@ out:
 
 insert_extent:
 	dprint(DBG_EXTENT, "insert new extent\n");
-	hfsplus_ext_write_extent_locked(inode);
+	res = hfsplus_ext_write_extent_locked(inode);
+	if (res)
+		goto out;
 
 	memset(hip->cached_extents, 0, sizeof(hfsplus_extent_rec));
 	hip->cached_extents[0].start_block = cpu_to_be32(start);
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index d685752..0bebf74 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -374,7 +374,7 @@ extern const struct file_operations hfsplus_dir_operations;
 
 /* extents.c */
 int hfsplus_ext_cmp_key(const hfsplus_btree_key *, const hfsplus_btree_key *);
-void hfsplus_ext_write_extent(struct inode *);
+int hfsplus_ext_write_extent(struct inode *);
 int hfsplus_get_block(struct inode *, sector_t, struct buffer_head *, int);
 int hfsplus_free_fork(struct super_block *, u32,
 		struct hfsplus_fork_raw *, int);
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 07a0502..0b41f4c 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -135,9 +135,13 @@ static int hfsplus_system_write_inode(struct inode *inode)
 static int hfsplus_write_inode(struct inode *inode,
 		struct writeback_control *wbc)
 {
+	int err;
+
 	dprint(DBG_INODE, "hfsplus_write_inode: %lu\n", inode->i_ino);
 
-	hfsplus_ext_write_extent(inode);
+	err = hfsplus_ext_write_extent(inode);
+	if (err)
+		return err;
 
 	if (inode->i_ino >= HFSPLUS_FIRSTUSER_CNID ||
 	    inode->i_ino == HFSPLUS_ROOT_CNID)
-- 
1.7.4.1

Re: [PATCH v2 0/2] hfsplus: add error checking for hfs_find_init()

[Christoph Hellwig]

Thanks a lot Alexey,

I've put both patches in.