Prevent oops when an app tries to create a pipe while pipefs
is not mounted.
Signed-off-by: Chuck Ebbert <[email protected]>
--- 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
On Mon, 11 Dec 2006 03:27:37 -0500
Chuck Ebbert <[email protected]> wrote:
> Prevent oops when an app tries to create a pipe while pipefs
> is not mounted.
>
> Signed-off-by: Chuck Ebbert <[email protected]>
>
> --- 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.
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 <[email protected]>
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.
On Mon, 11 Dec 2006 00:55:57 -0800
Andrew Morton <[email protected]> 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 ...
?
On Mon, Dec 11, 2006 at 01:13:27AM -0800, Andrew Morton wrote:
> On Mon, 11 Dec 2006 00:55:57 -0800
> Andrew Morton <[email protected]> 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.
On Mon, 11 Dec 2006 09:21:30 +0000
Al Viro <[email protected]> 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 <[email protected]> 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.
On Mon, Dec 11, 2006 at 01:25:45AM -0800, Andrew Morton wrote:
> On Mon, 11 Dec 2006 09:21:30 +0000
> Al Viro <[email protected]> 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 <[email protected]> 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...
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
In-Reply-To: <[email protected]>
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
On Mon, 11 Dec 2006 09:33:15 +0000
Al Viro <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
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.
On Mon, 11 Dec 2006 10:03:01 +0000
Al Viro <[email protected]> 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?
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.
On Mon, 11 Dec 2006 02:17:18 -0800
Andrew Morton <[email protected]> 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);
_
On Mon, 11 Dec 2006 10:22:07 +0000
Al Viro <[email protected]> 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.
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.
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()
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.
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...
--- linux-2.6.19/kernel/timer.c 2006-12-11 11:25:50.000000000 +0100
+++ linux-2.6.19-ed/kernel/timer.c 2006-12-11 11:31:30.000000000 +0100
@@ -1020,11 +1020,9 @@ static inline void calc_load(unsigned lo
* This read-write spinlock protects us from races in SMP while
* playing with xtime and avenrun.
*/
-#ifndef ARCH_HAVE_XTIME_LOCK
-__cacheline_aligned_in_smp DEFINE_SEQLOCK(xtime_lock);
+__attribute__((weak)) __cacheline_aligned_in_smp DEFINE_SEQLOCK(xtime_lock);
EXPORT_SYMBOL(xtime_lock);
-#endif
/*
* This function runs timers and the timer-tq in bottom half context.
--- linux-2.6.19/include/linux/time.h 2006-12-11 11:34:54.000000000 +0100
+++ linux-2.6.19-ed/include/linux/time.h 2006-12-11 11:34:54.000000000 +0100
@@ -90,7 +90,7 @@ static inline struct timespec timespec_s
extern struct timespec xtime;
extern struct timespec wall_to_monotonic;
-extern seqlock_t xtime_lock;
+extern seqlock_t xtime_lock __attribute__((weak));
void timekeeping_init(void);
--- linux-2.6.19/include/asm-x86_64/vsyscall.h 2006-12-11 11:34:54.000000000 +0100
+++ linux-2.6.19-ed/include/asm-x86_64/vsyscall.h 2006-12-11 11:34:54.000000000 +0100
@@ -55,11 +55,6 @@ extern struct vxtime_data vxtime;
extern int vgetcpu_mode;
extern struct timezone sys_tz;
extern int sysctl_vsyscall;
-extern seqlock_t xtime_lock;
-
-extern int sysctl_vsyscall;
-
-#define ARCH_HAVE_XTIME_LOCK 1
#endif /* __KERNEL__ */
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
--- linux-2.6.19/kernel/timer.c 2006-12-11 12:27:28.000000000 +0100
+++ linux-2.6.19-ed/kernel/timer.c 2006-12-11 12:29:11.000000000 +0100
@@ -1008,11 +1008,15 @@ static inline void calc_load(unsigned lo
unsigned long active_tasks; /* fixed-point */
static int count = LOAD_FREQ;
- active_tasks = count_active_tasks();
- for (count -= ticks; count < 0; count += LOAD_FREQ) {
- CALC_LOAD(avenrun[0], EXP_1, active_tasks);
- CALC_LOAD(avenrun[1], EXP_5, active_tasks);
- CALC_LOAD(avenrun[2], EXP_15, active_tasks);
+ count -= ticks;
+ if (unlikely(count < 0)) {
+ active_tasks = count_active_tasks();
+ do {
+ CALC_LOAD(avenrun[0], EXP_1, active_tasks);
+ CALC_LOAD(avenrun[1], EXP_5, active_tasks);
+ CALC_LOAD(avenrun[2], EXP_15, active_tasks);
+ count += LOAD_FREQ;
+ } while (count < 0);
}
}
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
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();
/*
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.
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
--- linux-2.6.19/include/linux/pipe_fs_i.h 2006-12-11 17:00:21.000000000 +0100
+++ linux-2.6.19-ed/include/linux/pipe_fs_i.h 2006-12-11 17:09:24.000000000 +0100
@@ -12,7 +12,7 @@
struct pipe_buffer {
struct page *page;
unsigned int offset, len;
- struct pipe_buf_operations *ops;
+ const struct pipe_buf_operations *ops;
unsigned int flags;
};
@@ -43,7 +43,6 @@ struct pipe_inode_info {
unsigned int nrbufs, curbuf;
struct pipe_buffer bufs[PIPE_BUFFERS];
struct page *tmp_page;
- unsigned int start;
unsigned int readers;
unsigned int writers;
unsigned int waiting_writers;
--- linux-2.6.19/fs/pipe.c 2006-12-11 17:04:00.000000000 +0100
+++ linux-2.6.19-ed/fs/pipe.c 2006-12-11 17:14:04.000000000 +0100
@@ -207,7 +207,7 @@ int generic_pipe_buf_pin(struct pipe_ino
return 0;
}
-static struct pipe_buf_operations anon_pipe_buf_ops = {
+static const struct pipe_buf_operations anon_pipe_buf_ops = {
.can_merge = 1,
.map = generic_pipe_buf_map,
.unmap = generic_pipe_buf_unmap,
@@ -243,7 +243,7 @@ pipe_read(struct kiocb *iocb, const stru
if (bufs) {
int curbuf = pipe->curbuf;
struct pipe_buffer *buf = pipe->bufs + curbuf;
- struct pipe_buf_operations *ops = buf->ops;
+ const struct pipe_buf_operations *ops = buf->ops;
void *addr;
size_t chars = buf->len;
int error, atomic;
@@ -365,7 +365,7 @@ pipe_write(struct kiocb *iocb, const str
int lastbuf = (pipe->curbuf + pipe->nrbufs - 1) &
(PIPE_BUFFERS-1);
struct pipe_buffer *buf = pipe->bufs + lastbuf;
- struct pipe_buf_operations *ops = buf->ops;
+ const struct pipe_buf_operations *ops = buf->ops;
int offset = buf->offset + buf->len;
if (ops->can_merge && offset + chars <= PAGE_SIZE) {
@@ -756,7 +756,7 @@ const struct file_operations rdwr_fifo_f
.fasync = pipe_rdwr_fasync,
};
-static struct file_operations read_pipe_fops = {
+static const struct file_operations read_pipe_fops = {
.llseek = no_llseek,
.read = do_sync_read,
.aio_read = pipe_read,
@@ -768,7 +768,7 @@ static struct file_operations read_pipe_
.fasync = pipe_read_fasync,
};
-static struct file_operations write_pipe_fops = {
+static const struct file_operations write_pipe_fops = {
.llseek = no_llseek,
.read = bad_pipe_r,
.write = do_sync_write,
@@ -780,7 +780,7 @@ static struct file_operations write_pipe
.fasync = pipe_write_fasync,
};
-static struct file_operations rdwr_pipe_fops = {
+static const struct file_operations rdwr_pipe_fops = {
.llseek = no_llseek,
.read = do_sync_read,
.aio_read = pipe_read,
--- linux-2.6.19/fs/splice.c 2006-12-11 17:04:00.000000000 +0100
+++ linux-2.6.19-ed/fs/splice.c 2006-12-11 17:09:24.000000000 +0100
@@ -42,7 +42,7 @@ struct splice_pipe_desc {
struct partial_page *partial; /* pages[] may not be contig */
int nr_pages; /* number of pages in map */
unsigned int flags; /* splice flags */
- struct pipe_buf_operations *ops;/* ops associated with output pipe */
+ const struct pipe_buf_operations *ops;/* ops associated with output pipe */
};
/*
@@ -139,7 +139,7 @@ error:
return err;
}
-static struct pipe_buf_operations page_cache_pipe_buf_ops = {
+static const struct pipe_buf_operations page_cache_pipe_buf_ops = {
.can_merge = 0,
.map = generic_pipe_buf_map,
.unmap = generic_pipe_buf_unmap,
@@ -159,7 +159,7 @@ static int user_page_pipe_buf_steal(stru
return generic_pipe_buf_steal(pipe, buf);
}
-static struct pipe_buf_operations user_page_pipe_buf_ops = {
+static const struct pipe_buf_operations user_page_pipe_buf_ops = {
.can_merge = 0,
.map = generic_pipe_buf_map,
.unmap = generic_pipe_buf_unmap,
@@ -724,7 +724,7 @@ static ssize_t __splice_from_pipe(struct
for (;;) {
if (pipe->nrbufs) {
struct pipe_buffer *buf = pipe->bufs + pipe->curbuf;
- struct pipe_buf_operations *ops = buf->ops;
+ const struct pipe_buf_operations *ops = buf->ops;
sd.len = buf->len;
if (sd.len > sd.total_len)
> 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.
On Mon, 11 Dec 2006 08:01:40 -0800 (PST)
Linus Torvalds <[email protected]> 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).
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
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
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