[patch] pipe: Don't oops when pipe filesystem isn't mounted

[patch] pipe: Don't oops when pipe filesystem isn't mounted

[Chuck Ebbert]

Prevent oops when an app tries to create a pipe while pipefs
is not mounted.

Signed-off-by: Chuck Ebbert <76306.1226@compuserve.com>

--- 2.6.19.1-pre1-32.orig/fs/pipe.c
+++ 2.6.19.1-pre1-32/fs/pipe.c
@@ -839,9 +839,11 @@ static struct dentry_operations pipefs_d
 
 static struct inode * get_pipe_inode(void)
 {
-	struct inode *inode = new_inode(pipe_mnt->mnt_sb);
+	struct inode *inode = NULL;
 	struct pipe_inode_info *pipe;
 
+	if (pipe_mnt)
+		inode = new_inode(pipe_mnt->mnt_sb);
 	if (!inode)
 		goto fail_inode;
 
-- 
Chuck

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Andrew Morton]

On Mon, 11 Dec 2006 03:27:37 -0500
Chuck Ebbert <76306.1226@compuserve.com> wrote:

> Prevent oops when an app tries to create a pipe while pipefs
> is not mounted.
> 
> Signed-off-by: Chuck Ebbert <76306.1226@compuserve.com>
> 
> --- 2.6.19.1-pre1-32.orig/fs/pipe.c
> +++ 2.6.19.1-pre1-32/fs/pipe.c
> @@ -839,9 +839,11 @@ static struct dentry_operations pipefs_d
>  
>  static struct inode * get_pipe_inode(void)
>  {
> -	struct inode *inode = new_inode(pipe_mnt->mnt_sb);
> +	struct inode *inode = NULL;
>  	struct pipe_inode_info *pipe;
>  
> +	if (pipe_mnt)
> +		inode = new_inode(pipe_mnt->mnt_sb);
>  	if (!inode)
>  		goto fail_inode;
>  

That's pretty lame.  It means that pipes just won't work, so people who are
using pipes in their initramfs setups will just get mysterious failures
running userspace on a crippled kernel.

I think the bug really is the running of populate_rootfs() before running
the initcalls, in init/main.c:init().  It's just more sensible to start
running userspace after the initcalls have been run.  Statically-linked
drivers which want to load firmware files will lose.  To fix that we'd need
a new callback.  It could be with a new linker section or perhaps simply a
notifier chain.

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Andrew Morton]

On Mon, 11 Dec 2006 00:55:57 -0800
Andrew Morton <akpm@osdl.org> wrote:

> I think the bug really is the running of populate_rootfs() before running
> the initcalls, in init/main.c:init().  It's just more sensible to start
> running userspace after the initcalls have been run.  Statically-linked
> drivers which want to load firmware files will lose.  To fix that we'd need
> a new callback.  It could be with a new linker section or perhaps simply a
> notifier chain.

hm, actually...  Add two new initcall levels, one for populate_rootfs() and
one for things which want to come after it (ie: drivers which want to
access the filesytem):

 #define core_initcall(fn)		__define_initcall("1",fn,1)
 #define core_initcall_sync(fn)		__define_initcall("1s",fn,1s)
 #define postcore_initcall(fn)		__define_initcall("2",fn,2)
 #define postcore_initcall_sync(fn)	__define_initcall("2s",fn,2s)
 #define arch_initcall(fn)		__define_initcall("3",fn,3)
 #define arch_initcall_sync(fn)		__define_initcall("3s",fn,3s)
 #define subsys_initcall(fn)		__define_initcall("4",fn,4)
 #define subsys_initcall_sync(fn)	__define_initcall("4s",fn,4s)
 #define fs_initcall(fn)		__define_initcall("5",fn,5)
 #define fs_initcall_sync(fn)		__define_initcall("5s",fn,5s)
 #define device_initcall(fn)		__define_initcall("6",fn,6)
 #define device_initcall_sync(fn)	__define_initcall("6s",fn,6s)
 #define late_initcall(fn)		__define_initcall("7",fn,7)
 #define late_initcall_sync(fn)		__define_initcall("7s",fn,7s)
+#define populate_rootfs_initcall ...
+#define post_populate_rootfs_initcall ...

?

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Al Viro]

On Mon, Dec 11, 2006 at 03:27:37AM -0500, Chuck Ebbert wrote:
> Prevent oops when an app tries to create a pipe while pipefs
> is not mounted.
> 
> Signed-off-by: Chuck Ebbert <76306.1226@compuserve.com>

That makes no sense at all.  pipe_mnt is not created by userland
mount; it's created by init_pipe_fs() and we'd bloody better
have it done before any applications get a chance to run.

Please, explain how the hell did you manage to get a perverted
config where that is not true.  In the meanwhile the patch is
NAKed.

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Al Viro]

