[PATCH] hugetlbfs: add err code in initializing module

[PATCH] hugetlbfs: add err code in initializing module

[Hillf Danton]

Error code is added if fail to create inode kmem cache, and newly registered
hugetlb FS is unregistered if fail to mount, both for unlikely corner cases.

--- a/fs/hugetlbfs/inode.c	Sun Mar 11 12:46:38 2012
+++ b/fs/hugetlbfs/inode.c	Sun Mar 11 12:49:28 2012
@@ -1000,6 +1000,7 @@ static int __init init_hugetlbfs_fs(void
 	hugetlbfs_inode_cachep = kmem_cache_create("hugetlbfs_inode_cache",
 					sizeof(struct hugetlbfs_inode_info),
 					0, 0, init_once);
+	error = -ENOMEM;
 	if (hugetlbfs_inode_cachep == NULL)
 		goto out2;

@@ -1015,6 +1016,7 @@ static int __init init_hugetlbfs_fs(void
 	}

 	error = PTR_ERR(vfsmount);
+	unregister_filesystem(&hugetlbfs_fs_type);

  out:
 	if (error)
--

Re: [PATCH] hugetlbfs: add err code in initializing module

[Hillf Danton]

Error code is added if fail to create inode kmem cache, and newly registered
hugetlb FS is unregistered if fail to mount, both for unlikely corner cases.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---

--- a/fs/hugetlbfs/inode.c	Sun Mar 11 12:46:38 2012
+++ b/fs/hugetlbfs/inode.c	Sun Mar 11 12:49:28 2012
@@ -1000,6 +1000,7 @@ static int __init init_hugetlbfs_fs(void
 	hugetlbfs_inode_cachep = kmem_cache_create("hugetlbfs_inode_cache",
 					sizeof(struct hugetlbfs_inode_info),
 					0, 0, init_once);
+	error = -ENOMEM;
 	if (hugetlbfs_inode_cachep == NULL)
 		goto out2;

@@ -1015,6 +1016,7 @@ static int __init init_hugetlbfs_fs(void
 	}

 	error = PTR_ERR(vfsmount);
+	unregister_filesystem(&hugetlbfs_fs_type);

  out:
 	if (error)
--

Re: [PATCH] hugetlbfs: add err code in initializing module

[David Rientjes]

On Sun, 11 Mar 2012, Hillf Danton wrote:

> --- a/fs/hugetlbfs/inode.c	Sun Mar 11 12:46:38 2012
> +++ b/fs/hugetlbfs/inode.c	Sun Mar 11 12:49:28 2012
> @@ -1000,6 +1000,7 @@ static int __init init_hugetlbfs_fs(void
>  	hugetlbfs_inode_cachep = kmem_cache_create("hugetlbfs_inode_cache",
>  					sizeof(struct hugetlbfs_inode_info),
>  					0, 0, init_once);
> +	error = -ENOMEM;
>  	if (hugetlbfs_inode_cachep == NULL)
>  		goto out2;
> 

The patch looks good, however, we typically do things like

	error = -ENOMEM;
	hugetlbfs_inode_cachep = kmem_cache_create(...);
	if (!hugetlbfs_inode_cachep)
		...

instead.  Sometimes people grep to see if a call a function that can fail, 
such as kmem_cache_create(), is followed up by a check for NULL and this 
would otherwise fail.

Re: [PATCH] hugetlbfs: add err code in initializing module

[Hillf Danton]

On Mon, Mar 12, 2012 at 4:25 AM, David Rientjes <rientjes@google.com> wrote:
>
> The patch looks good, however, we typically do things like
>
>        error = -ENOMEM;
>        hugetlbfs_inode_cachep = kmem_cache_create(...);
>        if (!hugetlbfs_inode_cachep)
>                ...
>
> instead.  Sometimes people grep to see if a call a function that can fail,
> such as kmem_cache_create(), is followed up by a check for NULL and this
> would otherwise fail.
>
>
It is updated, thanks David.

-hd
===cut here===
From: Hillf Danton <dhillf@gmail.com>
Subject: [PATCH] hugetlbfs: add err code in initilizing module

Error code is added if fail to create inode kmem cache, and newly registered
hugetlb FS is unregistered if fail to mount, both for unlikely corner cases.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---

--- a/fs/hugetlbfs/inode.c	Sun Mar 11 12:46:38 2012
+++ b/fs/hugetlbfs/inode.c	Mon Mar 12 19:52:16 2012
@@ -997,6 +997,7 @@ static int __init init_hugetlbfs_fs(void
 	if (error)
 		return error;

+	error = -ENOMEM;
 	hugetlbfs_inode_cachep = kmem_cache_create("hugetlbfs_inode_cache",
 					sizeof(struct hugetlbfs_inode_info),
 					0, 0, init_once);
@@ -1015,6 +1016,7 @@ static int __init init_hugetlbfs_fs(void
 	}

 	error = PTR_ERR(vfsmount);
+	unregister_filesystem(&hugetlbfs_fs_type);

  out:
 	if (error)
--

Re: [PATCH] hugetlbfs: add err code in initializing module

[David Rientjes]

On Mon, 12 Mar 2012, Hillf Danton wrote:

> From: Hillf Danton <dhillf@gmail.com>
> Subject: [PATCH] hugetlbfs: add err code in initilizing module
> 
> Error code is added if fail to create inode kmem cache, and newly registered
> hugetlb FS is unregistered if fail to mount, both for unlikely corner cases.
> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>

Acked-by: David Rientjes <rientjes@google.com>

> ---
> 
> --- a/fs/hugetlbfs/inode.c	Sun Mar 11 12:46:38 2012
> +++ b/fs/hugetlbfs/inode.c	Mon Mar 12 19:52:16 2012
> @@ -997,6 +997,7 @@ static int __init init_hugetlbfs_fs(void
>  	if (error)
>  		return error;
> 
> +	error = -ENOMEM;
>  	hugetlbfs_inode_cachep = kmem_cache_create("hugetlbfs_inode_cache",
>  					sizeof(struct hugetlbfs_inode_info),
>  					0, 0, init_once);
> @@ -1015,6 +1016,7 @@ static int __init init_hugetlbfs_fs(void
>  	}
> 
>  	error = PTR_ERR(vfsmount);
> +	unregister_filesystem(&hugetlbfs_fs_type);
> 
>   out:
>  	if (error)
> --
> 

Re: [PATCH] hugetlbfs: add err code in initializing module

[Andrew Morton]

On Mon, 12 Mar 2012 20:01:43 +0800
Hillf Danton <dhillf@gmail.com> wrote:

> On Mon, Mar 12, 2012 at 4:25 AM, David Rientjes <rientjes@google.com> wrote:
> >
> > The patch looks good, however, we typically do things like
> >
> > __ __ __ __error = -ENOMEM;
> > __ __ __ __hugetlbfs_inode_cachep = kmem_cache_create(...);
> > __ __ __ __if (!hugetlbfs_inode_cachep)
> > __ __ __ __ __ __ __ __...
> >
> > instead. __Sometimes people grep to see if a call a function that can fail,
> > such as kmem_cache_create(), is followed up by a check for NULL and this
> > would otherwise fail.
> >
> >
> It is updated, thanks David.
> 
> -hd
> ===cut here===
> From: Hillf Danton <dhillf@gmail.com>
> Subject: [PATCH] hugetlbfs: add err code in initilizing module
> 
> Error code is added if fail to create inode kmem cache, and newly registered
> hugetlb FS is unregistered if fail to mount, both for unlikely corner cases.
> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
> 
> --- a/fs/hugetlbfs/inode.c	Sun Mar 11 12:46:38 2012
> +++ b/fs/hugetlbfs/inode.c	Mon Mar 12 19:52:16 2012
> @@ -997,6 +997,7 @@ static int __init init_hugetlbfs_fs(void
>  	if (error)
>  		return error;
> 
> +	error = -ENOMEM;
>  	hugetlbfs_inode_cachep = kmem_cache_create("hugetlbfs_inode_cache",
>  					sizeof(struct hugetlbfs_inode_info),
>  					0, 0, init_once);
> @@ -1015,6 +1016,7 @@ static int __init init_hugetlbfs_fs(void
>  	}
> 
>  	error = PTR_ERR(vfsmount);
> +	unregister_filesystem(&hugetlbfs_fs_type);
> 
>   out:
>  	if (error)

And this, yes?

remove unneeded test of `error'

--- a/fs/hugetlbfs/inode.c~hugetlbfs-return-error-code-when-initializing-module-fix
+++ a/fs/hugetlbfs/inode.c
@@ -1035,8 +1035,7 @@ static int __init init_hugetlbfs_fs(void
 	unregister_filesystem(&hugetlbfs_fs_type);
 
  out:
-	if (error)
-		kmem_cache_destroy(hugetlbfs_inode_cachep);
+	kmem_cache_destroy(hugetlbfs_inode_cachep);
  out2:
 	bdi_destroy(&hugetlbfs_backing_dev_info);
 	return error;
_

Re: [PATCH] hugetlbfs: add err code in initializing module

[Al Viro]

On Sun, Mar 11, 2012 at 01:09:59PM +0800, Hillf Danton wrote:
> Error code is added if fail to create inode kmem cache, and newly registered
> hugetlb FS is unregistered if fail to mount, both for unlikely corner cases.
> 
> --- a/fs/hugetlbfs/inode.c	Sun Mar 11 12:46:38 2012
> +++ b/fs/hugetlbfs/inode.c	Sun Mar 11 12:49:28 2012
> @@ -1000,6 +1000,7 @@ static int __init init_hugetlbfs_fs(void
>  	hugetlbfs_inode_cachep = kmem_cache_create("hugetlbfs_inode_cache",
>  					sizeof(struct hugetlbfs_inode_info),
>  					0, 0, init_once);
> +	error = -ENOMEM;
>  	if (hugetlbfs_inode_cachep == NULL)
>  		goto out2;
> 
> @@ -1015,6 +1016,7 @@ static int __init init_hugetlbfs_fs(void
>  	}
> 
>  	error = PTR_ERR(vfsmount);
> +	unregister_filesystem(&hugetlbfs_fs_type);

Bloody bad idea, that...  Realistically, the proper action on failure here
(as well as in sock_init(), etc.) is panic().  If we fail to OOM that early,
the box is doomed anyway.

Note that unregister_filesystem() in module init is *always* wrong; it's not
an issue here (it's done too early to care about and realistically the box
is not going anywhere - it'll panic when attempt to exec /sbin/init fails,
if not earlier), but it's a damn bad example.

Consider a normal fs module.  Somebody loads it and in parallel with that
we get a mount attempt on that fs type.  It comes between register and
failure exits that causes unregister; at that point we are screwed since
grabbing a reference to module as done by mount is enough to prevent
exit, but not to prevent the failure of init.  As the result, module will
get freed when init fails, mounted fs of that type be damned.

Again, this is not an issue here, but we had a bunch of real races of that
sort - the fixes for those just went in.  We _still_ have b0rken ones -
e.g. fuse and gfs2 are FUBAR in that respect and there's not a damn thing
we can do with the current API - anything that registers several fs types
can fail on the last register_filesystem() and that's it.

BTW, why in the name of everything unholy does hugetlbfs have module_exit()?
	* it's not a module
	* it does kern_mount() in module_init, which pins it down anyway
	* even if it wouldn't bother with kern_mount() (that can be worked
around, by switching to simple_pin_fs() done on demand), we would still have
the code in core kernel calling the functions in there.  Good luck working
around _that_ in a race-free way...

Re: [PATCH] hugetlbfs: add err code in initializing module

[Hillf Danton]

On Thu, Mar 22, 2012 at 8:33 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Mar 11, 2012 at 01:09:59PM +0800, Hillf Danton wrote:
>> Error code is added if fail to create inode kmem cache, and newly registered
>> hugetlb FS is unregistered if fail to mount, both for unlikely corner cases.
>>
>> --- a/fs/hugetlbfs/inode.c    Sun Mar 11 12:46:38 2012
>> +++ b/fs/hugetlbfs/inode.c    Sun Mar 11 12:49:28 2012
>> @@ -1000,6 +1000,7 @@ static int __init init_hugetlbfs_fs(void
>>       hugetlbfs_inode_cachep = kmem_cache_create("hugetlbfs_inode_cache",
>>                                       sizeof(struct hugetlbfs_inode_info),
>>                                       0, 0, init_once);
>> +     error = -ENOMEM;
>>       if (hugetlbfs_inode_cachep == NULL)
>>               goto out2;
>>
>> @@ -1015,6 +1016,7 @@ static int __init init_hugetlbfs_fs(void
>>       }
>>
>>       error = PTR_ERR(vfsmount);
>> +     unregister_filesystem(&hugetlbfs_fs_type);
>
> Bloody bad idea, that...  Realistically, the proper action on failure here
> (as well as in sock_init(), etc.) is panic().  If we fail to OOM that early,
> the box is doomed anyway.
>
> Note that unregister_filesystem() in module init is *always* wrong; it's not
> an issue here (it's done too early to care about and realistically the box
> is not going anywhere - it'll panic when attempt to exec /sbin/init fails,
> if not earlier), but it's a damn bad example.
>
Thanks for your review, Al.

Due to bad idea and bad example, please issue Nack, then the patch
that was not well prepared will be dropped.

-hd

Re: [PATCH] hugetlbfs: add err code in initializing module

[David Rientjes]

On Thu, 22 Mar 2012, Hillf Danton wrote:

> Due to bad idea and bad example, please issue Nack, then the patch
> that was not well prepared will be dropped.
> 

The patch is still in linux-next as of yesterday's tree, so please propose 
a follow-up patch that removes the unregister_filesystem() since the 
setting of "error" when kmem_cache_create() is still correct.

Re: [PATCH] hugetlbfs: add err code in initializing module

[Hillf Danton]

On Wed, Mar 28, 2012 at 3:24 PM, David Rientjes <rientjes@google.com> wrote:
>
> The patch is still in linux-next as of yesterday's tree, so please propose
> a follow-up patch that removes the unregister_filesystem() since the
> setting of "error" when kmem_cache_create() is still correct.

Ah the window is still open.

Thanks
-hd

===cut here===
From: Hillf Danton <dhillf@gmail.com>
Subject: [PATCH] hugetlbfs: remove unregister_filesystem when
initializing module

It is introduced by

	commit	d1d5e05ffdc110021ae7937802e88ae0d223dcdc
	hugetlbfs: return error code when initializing module

but as Al pointed out, is an example of bad idea.

Quoted comments from Al.
Note that unregister_filesystem() in module init is *always* wrong; it's not
an issue here (it's done too early to care about and realistically the box
is not going anywhere - it'll panic when attempt to exec /sbin/init fails,
if not earlier), but it's a damn bad example.

Consider a normal fs module.  Somebody loads it and in parallel with that
we get a mount attempt on that fs type.  It comes between register and
failure exits that causes unregister; at that point we are screwed since
grabbing a reference to module as done by mount is enough to prevent
exit, but not to prevent the failure of init.  As the result, module will
get freed when init fails, mounted fs of that type be damned.
end of quote

So remove it.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---

--- a/fs/hugetlbfs/inode.c	Wed Mar 28 20:15:24 2012
+++ b/fs/hugetlbfs/inode.c	Wed Mar 28 20:17:46 2012
@@ -1032,7 +1032,6 @@ static int __init init_hugetlbfs_fs(void
 	}

 	error = PTR_ERR(vfsmount);
-	unregister_filesystem(&hugetlbfs_fs_type);

  out:
 	kmem_cache_destroy(hugetlbfs_inode_cachep);
--