From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751146AbdBAEWt (ORCPT ); Tue, 31 Jan 2017 23:22:49 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:55470 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750784AbdBAEWr (ORCPT ); Tue, 31 Jan 2017 23:22:47 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org A474D60290 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=stummala@codeaurora.org Subject: Re: [PATCH] jbd2: Fix use after free in kjournald2() To: Jan Kara References: <1485873537-32514-1-git-send-email-stummala@codeaurora.org> <20170131155155.GC15249@quack2.suse.cz> Cc: "Theodore Ts'o" , Jan Kara , linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org From: "Tummala, Sahitya" Message-ID: <5c4bcc82-386a-a8b1-a752-42ec89446df4@codeaurora.org> Date: Wed, 1 Feb 2017 09:52:41 +0530 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <20170131155155.GC15249@quack2.suse.cz> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > 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.