On Mon, Dec 11, 2006 at 01:13:27AM -0800, Andrew Morton wrote:
> On Mon, 11 Dec 2006 00:55:57 -0800
> Andrew Morton <akpm@osdl.org> wrote:
> 
> > I think the bug really is the running of populate_rootfs() before running
> > the initcalls, in init/main.c:init().  It's just more sensible to start
> > running userspace after the initcalls have been run.  Statically-linked
> > drivers which want to load firmware files will lose.  To fix that we'd need
> > a new callback.  It could be with a new linker section or perhaps simply a
> > notifier chain.
> 
> hm, actually...  Add two new initcall levels, one for populate_rootfs() and
> one for things which want to come after it (ie: drivers which want to
> access the filesytem):

IMO we should just call pipe (and socket) initialization directly at
the same level as slab, task, dcache, etc.

This is basic stuff needed to get operational kernel.  We could try
to put that on an additional initcall level, but then we ought to
take the rest of basic setup there as well.

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Andrew Morton]

On Mon, 11 Dec 2006 09:21:30 +0000
Al Viro <viro@ftp.linux.org.uk> wrote:

> On Mon, Dec 11, 2006 at 01:13:27AM -0800, Andrew Morton wrote:
> > On Mon, 11 Dec 2006 00:55:57 -0800
> > Andrew Morton <akpm@osdl.org> wrote:
> > 
> > > I think the bug really is the running of populate_rootfs() before running
> > > the initcalls, in init/main.c:init().  It's just more sensible to start
> > > running userspace after the initcalls have been run.  Statically-linked
> > > drivers which want to load firmware files will lose.  To fix that we'd need
> > > a new callback.  It could be with a new linker section or perhaps simply a
> > > notifier chain.
> > 
> > hm, actually...  Add two new initcall levels, one for populate_rootfs() and
> > one for things which want to come after it (ie: drivers which want to
> > access the filesytem):
> 
> IMO we should just call pipe (and socket) initialization directly at
> the same level as slab, task, dcache, etc.

spose that would work.  But what other initcall-initialised things are not
yet available when populate_rootfs() runs?

<does  grep _initcall */*.c>
<wonders why anything works at all>

> This is basic stuff needed to get operational kernel.  We could try
> to put that on an additional initcall level, but then we ought to
> take the rest of basic setup there as well.

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Chuck Ebbert]

In-Reply-To: <20061211005557.04643a75.akpm@osdl.org>

On Mon, 11 Dec 2006 00:55:57 -0800, Andrew Morton wrote:

> > Prevent oops when an app tries to create a pipe while pipefs
> > is not mounted.
> 
> That's pretty lame.  It means that pipes just won't work, so people who are
> using pipes in their initramfs setups will just get mysterious failures
> running userspace on a crippled kernel.

I know, I just wanted to keep the issue alive. :)

> I think the bug really is the running of populate_rootfs() before running
> the initcalls, in init/main.c:init().  It's just more sensible to start
> running userspace after the initcalls have been run.  Statically-linked
> drivers which want to load firmware files will lose.  To fix that we'd need
> a new callback.  It could be with a new linker section or perhaps simply a
> notifier chain.

Why not create a new initcall category for things that must run before
early userspace?

-- 
MBTI: IXTP

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Al Viro]

On Mon, Dec 11, 2006 at 01:25:45AM -0800, Andrew Morton wrote:
> On Mon, 11 Dec 2006 09:21:30 +0000
> Al Viro <viro@ftp.linux.org.uk> wrote:
> 
> > On Mon, Dec 11, 2006 at 01:13:27AM -0800, Andrew Morton wrote:
> > > On Mon, 11 Dec 2006 00:55:57 -0800
> > > Andrew Morton <akpm@osdl.org> wrote:
> > > 
> > > > I think the bug really is the running of populate_rootfs() before running
> > > > the initcalls, in init/main.c:init().  It's just more sensible to start
> > > > running userspace after the initcalls have been run.  Statically-linked
> > > > drivers which want to load firmware files will lose.  To fix that we'd need
> > > > a new callback.  It could be with a new linker section or perhaps simply a
> > > > notifier chain.
> > > 
> > > hm, actually...  Add two new initcall levels, one for populate_rootfs() and
> > > one for things which want to come after it (ie: drivers which want to
> > > access the filesytem):
> > 
> > IMO we should just call pipe (and socket) initialization directly at
> > the same level as slab, task, dcache, etc.
> 
> spose that would work.  But what other initcall-initialised things are not
> yet available when populate_rootfs() runs?
> 
> <does  grep _initcall */*.c>
> <wonders why anything works at all>

Explain, please...

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Michael Tokarev]

Al Viro wrote:
> On Mon, Dec 11, 2006 at 03:27:37AM -0500, Chuck Ebbert wrote:
>> Prevent oops when an app tries to create a pipe while pipefs
>> is not mounted.
[]
> That makes no sense at all.  pipe_mnt is not created by userland
> mount; it's created by init_pipe_fs() and we'd bloody better
> have it done before any applications get a chance to run.
> 
> Please, explain how the hell did you manage to get a perverted
> config where that is not true.  In the meanwhile the patch is
> NAKed.

