| Message ID | 779f767d-c08b-0c03-198e-06270100d529@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pipe: fix potential inode leak in create_pipe_files() | expand |
On Wed, Oct 28, 2020 at 11:03:52AM +0800, Zhiqiang Liu wrote: > > In create_pipe_files(), if alloc_file_clone() fails, we will call > put_pipe_info to release pipe, and call fput() to release f. > However, we donot call iput() to free inode. Huh? Have you actually tried to trigger that failure exit? > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> > Signed-off-by: Feilong Lin <linfeilong@huawei.com> > --- > fs/pipe.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/pipe.c b/fs/pipe.c > index 0ac197658a2d..8856607fde65 100644 > --- a/fs/pipe.c > +++ b/fs/pipe.c > @@ -924,6 +924,7 @@ int create_pipe_files(struct file **res, int flags) > if (IS_ERR(res[0])) { > put_pipe_info(inode, inode->i_pipe); > fput(f); > + iput(inode); > return PTR_ERR(res[0]); No. That inode is created with refcount 1. If alloc_file_pseudo() succeeds, the reference we'd been holding has been transferred into dentry allocated by alloc_file_pseudo() (and attached to f). From that point on we do *NOT* own a reference to inode and no subsequent failure exits have any business releasing it. In particular, alloc_file_clone() DOES NOT create extra references to inode, whether it succeeds or fails. Dropping the reference to f will take care of everything. If you tried to trigger that failure exit with your patch applied, you would've seen double iput(), as soon as you return from sys_pipe() to userland and task_work is processed (which is where the real destructor of struct file will happen). NAK.
On 2020/10/28 11:54, Al Viro wrote: > On Wed, Oct 28, 2020 at 11:03:52AM +0800, Zhiqiang Liu wrote: >> >> In create_pipe_files(), if alloc_file_clone() fails, we will call >> put_pipe_info to release pipe, and call fput() to release f. >> However, we donot call iput() to free inode. > > Huh? Have you actually tried to trigger that failure exit? > >> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> >> Signed-off-by: Feilong Lin <linfeilong@huawei.com> >> --- >> fs/pipe.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/fs/pipe.c b/fs/pipe.c >> index 0ac197658a2d..8856607fde65 100644 >> --- a/fs/pipe.c >> +++ b/fs/pipe.c >> @@ -924,6 +924,7 @@ int create_pipe_files(struct file **res, int flags) >> if (IS_ERR(res[0])) { >> put_pipe_info(inode, inode->i_pipe); >> fput(f); >> + iput(inode); >> return PTR_ERR(res[0]); > > No. That inode is created with refcount 1. If alloc_file_pseudo() > succeeds, the reference we'd been holding has been transferred into > dentry allocated by alloc_file_pseudo() (and attached to f). >>From that point on we do *NOT* own a reference to inode and no > subsequent failure exits have any business releasing it. > > In particular, alloc_file_clone() DOES NOT create extra references > to inode, whether it succeeds or fails. Dropping the reference > to f will take care of everything. > > If you tried to trigger that failure exit with your patch applied, > you would've seen double iput(), as soon as you return from sys_pipe() > to userland and task_work is processed (which is where the real > destructor of struct file will happen). > > NAK. > Thanks for your patient response. Learned a lot from your reply. Please ignore the patch. > . >
diff --git a/fs/pipe.c b/fs/pipe.c index 0ac197658a2d..8856607fde65 100644 --- a/fs/pipe.c +++ b/fs/pipe.c @@ -924,6 +924,7 @@ int create_pipe_files(struct file **res, int flags) if (IS_ERR(res[0])) { put_pipe_info(inode, inode->i_pipe); fput(f); + iput(inode); return PTR_ERR(res[0]); } res[0]->private_data = inode->i_pipe;