Skip to contentSkip to conversation

hugetlbfs: fix error path in module_init

PATCH In review Akinobu Mita 3 participants 8 messages Apr 28, 2007 HugeTLB

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 hugetlbfs: fix error path in module_init

init_hugetlbfs_fs() needs to unregister hugetlbfs
when kern_mount() returns error.

---
 fs/hugetlbfs/inode.c |   22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)
fs/hugetlbfs/inode.c +11 -8
Index: 2.6-mm/fs/hugetlbfs/inode.c
===================================================================
@@ -804,20 +804,23 @@ static int __init init_hugetlbfs_fs(void
 
 	error = register_filesystem(&hugetlbfs_fs_type);
 	if (error)
-		goto out;
+		goto out_cache;
 
 	vfsmount = kern_mount(&hugetlbfs_fs_type);
 
-	if (!IS_ERR(vfsmount)) {
-		hugetlbfs_vfsmount = vfsmount;
-		return 0;
+	if (IS_ERR(vfsmount)) {
+		error = PTR_ERR(vfsmount);
+		goto out_fs;
 	}
+	hugetlbfs_vfsmount = vfsmount;
 
-	error = PTR_ERR(vfsmount);
+	return 0;
+
+out_fs:
+	unregister_filesystem(&hugetlbfs_fs_type);
+out_cache:
+	kmem_cache_destroy(hugetlbfs_inode_cachep);
 
- out:
-	if (error)
-		kmem_cache_destroy(hugetlbfs_inode_cachep);
 	return error;
 }
 
#1 lore raw
Quoted 2 lines · Akinobu Mita

init_hugetlbfs_fs() needs to unregister hugetlbfs
when kern_mount() returns error.

HUGETLBFS is bool, so __init function should panic more and __exit function should be removed. Or someone is planning making it tristate?

#2 lore raw
Quoted 7 lines · Alexey Dobriyan
Quoted 2 lines · Akinobu Mita

init_hugetlbfs_fs() needs to unregister hugetlbfs
when kern_mount() returns error.

HUGETLBFS is bool, so __init function should panic more and __exit function should be removed. Or someone is planning making it tristate?

Seems reasonable to make it panic. Because hugetlbfs_vfsmount is not initialized when error happen, and shmget() (ipc/shm.c) can cause NULL pointer dereference by hugetlb_zero_setup().

Or we can add NULL check for hugetlbfs_vfsmount in hugetlbfs_vfsmount().

#3 lore raw
In reply to Alexey Dobriyan
Quoted 6 lines · Alexey Dobriyan
Quoted 2 lines · Akinobu Mita

init_hugetlbfs_fs() needs to unregister hugetlbfs
when kern_mount() returns error.

HUGETLBFS is bool, so __init function should panic more and __exit function should be removed. Or someone is planning making it tristate?

It'd be nice for it to be normal enough to go tristate, but there isn't going to be any effort expended directly to that end. Might as well remove the exit function. Probably mostly pointless to try to avoid panicking, but I'd not bother with new panics as a simplification just because panicking is somewhat rude.

- wli

#5 lore raw
Quoted 7 lines · William Lee Irwin III
Quoted 4 lines · Akinobu Mita

init_hugetlbfs_fs() needs to unregister hugetlbfs when kern_mount() returns error.

This patch resolves a clear failure to error check.

I reconsider this patch. It was wrong. Because it is overkill to unregister filesystem just for hugetlbfs_vfsmount mount failure. Because hugetlbfs_vfsmount is only used for shumget() system call and also there is some race condition between register_filesystem() and unregister_filesystem(). (it's not likely happen though)

So what we can do for hugetlbfs module_init failure is adding NULL check for hugetlbfs_vfsmount in hugetlb_zero_setup().

#6 lore raw

PATCH 1/1 hugetlbfs: add NULL check in hugetlb_zero_setup()

If hugetlbfs module_init() fails, hugetlbfs_vfsmount is not initialized and shmget() with SHM_HUGETLB flag will cause NULL pointer dereference.

fs/hugetlbfs/inode.c +3 -0
Index: 2.6-mm/fs/hugetlbfs/inode.c
===================================================================
@@ -740,6 +740,9 @@ struct file *hugetlb_zero_setup(size_t s
 	char buf[16];
 	static atomic_t counter;
 
+	if (!hugetlbfs_vfsmount)
+		return ERR_PTR(-ENOENT);
+
 	if (!can_do_hugetlb_shm())
 		return ERR_PTR(-EPERM);
 
Acked-by William Irwin on v1 1/1
#7 lore raw

Re: [PATCH] hugetlbfs: add NULL check in hugetlb_zero_setup()

Quoted 10 lines · Akinobu Mita

If hugetlbfs module_init() fails, hugetlbfs_vfsmount is not initialized and shmget() with SHM_HUGETLB flag will cause NULL pointer dereference.

Index: 2.6-mm/fs/hugetlbfs/inode.c
===================================================================

fs/hugetlbfs/inode.c struct file *hugetlb_zero_setup(size_t s · line 747

Quoted 8 lines
fs/hugetlbfs/inode.c +3 -0
@@ -740,6 +740,9 @@ struct file *hugetlb_zero_setup(size_t s
 	char buf[16];
 	static atomic_t counter;
 
+	if (!hugetlbfs_vfsmount)
+		return ERR_PTR(-ENOENT);
+
 	if (!can_do_hugetlb_shm())
 		return ERR_PTR(-EPERM);

Putting some thought into this, the failure to set up the vfsmount for shm should be reported noisily, the failure to register the filesystem should be noticed so as not to oops later in the init function (I guess one could panic() if he wanted to), and the attempt at kern_mount() should be conditional on SysV IPC.

I'll take this check, which should be made no matter what, and do a patch for the init function along the lines described above if you don't do it yourself first. I don't need the patch credits, so feel free to grab the free patch line if you want it.

-- wli