See for example http://marc.theaimsgroup.com/?t=116510390600001&r=1&w=2

Thanks.

/mjt

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Andrew Morton]

On Mon, 11 Dec 2006 09:33:15 +0000
Al Viro <viro@ftp.linux.org.uk> wrote:

> On Mon, Dec 11, 2006 at 01:25:45AM -0800, Andrew Morton wrote:
> > On Mon, 11 Dec 2006 09:21:30 +0000
> > Al Viro <viro@ftp.linux.org.uk> wrote:
> > 
> > > On Mon, Dec 11, 2006 at 01:13:27AM -0800, Andrew Morton wrote:
> > > > On Mon, 11 Dec 2006 00:55:57 -0800
> > > > Andrew Morton <akpm@osdl.org> wrote:
> > > > 
> > > > > I think the bug really is the running of populate_rootfs() before running
> > > > > the initcalls, in init/main.c:init().  It's just more sensible to start
> > > > > running userspace after the initcalls have been run.  Statically-linked
> > > > > drivers which want to load firmware files will lose.  To fix that we'd need
> > > > > a new callback.  It could be with a new linker section or perhaps simply a
> > > > > notifier chain.
> > > > 
> > > > hm, actually...  Add two new initcall levels, one for populate_rootfs() and
> > > > one for things which want to come after it (ie: drivers which want to
> > > > access the filesytem):
> > > 
> > > IMO we should just call pipe (and socket) initialization directly at
> > > the same level as slab, task, dcache, etc.
> > 
> > spose that would work.  But what other initcall-initialised things are not
> > yet available when populate_rootfs() runs?
> > 
> > <does  grep _initcall */*.c>
> > <wonders why anything works at all>
> 
> Explain, please...

- populate_rootfs() puts stuff into the filesystem

- we then run initcalls.

- an initcall runs /sbin/hotplug.

We're now running userspace before all the initcalls have been executed. 
Hence we're trying to run userspace when potentially none of "grep
_initcall */*.c" has been executed.  It isn't a kernel yet...

See http://marc.theaimsgroup.com/?l=linux-kernel&m=116510389000878&w=2

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Al Viro]

On Mon, Dec 11, 2006 at 01:47:27AM -0800, Andrew Morton wrote:
> - populate_rootfs() puts stuff into the filesystem
> 
> - we then run initcalls.
> 
> - an initcall runs /sbin/hotplug.
> 
> We're now running userspace before all the initcalls have been executed. 
> Hence we're trying to run userspace when potentially none of "grep
> _initcall */*.c" has been executed.  It isn't a kernel yet...

That's... arguable.  We certainly don't need lots and lots of initcalls
to be able to run userland code.  Which ones are missing in your opinion?

As for that example, I'd love to see specifics - which driver triggers
hotplug?  Presumably it happens from an initcall, so we also have something
fishy here...

Said that, I think that pipes should be initialized early.

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Andrew Morton]

On Mon, 11 Dec 2006 10:03:01 +0000
Al Viro <viro@ftp.linux.org.uk> wrote:

> On Mon, Dec 11, 2006 at 01:47:27AM -0800, Andrew Morton wrote:
> > - populate_rootfs() puts stuff into the filesystem
> > 
> > - we then run initcalls.
> > 
> > - an initcall runs /sbin/hotplug.
> > 
> > We're now running userspace before all the initcalls have been executed. 
> > Hence we're trying to run userspace when potentially none of "grep
> > _initcall */*.c" has been executed.  It isn't a kernel yet...
> 
> That's... arguable.  We certainly don't need lots and lots of initcalls
> to be able to run userland code.  Which ones are missing in your opinion?

I think we should aim to have as many subsystems ready to go as possible -
ideally all of them.  Right now we can potentially run userspace before
AIO, posix-timers, message-queues, BIO, networking, etc are ready to run.

It looks to be pretty easy to fix...

> As for that example, I'd love to see specifics - which driver triggers
> hotplug?  Presumably it happens from an initcall, so we also have something
> fishy here...

I don't know in this case - but firmware loading from a statically-linked
driver is a legit thing to do.

> Said that, I think that pipes should be initialized early.

Judging by the comment there, the only reason we prepare the rootfs prior
to running initcalls is for firmware.  So the sequence

	run initcalls
	populate rootfs
	run initcalls which want to access files

fixes everything?
	

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Al Viro]

On Mon, Dec 11, 2006 at 02:17:18AM -0800, Andrew Morton wrote:
> I think we should aim to have as many subsystems ready to go as possible -
> ideally all of them.  Right now we can potentially run userspace before
> AIO, posix-timers, message-queues, BIO, networking, etc are ready to run.
> 
> It looks to be pretty easy to fix...
> 
> > As for that example, I'd love to see specifics - which driver triggers
> > hotplug?  Presumably it happens from an initcall, so we also have something
> > fishy here...
> 
> I don't know in this case - but firmware loading from a statically-linked
> driver is a legit thing to do.

