Re: [PATCH 3.2.x] jbd2: add mutex_lock on j_checkpoint_mutex in jbd2_journal_flush

[Ben Hutchings <ben@decadent.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 2659 bytes --]

On Sun, 2015-09-06 at 03:25 +0200, Bartosz Kwitniewski wrote:
> Commit a3ceb22921615827bfed39d7612a9a370bff0edb (upstream 
> 79feb521a44705262d15cc819a4117a447b11ea7) in 3.2.x tree introduced 
> __jbd2_update_log_tail which requires j_checkpoint_mutex, but locking of 
> j_checkpoint_mutex in jbd2_journal_flush was not backported from upstream.

Oops.

> Fixes kernel BUG at fs/jbd2/journal.c:832 (__jbd2_update_log_tail):
> [] ? jbd2_cleanup_journal_tail+0x5d/0x61 [jbd2]
> [] ? jbd2_journal_flush+0xc2/0x156 [jbd2]
> [] ? ext4_freeze+0x2f/0x71 [ext4]
> [] ? filemap_write_and_wait+0x26/0x32
> [] ? freeze_super+0x8c/0xdd
> [] ? freeze_bdev+0x5b/0xa1
> [] ? start_cow_session+0xb3/0x2d6 [hcpdriver]
> [] ? printk+0x40/0x49
> [] ? alloc_cts_session+0x2e/0x33 [hcpdriver]
> [] ? ioctl_start_hcp_session+0x131/0x20d [hcpdriver]
> [] ? handle_ioctlStartHC2+0x95/0x1ab [hcpdriver]
> [] ? cow_ioctl_unlocked+0x13/0x18 [hcpdriver]
> [] ? do_vfs_ioctl+0x55a/0x5a9
> [] ? pax_randomize_kstack+0x4c/0x60
> [] ? sysret_check+0x20/0x62
> [] ? do_sys_open+0x11e/0x130
> [] ? sys_ioctl+0x3c/0x5f
> [] ? system_call_fastpath+0x16/0x1b
> 
> Signed-off-by: Bartosz Kwitniewski <zerg2000@astral.org.pl>
> Cc: stable@vger.kernel.org
> ---
> --- fs/jbd2/journal.c.orig> 	> 2015-08-12 16:33:24.000000000 +0200
> +++ fs/jbd2/journal.c> 	> 2015-09-06 00:57:56.890894891 +0200
> @@ -1828,10 +1828,13 @@ int jbd2_journal_flush(journal_t *journa
>  > 	> if (is_journal_aborted(journal))
>  > 	> 	> return -EIO;
>  
> +> 	> mutex_lock(&journal->j_checkpoint_mutex);
>  > 	> if (!err) {
>  > 	> 	> err = jbd2_cleanup_journal_tail(journal);
> -> 	> 	> if (err < 0)
> +> 	> 	> if (err < 0) {
> +> 	> 	> 	> mutex_unlock(&journal->j_checkpoint_mutex);
>  > 	> 	> 	> goto out;
> +> 	> 	> }
>  > 	> 	> err = 0;
>  > 	> }
>  
> @@ -1841,6 +1844,7 @@ int jbd2_journal_flush(journal_t *journa
>  > 	>  * commits of data to the journal will restore the current
>  > 	>  * s_start value. */
>  > 	> jbd2_mark_journal_empty(journal);
> +> 	> mutex_unlock(&journal->j_checkpoint_mutex);
>  > 	> write_lock(&journal->j_state_lock);
>  > 	> J_ASSERT(!journal->j_running_transaction);
>  > 	> J_ASSERT(!journal->j_committing_transaction);

Why is it sufficient to add locking of j_checkpoint_mutex only in this
one function?

Shouldn't I cherry-pick commits 24bcc89c7e7c ("jbd2: split updating of
journal superblock and marking journal empty") and a78bb11d7acd ("jbd2:
protect all log tail updates with j_checkpoint_mutex") as well?

Ben.

-- 
Ben Hutchings
I say we take off; nuke the site from orbit.  It's the only way to be sure.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]