* [PATCH] jbd2: Fix use after free in kjournald2()
@ 2017-01-31 14:38 Sahitya Tummala
2017-01-31 15:51 ` Jan Kara
0 siblings, 1 reply; 5+ messages in thread
From: Sahitya Tummala @ 2017-01-31 14:38 UTC (permalink / raw)
To: Theodore Ts'o, Jan Kara, linux-ext4, linux-kernel; +Cc: Sahitya Tummala
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.
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] jbd2: Fix use after free in kjournald2()
2017-01-31 14:38 Sahitya Tummala
@ 2017-01-31 15:51 ` Jan Kara
2017-02-01 4:22 ` Tummala, Sahitya
0 siblings, 1 reply; 5+ messages in thread
From: Jan Kara @ 2017-01-31 15:51 UTC (permalink / raw)
To: Sahitya Tummala; +Cc: Theodore Ts'o, Jan Kara, linux-ext4, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] jbd2: Fix use after free in kjournald2()
2017-01-31 15:51 ` Jan Kara
@ 2017-02-01 4:22 ` Tummala, Sahitya
0 siblings, 0 replies; 5+ messages in thread
From: Tummala, Sahitya @ 2017-02-01 4:22 UTC (permalink / raw)
To: Jan Kara; +Cc: Theodore Ts'o, Jan Kara, linux-ext4, linux-kernel
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] jbd2: Fix use after free in kjournald2()
@ 2017-02-01 17:31 Sahitya Tummala
2017-02-02 1:56 ` Theodore Ts'o
0 siblings, 1 reply; 5+ messages in thread
From: Sahitya Tummala @ 2017-02-01 17:31 UTC (permalink / raw)
To: Theodore Ts'o, Jan Kara, linux-ext4, linux-kernel; +Cc: Sahitya Tummala
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, 1 insertion(+), 1 deletion(-)
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index a097048..85d1483 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -276,11 +276,11 @@ static int kjournald2(void *arg)
goto loop;
end_loop:
- write_unlock(&journal->j_state_lock);
del_timer_sync(&journal->j_commit_timer);
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.
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] jbd2: Fix use after free in kjournald2()
2017-02-01 17:31 [PATCH] jbd2: Fix use after free in kjournald2() Sahitya Tummala
@ 2017-02-02 1:56 ` Theodore Ts'o
0 siblings, 0 replies; 5+ messages in thread
From: Theodore Ts'o @ 2017-02-02 1:56 UTC (permalink / raw)
To: Sahitya Tummala; +Cc: Jan Kara, linux-ext4, linux-kernel
On Wed, Feb 01, 2017 at 11:01:51PM +0530, 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().
Thanks, applied.
- Ted
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-02-02 1:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-01 17:31 [PATCH] jbd2: Fix use after free in kjournald2() Sahitya Tummala
2017-02-02 1:56 ` Theodore Ts'o
-- strict thread matches above, loose matches on Subject: below --
2017-01-31 14:38 Sahitya Tummala
2017-01-31 15:51 ` Jan Kara
2017-02-01 4:22 ` Tummala, Sahitya
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).