Umm... statically linked driver that might want firmware shouldn't precede
the subsystems unless something is seriously wrong with priorities...

IOW, I still wonder what's really going on - pipes are fs_initcall() and
any hardware stuff ought to be simple module_init().  So something fishy
is going on, regardless of anything else.

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Andrew Morton]

On Mon, 11 Dec 2006 02:17:18 -0800
Andrew Morton <akpm@osdl.org> wrote:

> > Said that, I think that pipes should be initialized early.
> 
> Judging by the comment there, the only reason we prepare the rootfs prior
> to running initcalls is for firmware.  So the sequence
> 
> 	run initcalls
> 	populate rootfs
> 	run initcalls which want to access files
> 
> fixes everything?

Like this...  The other part is hunting down all those drivers which want
to access the filesystem at init time.


 include/asm-generic/vmlinux.lds.h |    6 +++++-
 include/linux/init.h              |    5 +++++
 init/initramfs.c                  |    3 ++-
 init/main.c                       |    7 -------
 4 files changed, 12 insertions(+), 9 deletions(-)

diff -puN init/main.c~dont-run-userspace-until-initcalls-have-completed init/main.c
--- a/init/main.c~dont-run-userspace-until-initcalls-have-completed
+++ a/init/main.c
@@ -94,7 +94,6 @@ extern void pidmap_init(void);
 extern void prio_tree_init(void);
 extern void radix_tree_init(void);
 extern void free_initmem(void);
-extern void populate_rootfs(void);
 extern void driver_init(void);
 extern void prepare_namespace(void);
 #ifdef	CONFIG_ACPI
@@ -739,12 +738,6 @@ static int init(void * unused)
 
 	cpuset_init_smp();
 
