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
