Re: Kernel crash in free_pipe_info()

From: Linus Torvalds
Date: Mon Oct 30 2017 - 23:06:30 EST


On Mon, Oct 30, 2017 at 7:08 PM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> I'm not seeing anything that makes sense. I'll have to think about this.

Al, would you mind taking a look at the error handling in create_pipe_files().

In particular, look here:

- we start out allocating the inode with "get_pipe_inode().

That sets up a inode->i_pipe, with pipe->files initialized to 2.
Fine. We're going to have two file descriptors.

- we then create the dummy path:

path.dentry = d_alloc_pseudo(pipe_mnt->mnt_sb, &empty_name);

fine fine. Again, this looks all good for the success cases.

But the *error* cases are a bit dodgy, aren't they?

We have three different error cases:

- we couldn't even allocate a dentry. We do

free_pipe_info(inode->i_pipe);
iput(inode);

- we couldn't allocate any file at all:

free_pipe_info(inode->i_pipe);
path_put(&path);

- we allocated the first file, but not the second:

put_filp(f);
free_pipe_info(inode->i_pipe);
path_put(&path);

and it worries me a bit that in all those error cases, we end up doing
that "free_pipe_info()", but we basically do this half-arsed job of
freeing things.

For example, we use "put_filp()" to free the file pointer, not "fput()".

We do that "free_pipe_info(inode->i_pipe);", but we never actually
clear inode->i_pipe, so now we have an inode that looks like a pipe
inode, and has a stale pointer to a pipe_inode_info.

It all looks technically correct. It's fine to use put_filp(), because
the file pointer has never really been used. And the inode should
never get re-used anyway without going through the whole reinit in
inode_init_always().

So I don't see anything *wrong*, but I see a lot that is just unusual,
and seems to depend on half-initialized state being fine. Can you look
at this?

Linus