-	/*
-	 * Do this before initcalls, because some drivers want to access
-	 * firmware files.
-	 */
-	populate_rootfs();
-
 	do_basic_setup();
 
 	/*
diff -puN include/linux/init.h~dont-run-userspace-until-initcalls-have-completed include/linux/init.h
--- a/include/linux/init.h~dont-run-userspace-until-initcalls-have-completed
+++ a/include/linux/init.h
@@ -115,6 +115,11 @@ extern void setup_arch(char **);
 #define device_initcall_sync(fn)	__define_initcall("6s",fn,6s)
 #define late_initcall(fn)		__define_initcall("7",fn,7)
 #define late_initcall_sync(fn)		__define_initcall("7s",fn,7s)
+#define populate_rootfs_initcall(fn)	__define_initcall("8",fn,8)
+#define populate_rootfs_initcall_sync(fn) __define_initcall("8s",fn,8s)
+#define rootfs_neeeded_initcall(fn)	__define_initcall("9",fn,9)
+#define rootfs_neeeded_initcall_sync(fn) __define_initcall("9s",fn,9s)
+
 
 #define __initcall(fn) device_initcall(fn)
 
diff -puN include/asm-generic/vmlinux.lds.h~dont-run-userspace-until-initcalls-have-completed include/asm-generic/vmlinux.lds.h
--- a/include/asm-generic/vmlinux.lds.h~dont-run-userspace-until-initcalls-have-completed
+++ a/include/asm-generic/vmlinux.lds.h
@@ -245,5 +245,9 @@
   	*(.initcall6.init)						\
   	*(.initcall6s.init)						\
   	*(.initcall7.init)						\
-  	*(.initcall7s.init)
+  	*(.initcall7s.init)						\
+  	*(.initcall8.init)						\
+  	*(.initcall8s.init)						\
+  	*(.initcall9.init)						\
+  	*(.initcall9s.init)
 
diff -puN init/initramfs.c~dont-run-userspace-until-initcalls-have-completed init/initramfs.c
--- a/init/initramfs.c~dont-run-userspace-until-initcalls-have-completed
+++ a/init/initramfs.c
@@ -526,7 +526,7 @@ static void __init free_initrd(void)
 
 #endif
 
-void __init populate_rootfs(void)
+static int __init populate_rootfs(void)
 {
 	char *err = unpack_to_rootfs(__initramfs_start,
 			 __initramfs_end - __initramfs_start, 0);
@@ -566,3 +566,4 @@ void __init populate_rootfs(void)
 	}
 #endif
 }
+populate_rootfs_initcall(populate_rootfs);
_

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Andrew Morton]

On Mon, 11 Dec 2006 10:22:07 +0000
Al Viro <viro@ftp.linux.org.uk> wrote:

> On Mon, Dec 11, 2006 at 02:17:18AM -0800, Andrew Morton wrote:
> > I think we should aim to have as many subsystems ready to go as possible -
> > ideally all of them.  Right now we can potentially run userspace before
> > AIO, posix-timers, message-queues, BIO, networking, etc are ready to run.
> > 
> > It looks to be pretty easy to fix...
> > 
> > > As for that example, I'd love to see specifics - which driver triggers
> > > hotplug?  Presumably it happens from an initcall, so we also have something
> > > fishy here...
> > 
> > I don't know in this case - but firmware loading from a statically-linked
> > driver is a legit thing to do.
> 
> Umm... statically linked driver that might want firmware shouldn't precede
> the subsystems unless something is seriously wrong with priorities...

There are plenty of drivers in there using subsys_initcall, arch_initcall,
postcore_initcall, core_initcall and even one pure_initcall.

Heaven knows why.  They're drivers :(

> IOW, I still wonder what's really going on - pipes are fs_initcall() and
> any hardware stuff ought to be simple module_init().  So something fishy
> is going on, regardless of anything else.

A heck of a lot of things can trigger an /sbin/hotplug run.  It could well
be that Andrew's driver didn't want to run hotplug at all, but the kernel
did it anwyay.  But as soon as the script appeared at /sbin/hotplug, and it
happened to use foo|bar: boom.

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Al Viro]

On Mon, Dec 11, 2006 at 02:27:46AM -0800, Andrew Morton wrote:
> @@ -115,6 +115,11 @@ extern void setup_arch(char **);
>  #define device_initcall_sync(fn)	__define_initcall("6s",fn,6s)
>  #define late_initcall(fn)		__define_initcall("7",fn,7)
>  #define late_initcall_sync(fn)		__define_initcall("7s",fn,7s)
> +#define populate_rootfs_initcall(fn)	__define_initcall("8",fn,8)
> +#define populate_rootfs_initcall_sync(fn) __define_initcall("8s",fn,8s)
> +#define rootfs_neeeded_initcall(fn)	__define_initcall("9",fn,9)
> +#define rootfs_neeeded_initcall_sync(fn) __define_initcall("9s",fn,9s)

Ewww....  After module_init()?  Please, don't.  Come on, if it can
be a module, it _must_ be ready to run late in the game.

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Al Viro]

On Mon, Dec 11, 2006 at 02:34:36AM -0800, Andrew Morton wrote:

> There are plenty of drivers in there using subsys_initcall, arch_initcall,
> postcore_initcall, core_initcall and even one pure_initcall.
> 
> Heaven knows why.  They're drivers :(
 
> A heck of a lot of things can trigger an /sbin/hotplug run.  It could well
> be that Andrew's driver didn't want to run hotplug at all, but the kernel
> did it anwyay.  But as soon as the script appeared at /sbin/hotplug, and it
> happened to use foo|bar: boom.

Again, I agree with moving pipes up, but keep in mind that any caller
of /sbin/hotplug either
	* doesn't need it or
	* really handles failure or
	* really should _not_ be run before populate_rootfs()

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Olaf Hering]

On Mon, Dec 11, Chuck Ebbert wrote:

> Why not create a new initcall category for things that must run before
> early userspace?

Why do you want to continue with papering over the root cause?
Pick some janitor, let him write something that implements something
like make style dependencies for initcalls.
Then you can get rid of all that foo_initcall stuff.
It surely needs work to get all that done.

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Al Viro]

On Mon, Dec 11, 2006 at 11:48:57AM +0100, Olaf Hering wrote:
> On Mon, Dec 11, Chuck Ebbert wrote:
> 
> > Why not create a new initcall category for things that must run before
> > early userspace?
> 
> Why do you want to continue with papering over the root cause?
> Pick some janitor, let him write something that implements something
> like make style dependencies for initcalls.
> Then you can get rid of all that foo_initcall stuff.
> It surely needs work to get all that done.

I would argue that _this_ is papering over the root cause.  Namely,
too complex ordering requirements.  Growing a technics for allowing
them to fester is an odd solution...

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Jeff Garzik]

Andrew Morton wrote:
> A heck of a lot of things can trigger an /sbin/hotplug run.  It could well
> be that Andrew's driver didn't want to run hotplug at all, but the kernel
> did it anwyay.  But as soon as the script appeared at /sbin/hotplug, and it
> happened to use foo|bar: boom.

In fact, <many> things run /sbin/hotplug but don't necessarily need to :/

I think bcrl used a counter one time, and saw over 1000 /sbin/hotplug 
executions during a single boot.

	Jeff

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Linus Torvalds]

On Mon, 11 Dec 2006, Chuck Ebbert wrote:
>
> Prevent oops when an app tries to create a pipe while pipefs
> is not mounted.

Have you actually seen this, or is this just from looking at code?

Quite frankly, if "pipe_mnt" is ever NULL, we're dead for lots of other 
reasons. 

In fact, pipe_mnt can't be NULL. The way it is initialized is:

	pipe_mnt = kern_mount(&pipe_fs_type);

and pipe_mnt doesn't even return NULL - it returns an error pointer, so if 
"kern_mount()" were to have failed, pipe_mnt will be some random invalid 
pointer that could only be tested with IS_ERR(), not by comparing against 
NULL.

But more fundamentally - we might as well oops. We need to panic or oops 
or do _something_ bad at some point anyway, because it's MUCH better to 
fail spectacularly than it would be to just silently fail without a pipe.

Hmm?

		Linus

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Linus Torvalds]

On Mon, 11 Dec 2006, Al Viro wrote:

> On Mon, Dec 11, 2006 at 02:27:46AM -0800, Andrew Morton wrote:
> > @@ -115,6 +115,11 @@ extern void setup_arch(char **);
> >  #define device_initcall_sync(fn)	__define_initcall("6s",fn,6s)
> >  #define late_initcall(fn)		__define_initcall("7",fn,7)
> >  #define late_initcall_sync(fn)		__define_initcall("7s",fn,7s)
> > +#define populate_rootfs_initcall(fn)	__define_initcall("8",fn,8)
> > +#define populate_rootfs_initcall_sync(fn) __define_initcall("8s",fn,8s)
> > +#define rootfs_neeeded_initcall(fn)	__define_initcall("9",fn,9)
> > +#define rootfs_neeeded_initcall_sync(fn) __define_initcall("9s",fn,9s)
> 
> Ewww....  After module_init()?  Please, don't.  Come on, if it can
> be a module, it _must_ be ready to run late in the game.

Yeah, I think you should just run "populate_rootfs()" just before 
"module_init" (which is the same as "device_initcall()").

So perhaps somethign like this? (totally untested)

Btw, if the linker sorts sections some way (does it?) we could probably 
just make the vmlinux.lds.S file do

	*(.initcall*.init)

or something, and then just let special cases like this use

	__initcall(myfn, 5.1);

to show that it's between levels 5 and 6. But that would depend on the 
linker section beign sorted alphabetically. Does anybody know if the 
linker sorts these things somehow?

This patch is totally untested, but it looks obvious. It just says that 
we'll populate rootfs _after_ we've done the fs-level initcalls, but 
before we do any actual "device" initcalls.

If any really core stuff needs user-land - tough titties, as they say.

		Linus
----
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 6e9fceb..7437cca 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -242,6 +242,7 @@
   	*(.initcall4s.init)						\
   	*(.initcall5.init)						\
   	*(.initcall5s.init)						\
+	*(.initcallrootfs.init)						\
   	*(.initcall6.init)						\
   	*(.initcall6s.init)						\
   	*(.initcall7.init)						\
diff --git a/include/linux/init.h b/include/linux/init.h
index 5eb5d24..5a593a1 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -111,6 +111,7 @@ extern void setup_arch(char **);
 #define subsys_initcall_sync(fn)	__define_initcall("4s",fn,4s)
 #define fs_initcall(fn)			__define_initcall("5",fn,5)
 #define fs_initcall_sync(fn)		__define_initcall("5s",fn,5s)
+#define rootfs_initcall(fn)		__define_initcall("rootfs",fn,rootfs)
 #define device_initcall(fn)		__define_initcall("6",fn,6)
 #define device_initcall_sync(fn)	__define_initcall("6s",fn,6s)
 #define late_initcall(fn)		__define_initcall("7",fn,7)
diff --git a/init/initramfs.c b/init/initramfs.c
index 85f0403..4fa0f79 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -526,7 +526,7 @@ static void __init free_initrd(void)
 
 #endif
 
-void __init populate_rootfs(void)
+static int __init populate_rootfs(void)
 {
 	char *err = unpack_to_rootfs(__initramfs_start,
 			 __initramfs_end - __initramfs_start, 0);
@@ -544,7 +544,7 @@ void __init populate_rootfs(void)
 			unpack_to_rootfs((char *)initrd_start,
 				initrd_end - initrd_start, 0);
 			free_initrd();
-			return;
+			return 0;
 		}
 		printk("it isn't (%s); looks like an initrd\n", err);
 		fd = sys_open("/initrd.image", O_WRONLY|O_CREAT, 0700);
@@ -565,4 +565,6 @@ void __init populate_rootfs(void)
 #endif
 	}
 #endif
+	return 0;
 }
+rootfs_initcall(populate_rootfs);
diff --git a/init/main.c b/init/main.c
index 036f97c..ae12fa3 100644
--- a/init/main.c
+++ b/init/main.c
@@ -94,7 +94,6 @@ extern void pidmap_init(void);
 extern void prio_tree_init(void);
 extern void radix_tree_init(void);
 extern void free_initmem(void);
-extern void populate_rootfs(void);
 extern void driver_init(void);
 extern void prepare_namespace(void);
 #ifdef	CONFIG_ACPI
@@ -739,12 +738,6 @@ static int init(void * unused)
 
 	cpuset_init_smp();
 
-	/*
-	 * Do this before initcalls, because some drivers want to access
-	 * firmware files.
-	 */
-	populate_rootfs();
-
 	do_basic_setup();
 
 	/*

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Al Viro]

On Mon, Dec 11, 2006 at 08:01:40AM -0800, Linus Torvalds wrote:
> 
> 
> On Mon, 11 Dec 2006, Al Viro wrote:
> 
> > On Mon, Dec 11, 2006 at 02:27:46AM -0800, Andrew Morton wrote:
> > > @@ -115,6 +115,11 @@ extern void setup_arch(char **);
> > >  #define device_initcall_sync(fn)	__define_initcall("6s",fn,6s)
> > >  #define late_initcall(fn)		__define_initcall("7",fn,7)
> > >  #define late_initcall_sync(fn)		__define_initcall("7s",fn,7s)
> > > +#define populate_rootfs_initcall(fn)	__define_initcall("8",fn,8)
> > > +#define populate_rootfs_initcall_sync(fn) __define_initcall("8s",fn,8s)
> > > +#define rootfs_neeeded_initcall(fn)	__define_initcall("9",fn,9)
> > > +#define rootfs_neeeded_initcall_sync(fn) __define_initcall("9s",fn,9s)
> > 
> > Ewww....  After module_init()?  Please, don't.  Come on, if it can
> > be a module, it _must_ be ready to run late in the game.
> 
> Yeah, I think you should just run "populate_rootfs()" just before 
> "module_init" (which is the same as "device_initcall()").
 
> If any really core stuff needs user-land - tough titties, as they say.

FWIW, I really think that this sort of bugs ("oh, I call hotplug,
rootfs is there but kernel is not ready, woe is me") clearly show
that many, _many_ users of hotplug are BS.  The reason is simple -
if we have a call of hotplug that early, we have a driver that lives
with hotplug failures _and_ with different behaviour in built-in
and modular cases.  I would argue that any such driver is broken.

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Linus Torvalds]

On Mon, 11 Dec 2006, Al Viro wrote:
> 
> FWIW, I really think that this sort of bugs ("oh, I call hotplug,
> rootfs is there but kernel is not ready, woe is me") clearly show
> that many, _many_ users of hotplug are BS.  The reason is simple -
> if we have a call of hotplug that early, we have a driver that lives
> with hotplug failures _and_ with different behaviour in built-in
> and modular cases.  I would argue that any such driver is broken.

Now, you are probably right that many of them are unnecessary, but I don't 
agree in _general_. 

It's perfectly normal behaviour to say "I discovered this device".

The fact that device discovery happens during early bootup and nobody even 
_cares_ (because early bootup ends up enumerating all discovered devices 
on its own) is a separate issue. The fact that many distros don't care 
during early bootup doesn't mean that they don't care later on.

Another reason for "unnecessary" hotplug events is that generic functions 
like "I added a bus" happen both for static buses _and_ for "hey, somebody 
loaded a module that found this bus". Again, the static case may be 
something that people don't _care_ about, but we should (a) use common 
code and (b) be consistent, so we do a hotplug event regardless.

That said, I definitely agree that people shouldn't expect hotplug events 
to be delievered too early. If it's something that runs in a 
core_initcall, and is compiled in, no way in *hell* should we actually 
expose that to anything - it's just too early.

So it makes perfect sense to say

   "you won't be getting any notification by anything built-in, until 
    'device_initcall' (which is the default module_init, of course)".

which in the case of certain drivers obviously _does_ mean that they had 
better not try to use any early initcalls to load firmware.

		Linus

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Benjamin Herrenschmidt]

> So it makes perfect sense to say
> 
>    "you won't be getting any notification by anything built-in, until 
>     'device_initcall' (which is the default module_init, of course)".
> 
> which in the case of certain drivers obviously _does_ mean that they had 
> better not try to use any early initcalls to load firmware.

And that will fix some other issues I think I've seen (a while ago, I
might have a memory mixup here) related to /sbin/hotplug being called
before /dev/null & /dev/zero are initialized (they are fs_initcall). At
least with that patch, it won't happen.

Ben.

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Andrew Morton]

On Mon, 11 Dec 2006 08:01:40 -0800 (PST)
Linus Torvalds <torvalds@osdl.org> wrote:

> 
> 
> On Mon, 11 Dec 2006, Al Viro wrote:
> 
> > On Mon, Dec 11, 2006 at 02:27:46AM -0800, Andrew Morton wrote:
> > > @@ -115,6 +115,11 @@ extern void setup_arch(char **);
> > >  #define device_initcall_sync(fn)	__define_initcall("6s",fn,6s)
> > >  #define late_initcall(fn)		__define_initcall("7",fn,7)
> > >  #define late_initcall_sync(fn)		__define_initcall("7s",fn,7s)
> > > +#define populate_rootfs_initcall(fn)	__define_initcall("8",fn,8)
> > > +#define populate_rootfs_initcall_sync(fn) __define_initcall("8s",fn,8s)
> > > +#define rootfs_neeeded_initcall(fn)	__define_initcall("9",fn,9)
> > > +#define rootfs_neeeded_initcall_sync(fn) __define_initcall("9s",fn,9s)
> > 
> > Ewww....  After module_init()?  Please, don't.  Come on, if it can
> > be a module, it _must_ be ready to run late in the game.
> 
> Yeah, I think you should just run "populate_rootfs()" just before 
> "module_init" (which is the same as "device_initcall()").
> 
> So perhaps somethign like this? (totally untested)
> 
> Btw, if the linker sorts sections some way (does it?) we could probably 
> just make the vmlinux.lds.S file do
> 
> 	*(.initcall*.init)
> 
> or something, and then just let special cases like this use
> 
> 	__initcall(myfn, 5.1);
> 
> to show that it's between levels 5 and 6. But that would depend on the 
> linker section beign sorted alphabetically. Does anybody know if the 
> linker sorts these things somehow?
> 
> This patch is totally untested, but it looks obvious. It just says that 
> we'll populate rootfs _after_ we've done the fs-level initcalls, but 
> before we do any actual "device" initcalls.
> 
> If any really core stuff needs user-land - tough titties, as they say.
> 
> 		Linus
> ----
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 6e9fceb..7437cca 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -242,6 +242,7 @@
>    	*(.initcall4s.init)						\
>    	*(.initcall5.init)						\
>    	*(.initcall5s.init)						\
> +	*(.initcallrootfs.init)						\
>    	*(.initcall6.init)						\
>    	*(.initcall6s.init)						\
>    	*(.initcall7.init)						\
> diff --git a/include/linux/init.h b/include/linux/init.h
> index 5eb5d24..5a593a1 100644
> --- a/include/linux/init.h
> +++ b/include/linux/init.h
> @@ -111,6 +111,7 @@ extern void setup_arch(char **);
>  #define subsys_initcall_sync(fn)	__define_initcall("4s",fn,4s)
>  #define fs_initcall(fn)			__define_initcall("5",fn,5)
>  #define fs_initcall_sync(fn)		__define_initcall("5s",fn,5s)
> +#define rootfs_initcall(fn)		__define_initcall("rootfs",fn,rootfs)
>  #define device_initcall(fn)		__define_initcall("6",fn,6)
>  #define device_initcall_sync(fn)	__define_initcall("6s",fn,6s)
>  #define late_initcall(fn)		__define_initcall("7",fn,7)

Looks like this might break pcmcia which for some reason does firmware
requesting at fs_initcall level (drivers/pcmcia/ds.c).

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Dominik Brodowski]

On Mon, Dec 11, 2006 at 06:08:22PM -0800, Andrew Morton wrote:
> > diff --git a/include/linux/init.h b/include/linux/init.h
> > index 5eb5d24..5a593a1 100644
> > --- a/include/linux/init.h
> > +++ b/include/linux/init.h
> > @@ -111,6 +111,7 @@ extern void setup_arch(char **);
> >  #define subsys_initcall_sync(fn)	__define_initcall("4s",fn,4s)
> >  #define fs_initcall(fn)			__define_initcall("5",fn,5)
> >  #define fs_initcall_sync(fn)		__define_initcall("5s",fn,5s)
> > +#define rootfs_initcall(fn)		__define_initcall("rootfs",fn,rootfs)
> >  #define device_initcall(fn)		__define_initcall("6",fn,6)
> >  #define device_initcall_sync(fn)	__define_initcall("6s",fn,6s)
> >  #define late_initcall(fn)		__define_initcall("7",fn,7)
> 
> Looks like this might break pcmcia which for some reason does firmware
> requesting at fs_initcall level (drivers/pcmcia/ds.c).

That codepath is not triggered before device_initcall()s occur. So it's a
non-issue for PCMCIA.

Thanks,
	Dominik

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Linus Torvalds]

On Mon, 11 Dec 2006, Andrew Morton wrote:
> 
> Looks like this might break pcmcia which for some reason does firmware
> requesting at fs_initcall level (drivers/pcmcia/ds.c).

Ok, that's just strange. 

I think it's fine to do init_pcmcia_bus early to make sure that the PCMCIA 
bus interface is there by the time the driver init stuff happens, but I 
really don't see the point of that firmware load to be there. And all that 
MATCH_FAKE_CIS stuff is about the _devices_, not the PCMCIA bus, so that 
whole thing looks pretty silly. It should be done by the device 
registration (which is obviously device_initcall), not by some bus layer.

Hopefully Dominik can fix whatever up (if it even needs it)

		Linus

Re: [patch] pipe: Don't oops when pipe filesystem isn't mounted

[Michael Tokarev]

Linus Torvalds wrote:
> 
> On Mon, 11 Dec 2006, Chuck Ebbert wrote:
>> Prevent oops when an app tries to create a pipe while pipefs
>> is not mounted.
> 
> Have you actually seen this, or is this just from looking at code?
> 
> Quite frankly, if "pipe_mnt" is ever NULL, we're dead for lots of other 
> reasons. 
> 
> In fact, pipe_mnt can't be NULL. The way it is initialized is:
> 
> 	pipe_mnt = kern_mount(&pipe_fs_type);
> 
> and pipe_mnt doesn't even return NULL - it returns an error pointer, so if 
> "kern_mount()" were to have failed, pipe_mnt will be some random invalid 
> pointer that could only be tested with IS_ERR(), not by comparing against 
> NULL.

http://marc.theaimsgroup.com/?t=116510390600001&r=1&w=2

/mjt