[PATCH] hfsplus: avoid crash on failed block map free

[PATCH] hfsplus: avoid crash on failed block map free

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

If the read fails we kmap an error code. This doesn't end well. Instead
print a critical error and pray. This mirrors the rest of the fs behaviour
with critical error cases.

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 fs/hfsplus/bitmap.c |    8 ++++++++
 1 file changed, 8 insertions(+)


diff --git a/fs/hfsplus/bitmap.c b/fs/hfsplus/bitmap.c
index 4cfbe2e..5eeefee 100644
--- a/fs/hfsplus/bitmap.c
+++ b/fs/hfsplus/bitmap.c
@@ -182,6 +182,8 @@ int hfsplus_block_free(struct super_block *sb, u32 offset, u32 count)
 	mapping = sbi->alloc_file->i_mapping;
 	pnr = offset / PAGE_CACHE_BITS;
 	page = read_mapping_page(mapping, pnr, NULL);
+	if (IS_ERR(page))
+		goto kaboom;
 	pptr = kmap(page);
 	curr = pptr + (offset & (PAGE_CACHE_BITS - 1)) / 32;
 	end = pptr + PAGE_CACHE_BITS / 32;
@@ -214,6 +216,8 @@ int hfsplus_block_free(struct super_block *sb, u32 offset, u32 count)
 		set_page_dirty(page);
 		kunmap(page);
 		page = read_mapping_page(mapping, ++pnr, NULL);
+		if (IS_ERR(page))
+			goto kaboom;
 		pptr = kmap(page);
 		curr = pptr;
 		end = pptr + PAGE_CACHE_BITS / 32;
@@ -231,5 +235,9 @@ out:
 	hfsplus_mark_mdb_dirty(sb);
 	mutex_unlock(&sbi->alloc_mutex);
 
+kaboom:
+	printk(KERN_CRIT "hfsplus: unable to mark blocks free: error %ld\n",
+			PTR_ERR(page));
+	mutex_unlock(&sbi->alloc_mutex);
 	return 0;
 }

Re: [PATCH] hfsplus: avoid crash on failed block map free

[Vyacheslav Dubeyko]

Hi,

On Oct 30, 2012, at 5:23 PM, Alan Cox wrote:

> From: Alan Cox <alan@linux.intel.com>
> 
> If the read fails we kmap an error code. This doesn't end well. Instead
> print a critical error and pray. This mirrors the rest of the fs behaviour
> with critical error cases.
> 
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> ---
> 
> fs/hfsplus/bitmap.c |    8 ++++++++
> 1 file changed, 8 insertions(+)
> 
> 
> diff --git a/fs/hfsplus/bitmap.c b/fs/hfsplus/bitmap.c
> index 4cfbe2e..5eeefee 100644
> --- a/fs/hfsplus/bitmap.c
> +++ b/fs/hfsplus/bitmap.c
> @@ -182,6 +182,8 @@ int hfsplus_block_free(struct super_block *sb, u32 offset, u32 count)
> 	mapping = sbi->alloc_file->i_mapping;
> 	pnr = offset / PAGE_CACHE_BITS;
> 	page = read_mapping_page(mapping, pnr, NULL);
> +	if (IS_ERR(page))
> +		goto kaboom;
> 	pptr = kmap(page);
> 	curr = pptr + (offset & (PAGE_CACHE_BITS - 1)) / 32;
> 	end = pptr + PAGE_CACHE_BITS / 32;
> @@ -214,6 +216,8 @@ int hfsplus_block_free(struct super_block *sb, u32 offset, u32 count)
> 		set_page_dirty(page);
> 		kunmap(page);
> 		page = read_mapping_page(mapping, ++pnr, NULL);
> +		if (IS_ERR(page))
> +			goto kaboom;
> 		pptr = kmap(page);
> 		curr = pptr;
> 		end = pptr + PAGE_CACHE_BITS / 32;
> @@ -231,5 +235,9 @@ out:
> 	hfsplus_mark_mdb_dirty(sb);
> 	mutex_unlock(&sbi->alloc_mutex);
> 
> +kaboom:
> +	printk(KERN_CRIT "hfsplus: unable to mark blocks free: error %ld\n",
> +			PTR_ERR(page));
> +	mutex_unlock(&sbi->alloc_mutex);

Your patch is good. But what about returning an error code (for example, -EIO)? Unfortunately, as I can see, the code that call hfsplus_block_free() doesn't analyze return code. I think that it is not good.

With the best regards,
Vyacheslav Dubeyko. 

> 	return 0;
> }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hfsplus: avoid crash on failed block map free

[Alan Cox]

> Your patch is good. But what about returning an error code (for example, -EIO)? Unfortunately, as I can see, the code that call hfsplus_block_free() doesn't analyze return code. I think that it is not good.

Actually handling the error nicely is a major rework of the file system.
If you want to do that send patches but you'll need to fix up another ton
of places.

The patch fixes the crash to the same level of not crashing the rest of
that file system has.

Alan

Re: [PATCH] hfsplus: avoid crash on failed block map free

[Vyacheslav Dubeyko]

On Nov 3, 2012, at 7:27 PM, Alan Cox wrote:

>> Your patch is good. But what about returning an error code (for example, -EIO)? Unfortunately, as I can see, the code that call hfsplus_block_free() doesn't analyze return code. I think that it is not good.
> 
> Actually handling the error nicely is a major rework of the file system.
> If you want to do that send patches but you'll need to fix up another ton
> of places.
> 

Ok. I see.

I simply suggest to add in this patch of error code return. I can rework the rest by myself. :-)

With the best regards,
Vyacheslav Dubeyko.

> The patch fixes the crash to the same level of not crashing the rest of
> that file system has.
> 
> Alan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html