Messages in this thread | | | From | Linus Torvalds <> | Date | Mon, 30 Oct 2017 20:06:23 -0700 | Subject | Re: Kernel crash in free_pipe_info() |
| |
On Mon, Oct 30, 2017 at 7:08 PM, Linus Torvalds <torvalds@linux-foundation.org> 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
| |