Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1570804 > unrolled thread
| Started by | Sahitya Tummala <stummala@codeaurora.org> |
|---|---|
| First post | 2017-01-31 15:50 +0100 |
| Last post | 2017-02-01 05:30 +0100 |
| Articles | 3 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH] jbd2: Fix use after free in kjournald2() Sahitya Tummala <stummala@codeaurora.org> - 2017-01-31 15:50 +0100
Re: [PATCH] jbd2: Fix use after free in kjournald2() Jan Kara <jack@suse.cz> - 2017-01-31 17:00 +0100
Re: [PATCH] jbd2: Fix use after free in kjournald2() "Tummala, Sahitya" <stummala@codeaurora.org> - 2017-02-01 05:30 +0100
| From | Sahitya Tummala <stummala@codeaurora.org> |
|---|---|
| Date | 2017-01-31 15:50 +0100 |
| Subject | [PATCH] jbd2: Fix use after free in kjournald2() |
| Message-ID | <t5HUS-1S1-29@gated-at.bofh.it> |
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(+)
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;
}
--
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.
[toc] | [next] | [standalone]
| From | Jan Kara <jack@suse.cz> |
|---|---|
| Date | 2017-01-31 17:00 +0100 |
| Message-ID | <t5J0C-2tO-31@gated-at.bofh.it> |
| In reply to | #1570804 |
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.
>
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
[toc] | [prev] | [next] | [standalone]
| From | "Tummala, Sahitya" <stummala@codeaurora.org> |
|---|---|
| Date | 2017-02-01 05:30 +0100 |
| Message-ID | <t5UIp-1aG-1@gated-at.bofh.it> |
| In reply to | #1570848 |
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.
>>
>>
--
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.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web