| Message ID | 1485873537-32514-1-git-send-email-stummala@codeaurora.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue 31-01-17 20:08:57, Sahitya Tummala wrote: > Below is the synchronization issue between unmount and kjournald2 > contexts, which results into use after free issue in kjournald2(). > Fix this issue by using journal->j_state_lock to synchronize the > wait_event() done in journal_kill_thread() and the wake_up() done > in kjournald2(). > > TASK 1: > umount cmd: > |--jbd2_journal_destroy() { > |--journal_kill_thread() { > write_lock(&journal->j_state_lock); > journal->j_flags |= JBD2_UNMOUNT; > ... > write_unlock(&journal->j_state_lock); > wake_up(&journal->j_wait_commit); TASK 2 wakes up here: > kjournald2() { > ... > checks JBD2_UNMOUNT flag and calls goto end-loop; > ... > end_loop: > write_unlock(&journal->j_state_lock); > journal->j_task = NULL; --> If this thread gets > pre-empted here, then TASK 1 wait_event will > exit even before this thread is completely > done. > wait_event(journal->j_wait_done_commit, journal->j_task == NULL); > ... > write_lock(&journal->j_state_lock); > write_unlock(&journal->j_state_lock); > } > |--kfree(journal); > } > } > wake_up(&journal->j_wait_done_commit); --> this step > now results into use after free issue. > } > > Signed-off-by: Sahitya Tummala <stummala@codeaurora.org> Yeah, what you write looks possible (although rather unlikely). Thanks for catching this. One small nit below: > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c > index a097048..f5cd3c0 100644 > --- a/fs/jbd2/journal.c > +++ b/fs/jbd2/journal.c > @@ -278,9 +278,11 @@ static int kjournald2(void *arg) > end_loop: > write_unlock(&journal->j_state_lock); > del_timer_sync(&journal->j_commit_timer); > + write_lock(&journal->j_state_lock); There's no good reason to do del_timer_sync() outside of j_state_lock. This is not performance critical code and commit_timeout is trivial and cannot block on anything. So just keep j_state_lock locked upto the place where you unlock it now... Honza > journal->j_task = NULL; > wake_up(&journal->j_wait_done_commit); > jbd_debug(1, "Journal thread exiting.\n"); > + write_unlock(&journal->j_state_lock); > return 0; > } > > -- > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. > >
On 1/31/2017 9:21 PM, Jan Kara wrote: > On Tue 31-01-17 20:08:57, Sahitya Tummala wrote: >> Below is the synchronization issue between unmount and kjournald2 >> contexts, which results into use after free issue in kjournald2(). >> Fix this issue by using journal->j_state_lock to synchronize the >> wait_event() done in journal_kill_thread() and the wake_up() done >> in kjournald2(). >> >> TASK 1: >> umount cmd: >> |--jbd2_journal_destroy() { >> |--journal_kill_thread() { >> write_lock(&journal->j_state_lock); >> journal->j_flags |= JBD2_UNMOUNT; >> ... >> write_unlock(&journal->j_state_lock); >> wake_up(&journal->j_wait_commit); TASK 2 wakes up here: >> kjournald2() { >> ... >> checks JBD2_UNMOUNT flag and calls goto end-loop; >> ... >> end_loop: >> write_unlock(&journal->j_state_lock); >> journal->j_task = NULL; --> If this thread gets >> pre-empted here, then TASK 1 wait_event will >> exit even before this thread is completely >> done. >> wait_event(journal->j_wait_done_commit, journal->j_task == NULL); >> ... >> write_lock(&journal->j_state_lock); >> write_unlock(&journal->j_state_lock); >> } >> |--kfree(journal); >> } >> } >> wake_up(&journal->j_wait_done_commit); --> this step >> now results into use after free issue. >> } >> >> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org> > Yeah, what you write looks possible (although rather unlikely). Thanks for > catching this. One small nit below: Yes, it was observed only once and is very hard to reproduce. >> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c >> index a097048..f5cd3c0 100644 >> --- a/fs/jbd2/journal.c >> +++ b/fs/jbd2/journal.c >> @@ -278,9 +278,11 @@ static int kjournald2(void *arg) >> end_loop: >> write_unlock(&journal->j_state_lock); >> del_timer_sync(&journal->j_commit_timer); >> + write_lock(&journal->j_state_lock); > There's no good reason to do del_timer_sync() outside of j_state_lock. This > is not performance critical code and commit_timeout is trivial and cannot > block on anything. So just keep j_state_lock locked upto the place where > you unlock it now... > Sure, I will update the patch. > Honza >> journal->j_task = NULL; >> wake_up(&journal->j_wait_done_commit); >> jbd_debug(1, "Journal thread exiting.\n"); >> + write_unlock(&journal->j_state_lock); >> return 0; >> } >> >> -- >> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. >> >>
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index a097048..f5cd3c0 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -278,9 +278,11 @@ static int kjournald2(void *arg) end_loop: write_unlock(&journal->j_state_lock); del_timer_sync(&journal->j_commit_timer); + write_lock(&journal->j_state_lock); journal->j_task = NULL; wake_up(&journal->j_wait_done_commit); jbd_debug(1, "Journal thread exiting.\n"); + write_unlock(&journal->j_state_lock); return 0; }
Below is the synchronization issue between unmount and kjournald2 contexts, which results into use after free issue in kjournald2(). Fix this issue by using journal->j_state_lock to synchronize the wait_event() done in journal_kill_thread() and the wake_up() done in kjournald2(). TASK 1: umount cmd: |--jbd2_journal_destroy() { |--journal_kill_thread() { write_lock(&journal->j_state_lock); journal->j_flags |= JBD2_UNMOUNT; ... write_unlock(&journal->j_state_lock); wake_up(&journal->j_wait_commit); TASK 2 wakes up here: kjournald2() { ... checks JBD2_UNMOUNT flag and calls goto end-loop; ... end_loop: write_unlock(&journal->j_state_lock); journal->j_task = NULL; --> If this thread gets pre-empted here, then TASK 1 wait_event will exit even before this thread is completely done. wait_event(journal->j_wait_done_commit, journal->j_task == NULL); ... write_lock(&journal->j_state_lock); write_unlock(&journal->j_state_lock); } |--kfree(journal); } } wake_up(&journal->j_wait_done_commit); --> this step now results into use after free issue. } Signed-off-by: Sahitya Tummala <stummala@codeaurora.org> --- fs/jbd2/journal.c | 2 ++ 1 file changed, 2 insertions(+)