Skip to contentSkip to conversation

hfsplus: avoid crash on failed block map free

PATCH In review Alan Cox 2 participants 4 messages Oct 30, 2012 – Nov 3, 2012

Subsystems (2)
VFS Hfsplus filesystem
lore
Share & tools
lore

Alan Cox

author
#0 lore raw

PATCH 1/1 hfsplus: avoid crash on failed block map free

From: Alan Cox <[email protected]>

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 <[email protected]>

---

 fs/hfsplus/bitmap.c |    8 ++++++++
 1 file changed, 8 insertions(+)
fs/hfsplus/bitmap.c +8 -0
index 4cfbe2e..5eeefee 100644
@@ -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;
 }
maintainer
#1 lore raw

Hi,

Quoted 36 lines · Alan Cox

From: Alan Cox <[email protected]>

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 <[email protected]>

---

fs/hfsplus/bitmap.c | 8 ++++++++

1 file changed, 8 insertions(+)
fs/hfsplus/bitmap.c +4 -0
index 4cfbe2e..5eeefee 100644
@@ -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;

fs/hfsplus/bitmap.c out: · line 241

Quoted 7 lines
fs/hfsplus/bitmap.c +4 -0
@@ -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.

Quoted 7 lines
fs/hfsplus/bitmap.c +0 -1
 	return 0;
 }
 
--

To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#2 lore raw
Quoted 1 line

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

maintainer
#3 lore raw
Quoted 6 lines · Alan Cox
Quoted 1 line

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.

Quoted 8 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html