[PATCH] hugetlbfs: fix error path in module_init

[PATCH] hugetlbfs: fix error path in module_init

[Akinobu Mita]

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

Cc: William Irwin <wli@holomorphy.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

---
 fs/hugetlbfs/inode.c |   22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

Index: 2.6-mm/fs/hugetlbfs/inode.c
===================================================================
--- 2.6-mm.orig/fs/hugetlbfs/inode.c
+++ 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;
 }
 

Re: [PATCH] hugetlbfs: fix error path in module_init

[Alexey Dobriyan]

On 4/28/07, Akinobu Mita <akinobu.mita@gmail.com> wrote:
> 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?

Re: [PATCH] hugetlbfs: fix error path in module_init

[Akinobu Mita]

On Sat, Apr 28, 2007 at 11:44:43AM +0400, Alexey Dobriyan wrote:
> On 4/28/07, Akinobu Mita <akinobu.mita@gmail.com> wrote:
> >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().

Re: [PATCH] hugetlbfs: fix error path in module_init

[William Lee Irwin III]

On Sat, Apr 28, 2007 at 11:44:43AM +0400, Alexey Dobriyan wrote:
> On 4/28/07, Akinobu Mita <akinobu.mita@gmail.com> wrote:
> >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

Re: [PATCH] hugetlbfs: fix error path in module_init

[William Lee Irwin III]

On Sat, Apr 28, 2007 at 04:19:23PM +0900, Akinobu Mita wrote:
> init_hugetlbfs_fs() needs to unregister hugetlbfs
> when kern_mount() returns error.
> Cc: William Irwin <wli@holomorphy.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

This patch resolves a clear failure to error check.

Acked-by: William Irwin <bill.irwin@oracle.com>

I'll work out how to do anything with git and point people at a tree
shortly.


-- wli

Re: [PATCH] hugetlbfs: fix error path in module_init

[Akinobu Mita]

On Sat, Apr 28, 2007 at 07:50:39AM -0700, William Lee Irwin III wrote:
> On Sat, Apr 28, 2007 at 04:19:23PM +0900, Akinobu Mita wrote:
> > init_hugetlbfs_fs() needs to unregister hugetlbfs
> > when kern_mount() returns error.
> > Cc: William Irwin <wli@holomorphy.com>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> 
> 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().

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

[Akinobu Mita]

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

Cc: William Irwin <wli@holomorphy.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

Index: 2.6-mm/fs/hugetlbfs/inode.c
===================================================================
--- 2.6-mm.orig/fs/hugetlbfs/inode.c
+++ 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);
 

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

[William Lee Irwin III]

On Sun, Apr 29, 2007 at 01:29:48AM +0900, Akinobu Mita wrote:
> If hugetlbfs module_init() fails, hugetlbfs_vfsmount
> is not initialized and shmget() with SHM_HUGETLB flag will
> cause NULL pointer dereference.
> Cc: William Irwin <wli@holomorphy.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Index: 2.6-mm/fs/hugetlbfs/inode.c
> ===================================================================
> --- 2.6-mm.orig/fs/hugetlbfs/inode.c
> +++ 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);

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.

Acked-by: William Irwin <bill.irwin@oracle.com>


-- wli