| Message ID | 154149557692.17196.12607896696117775780.stgit@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: Put leaked request on error path of fuse_retrieve() | expand |
On Tue, Nov 6, 2018 at 10:13 AM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote: > fuse_request_send_notify_reply() may fail, and this case > it remains leaked (fuse_retrieve_end(), which is called > on error path, does not do that). Also, fc->num_waiting, > will never be decremented, and fuse_wait_aborted() will > never finish. So, put the request patently. > > Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> Posted same patch yesterday for a syzbot report. How did you notice this? Thanks, Miklos > --- > fs/fuse/dev.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c > index ae813e609932..6fe330cc9709 100644 > --- a/fs/fuse/dev.c > +++ b/fs/fuse/dev.c > @@ -1768,8 +1768,10 @@ static int fuse_retrieve(struct fuse_conn *fc, struct inode *inode, > req->in.args[1].size = total_len; > > err = fuse_request_send_notify_reply(fc, req, outarg->notify_unique); > - if (err) > + if (err) { > fuse_retrieve_end(fc, req); > + fuse_put_request(fc, req); > + } > > return err; > } >
On 06.11.2018 12:23, Miklos Szeredi wrote: > On Tue, Nov 6, 2018 at 10:13 AM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote: >> fuse_request_send_notify_reply() may fail, and this case >> it remains leaked (fuse_retrieve_end(), which is called >> on error path, does not do that). Also, fc->num_waiting, >> will never be decremented, and fuse_wait_aborted() will >> never finish. So, put the request patently. >> >> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> > > Posted same patch yesterday for a syzbot report. How did you notice this? I've found this by code review. I did this last week and I have 10 patches more on different theme. I was waiting for when the merge window opens. > >> --- >> fs/fuse/dev.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c >> index ae813e609932..6fe330cc9709 100644 >> --- a/fs/fuse/dev.c >> +++ b/fs/fuse/dev.c >> @@ -1768,8 +1768,10 @@ static int fuse_retrieve(struct fuse_conn *fc, struct inode *inode, >> req->in.args[1].size = total_len; >> >> err = fuse_request_send_notify_reply(fc, req, outarg->notify_unique); >> - if (err) >> + if (err) { >> fuse_retrieve_end(fc, req); >> + fuse_put_request(fc, req); >> + } >> >> return err; >> } >>
On Tue, Nov 6, 2018 at 10:25 AM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote: > On 06.11.2018 12:23, Miklos Szeredi wrote: >> On Tue, Nov 6, 2018 at 10:13 AM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote: >>> fuse_request_send_notify_reply() may fail, and this case >>> it remains leaked (fuse_retrieve_end(), which is called >>> on error path, does not do that). Also, fc->num_waiting, >>> will never be decremented, and fuse_wait_aborted() will >>> never finish. So, put the request patently. >>> >>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> >> >> Posted same patch yesterday for a syzbot report. How did you notice this? > > I've found this by code review. I did this last week and I have 10 patches more > on different theme. I was waiting for when the merge window opens. Well, the merge window just closed. But never worry, bugfixes can go in at anytime. If you notice a bug, such as this, you don't need to hold back until any particular time, the sooner it's known, the better. Thanks, Miklos
On 06.11.2018 12:33, Miklos Szeredi wrote: > On Tue, Nov 6, 2018 at 10:25 AM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote: >> On 06.11.2018 12:23, Miklos Szeredi wrote: >>> On Tue, Nov 6, 2018 at 10:13 AM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote: >>>> fuse_request_send_notify_reply() may fail, and this case >>>> it remains leaked (fuse_retrieve_end(), which is called >>>> on error path, does not do that). Also, fc->num_waiting, >>>> will never be decremented, and fuse_wait_aborted() will >>>> never finish. So, put the request patently. >>>> >>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> >>> >>> Posted same patch yesterday for a syzbot report. How did you notice this? >> >> I've found this by code review. I did this last week and I have 10 patches more >> on different theme. I was waiting for when the merge window opens. > > Well, the merge window just closed. But never worry, bugfixes can go > in at anytime. > > If you notice a bug, such as this, you don't need to hold back until > any particular time, the sooner it's known, the better. Ok, no problem :) Thanks, Kirill
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index ae813e609932..6fe330cc9709 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -1768,8 +1768,10 @@ static int fuse_retrieve(struct fuse_conn *fc, struct inode *inode, req->in.args[1].size = total_len; err = fuse_request_send_notify_reply(fc, req, outarg->notify_unique); - if (err) + if (err) { fuse_retrieve_end(fc, req); + fuse_put_request(fc, req); + } return err; }
fuse_request_send_notify_reply() may fail, and this case it remains leaked (fuse_retrieve_end(), which is called on error path, does not do that). Also, fc->num_waiting, will never be decremented, and fuse_wait_aborted() will never finish. So, put the request patently. Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> --- fs/fuse/dev.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)