lkml.org 
[lkml]   [2006]   [Dec]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
Date
From
SubjectRe: kernel NULL pointer deref in pipe() - hotplug in initramfs
Chuck Ebbert wrote:
> In-Reply-To: <45721258.1070802@tls.msk.ru>
>
> On Sun, 03 Dec 2006 02:55:04 +0300, Michael Tokarev wrote:
>
>> When /sbin/hotplug is present in initramfs, and it's a shell
>> script, kernel OOPSes on every hotplug invocation.
>
> Does this mean that if it's _not_ a shell script everything
> is fine?

It means that if there's no pipe() call, everything is fine.
That is, for a shell script it's just more likely to contain
some pipelines.

[]
>> <0>EFLAGS: 00010282 (2.6.19-c3 #2.6.19.0)
[]
>> <4> <1>BUG: unable to handle kernel NULL pointer dereference at virtual address 00000014
>> (note also the formatting is a bit wrong here -- that <4> prefix in front of <1>BUG..)
>
> Yeah, that's from the previous oops. I don't know why bust_spinlocks() leaves
> that hanging space, but it's been that way for a long time.

So can this be fixed, too? ;)

> BTW did you hand-edit the kernel .version file? "(2.6.19-c3 #2.6.19.0)"
> should not print unless you put that 2.6.19.0 there yourself.

Yes I did. For an rpm- or deb-based kernel, the "build number" is irrelevant
(it's always 1, since both rpm and deb rebuilds everything from scratch).
This #2.6.19.0 comes from the package revision number - I just do
echo $revision > .version
and remove the part from Makefile which updates the file on every (re)build.
But this isn't relevant for the discussion.

>> Are pipes disallowed in hotplug fired off initramfs?
>
> AFAICT the pipe filesystem isn't registered yet, so probably not.
>
>> (Even if they are, kernel probably still should not OOPS like that ;)
>
> Either (a) pipefs should be registered early, or (b) the call should just
> fail instead of oopsing. Untested patch for (b) below, can you test it?
> (I'm not messing with the init sequence.)
>
>> The error seems to be harmful however. That is, hotplug script does not
>> run, but the kernel continues running.
>
> You mean harmless, right?

Yup. Oh those foreign languages... :)

> Signed-off-by: Chuck Ebbert <76306.1226@compuserve.com>
>
> --- 2.6.19.0.2-32.orig/fs/pipe.c
> +++ 2.6.19.0.2-32/fs/pipe.c

Another funny revision number? ;)

> @@ -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;

Well, that fixes the OOPs, but I'd do it a in bit more readable way,
and eliminating the gotos too (this is a stylistic change - I see alot
of gotos in kernel for error returns, but here, in such a small function,
gotos looks somewhat funny, esp. when the only statement after the label
is 'return NULL'):

--- linux-2.6.19/fs/pipe.c.orig 2006-11-30 00:57:37.000000000 +0300
+++ linux-2.6.19/fs/pipe.c 2006-12-03 14:27:06.000000000 +0300
@@ -839,15 +839,20 @@ 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;
struct pipe_inode_info *pipe;

+ if (!pipe_mnt) /* if pipefs isn't mounted (yet) */
+ return NULL;
+ inode = new_inode(pipe_mnt->mnt_sb);
if (!inode)
- goto fail_inode;
+ return NULL;

pipe = alloc_pipe_info(inode);
- if (!pipe)
- goto fail_iput;
+ if (!pipe) {
+ iput(inode);
+ return NULL;
+ }
inode->i_pipe = pipe;

pipe->readers = pipe->writers = 1;
@@ -866,12 +871,6 @@ static struct inode * get_pipe_inode(voi
inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;

return inode;
-
-fail_iput:
- iput(inode);
-
-fail_inode:
- return NULL;
}

struct file *create_write_pipe(void)

Or like this (without messing up with goto):

--- linux-2.6.19/fs/pipe.c.orig 2006-11-30 00:57:37.000000000 +0300
+++ linux-2.6.19/fs/pipe.c 2006-12-03 14:31:19.000000000 +0300
@@ -839,9 +839,12 @@ 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;
struct pipe_inode_info *pipe;

+ if (!pipe_mnt) /* in case pipefs isn't mounted (yet) */
+ goto fail_inode;
+ inode = new_inode(pipe_mnt->mnt_sb);
if (!inode)
goto fail_inode;

In either case, this change not fixes the problem. As I said, the OOPs is
harmless (yes, not harmful :), so the end effect is the same, just instead
of the OOPs, now the bad stuff is coming from shell complaining about failing
to create pipe.

/mjt
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-12-03 12:41    [W:1.143 / U:0.512 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site