jbd2: fix incorrect unlock on j_list_lock

PATCH Applied Taesoo Kim 5 participants 8 messages Mar 18, 2015 – Oct 13, 2016

Subsystems (2)

Stable branch Journalling layer for block devices (JBD2)

lore

Share & tools

lore

Lukáš Czerner

Reviewed-by on v1 1/1

2015-03-18 10:43 UTC #1 lore raw

Quoted 7 lines · Taesoo Kim

Date: Tue, 17 Mar 2015 22:08:38 -0400
From: Taesoo Kim <[email protected]>
To: [email protected], [email protected], [email protected]
Cc: [email protected], [email protected], [email protected],
[email protected], [email protected], Taesoo Kim <[email protected]>
Subject: [PATCH 1/1] jbd2: fix incorrect unlock on j_list_lock

Quoted 7 lines

When 'jh->b_transaction == transaction' (asserted by below)
J_ASSERT_JH(jh, (jh->b_transaction == transaction || ...
'journal->j_list_lock' will be incorrectly unlocked, since the the lock is aquired only at the end of if / else-if statements (missing the else case).

The patch looks good, thanks.

Reviewed-by Lukas Czerner <[email protected]>

Btw, were you able to reproduce the problem, or have you seen the problem in the wild ? Or did you just spot it in the code ?

Thanks!
-Lukas

Quoted 36 lines

Signed-off-by Taesoo Kim <[email protected]>

---
 fs/jbd2/transaction.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/jbd2/transaction.c +2 -2

index 5f09370..edb7f59 100644
@@ -1091,6 +1091,7 @@ int jbd2_journal_get_create_access(handle_t *handle, struct buffer_head *bh)
 		JBUFFER_TRACE(jh, "file as BJ_Reserved");
 		spin_lock(&journal->j_list_lock);
 		__jbd2_journal_file_buffer(jh, transaction, BJ_Reserved);
+		spin_unlock(&journal->j_list_lock);
 	} else if (jh->b_transaction == journal->j_committing_transaction) {
 		/* first access by this transaction */
 		jh->b_modified = 0;

@@ -1098,8 +1099,8 @@ int jbd2_journal_get_create_access(handle_t *handle, struct buffer_head *bh)
 		JBUFFER_TRACE(jh, "set next transaction");
 		spin_lock(&journal->j_list_lock);
 		jh->b_next_transaction = transaction;
+		spin_unlock(&journal->j_list_lock);
 	}
-	spin_unlock(&journal->j_list_lock);
 	jbd_unlock_bh_state(bh);
 
 	/*
--

2.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Taesoo Kim

2015-03-18 17:39 UTC #2 lore raw

Quoted 1 line

The patch looks good, thanks.

Thank you.

Quoted 4 lines

Reviewed-by Lukas Czerner <[email protected]>
Btw, were you able to reproduce the problem, or have you seen the problem in the wild ? Or did you just spot it in the code ?

We are developing a static checker to spot inconsistent programming patterns; our first goal is to scan over existing filesystems and figure out how they are implemented differently (or similarly). We will report bugs in sequence as soon as our team confirm (just start sending patches to other fs).

Thanks,
Taesoo

Quoted 39 lines

Thanks!
-Lukas

Quoted 36 lines

Signed-off-by Taesoo Kim <[email protected]>

---
 fs/jbd2/transaction.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/jbd2/transaction.c +2 -2

index 5f09370..edb7f59 100644
@@ -1091,6 +1091,7 @@ int jbd2_journal_get_create_access(handle_t *handle, struct buffer_head *bh)
 		JBUFFER_TRACE(jh, "file as BJ_Reserved");
 		spin_lock(&journal->j_list_lock);
 		__jbd2_journal_file_buffer(jh, transaction, BJ_Reserved);
+		spin_unlock(&journal->j_list_lock);
 	} else if (jh->b_transaction == journal->j_committing_transaction) {
 		/* first access by this transaction */
 		jh->b_modified = 0;

@@ -1098,8 +1099,8 @@ int jbd2_journal_get_create_access(handle_t *handle, struct buffer_head *bh)
 		JBUFFER_TRACE(jh, "set next transaction");
 		spin_lock(&journal->j_list_lock);
 		jh->b_next_transaction = transaction;
+		spin_unlock(&journal->j_list_lock);
 	}
-	spin_unlock(&journal->j_list_lock);
 	jbd_unlock_bh_state(bh);
 
 	/*
--

2.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Lukáš Czerner

2015-03-19 09:48 UTC #3 lore raw

Quoted 15 lines · Taesoo Kim

Date: Wed, 18 Mar 2015 13:39:31 -0400
From: Taesoo Kim <[email protected]>
To: Lukáš Czerner <[email protected]>
Cc: Taesoo Kim <[email protected]>, [email protected],
[email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH 1/1] jbd2: fix incorrect unlock on j_list_lock
Quoted 1 line
The patch looks good, thanks.
Thank you.
Quoted 2 lines
Reviewed-by Lukas Czerner <[email protected]>

Quoted 8 lines

Quoted 2 lines
Btw, were you able to reproduce the problem, or have you seen the problem in the wild ? Or did you just spot it in the code ?
We are developing a static checker to spot inconsistent programming patterns; our first goal is to scan over existing filesystems and figure out how they are implemented differently (or similarly). We will report bugs in sequence as soon as our team confirm (just start sending patches to other fs).

And this was found but it ?

But anyway it sounds really interesting, do you have any more information you can share about the project ? Project website, description, or source code would be great :)

Thanks!
-Lukas

Quoted 44 lines

Thanks,
Taesoo

Quoted 39 lines

Thanks!
-Lukas

Quoted 36 lines

Signed-off-by Taesoo Kim <[email protected]>

---
 fs/jbd2/transaction.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/jbd2/transaction.c +2 -2

index 5f09370..edb7f59 100644
@@ -1091,6 +1091,7 @@ int jbd2_journal_get_create_access(handle_t *handle, struct buffer_head *bh)
 		JBUFFER_TRACE(jh, "file as BJ_Reserved");
 		spin_lock(&journal->j_list_lock);
 		__jbd2_journal_file_buffer(jh, transaction, BJ_Reserved);
+		spin_unlock(&journal->j_list_lock);
 	} else if (jh->b_transaction == journal->j_committing_transaction) {
 		/* first access by this transaction */
 		jh->b_modified = 0;

@@ -1098,8 +1099,8 @@ int jbd2_journal_get_create_access(handle_t *handle, struct buffer_head *bh)
 		JBUFFER_TRACE(jh, "set next transaction");
 		spin_lock(&journal->j_list_lock);
 		jh->b_next_transaction = transaction;
+		spin_unlock(&journal->j_list_lock);
 	}
-	spin_unlock(&journal->j_list_lock);
 	jbd_unlock_bh_state(bh);
 
 	/*
--

2.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Taesoo Kim

2015-03-19 12:02 UTC #4 lore raw

Quoted 20 lines · Lukáš Czerner

Quoted 18 lines · Taesoo Kim
Date: Wed, 18 Mar 2015 13:39:31 -0400
From: Taesoo Kim <[email protected]>
To: Lukáš Czerner <[email protected]>
Cc: Taesoo Kim <[email protected]>, [email protected],
[email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH 1/1] jbd2: fix incorrect unlock on j_list_lock
Quoted 1 line
The patch looks good, thanks.
Thank you.
Quoted 4 lines
Reviewed-by Lukas Czerner <[email protected]>
Btw, were you able to reproduce the problem, or have you seen the problem in the wild ? Or did you just spot it in the code ?

Quoted 7 lines

Quoted 5 lines
We are developing a static checker to spot inconsistent programming patterns; our first goal is to scan over existing filesystems and figure out how they are implemented differently (or similarly). We will report bugs in sequence as soon as our team confirm (just start sending patches to other fs).
And this was found but it ?

Our current prototype.

Quoted 3 lines

But anyway it sounds really interesting, do you have any more information you can share about the project ? Project website, description, or source code would be great :)

We need 1-2 months to wrap up. I would say, right after the deadline, we plan to make the code/result publicly available :)

Taesoo

Quoted 47 lines

Thanks!
-Lukas

Quoted 44 lines

Thanks,
Taesoo

Quoted 39 lines

Thanks!
-Lukas

Quoted 36 lines

Signed-off-by Taesoo Kim <[email protected]>

---
 fs/jbd2/transaction.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/jbd2/transaction.c +2 -2

index 5f09370..edb7f59 100644
@@ -1091,6 +1091,7 @@ int jbd2_journal_get_create_access(handle_t *handle, struct buffer_head *bh)
 		JBUFFER_TRACE(jh, "file as BJ_Reserved");
 		spin_lock(&journal->j_list_lock);
 		__jbd2_journal_file_buffer(jh, transaction, BJ_Reserved);
+		spin_unlock(&journal->j_list_lock);
 	} else if (jh->b_transaction == journal->j_committing_transaction) {
 		/* first access by this transaction */
 		jh->b_modified = 0;

@@ -1098,8 +1099,8 @@ int jbd2_journal_get_create_access(handle_t *handle, struct buffer_head *bh)
 		JBUFFER_TRACE(jh, "set next transaction");
 		spin_lock(&journal->j_list_lock);
 		jh->b_next_transaction = transaction;
+		spin_unlock(&journal->j_list_lock);
 	}
-	spin_unlock(&journal->j_list_lock);
 	jbd_unlock_bh_state(bh);
 
 	/*
--

2.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Lukáš Czerner

2015-03-19 12:13 UTC #5 lore raw

Quoted 38 lines · Taesoo Kim

Date: Thu, 19 Mar 2015 08:02:43 -0400
From: Taesoo Kim <[email protected]>
To: Lukáš Czerner <[email protected]>
Cc: Taesoo Kim <[email protected]>, [email protected],
[email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH 1/1] jbd2: fix incorrect unlock on j_list_lock
Quoted 20 lines · Lukáš Czerner
Quoted 18 lines · Taesoo Kim
Date: Wed, 18 Mar 2015 13:39:31 -0400
From: Taesoo Kim <[email protected]>
To: Lukáš Czerner <[email protected]>
Cc: Taesoo Kim <[email protected]>, [email protected],
[email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH 1/1] jbd2: fix incorrect unlock on j_list_lock
Quoted 1 line
The patch looks good, thanks.
Thank you.
Quoted 4 lines
Reviewed-by Lukas Czerner <[email protected]>
Btw, were you able to reproduce the problem, or have you seen the problem in the wild ? Or did you just spot it in the code ?
Quoted 7 lines
Quoted 5 lines
We are developing a static checker to spot inconsistent programming patterns; our first goal is to scan over existing filesystems and figure out how they are implemented differently (or similarly). We will report bugs in sequence as soon as our team confirm (just start sending patches to other fs).
And this was found but it ?

Quoted 8 lines

Our current prototype.
Quoted 3 lines
But anyway it sounds really interesting, do you have any more information you can share about the project ? Project website, description, or source code would be great :)
We need 1-2 months to wrap up. I would say, right after the deadline, we plan to make the code/result publicly available :)

Great! Looking forward to it.

Thanks!
-Lukas

Andreas Dilger

Reviewed-by on v1 1/1

2016-10-12 22:58 UTC #6 lore raw

In reply to Taesoo Kim

Quoted 20 lines · Taesoo Kim

When 'jh->b_transaction == transaction' (asserted by below)
        J_ASSERT_JH(jh, (jh->b_transaction == transaction || ...
'journal->j_list_lock' will be incorrectly unlocked, since spin_lock() is called only in the 'if' and 'else-if' blocks but not in the missing 'else' case, which results in a hang or an oops:
    ------------[ cut here ]------------
    kernel BUG at fs/jbd2/transaction.c:2239!
    invalid opcode: 0000 [#1] SMP
    RIP: 0010: __jbd2_journal_file_buffer+0x206/0x220 [jbd2]
    Call Trace:
    do_get_write_access+0x33c/0x4e0 [jbd2]
    jbd2_journal_get_write_access+0x27/0x40 [jbd2]
    __ext4_journal_get_write_access+0x3b/0x80 [ext4]
    ext4_delete_entry+0xa9/0x1a0 [ext4]
    ext4_unlink+0x600/0xa90 [ext4]

Quoted 5 lines

which is assert_spin_locked(&transaction->t_journal->j_list_lock).
Move spin_unlock() into the two cases where it is actually locked.
Signed-off-by Taesoo Kim <[email protected]>

We've hit this repeatedly on kernels with commit v3.14-rc2-30-g6e4862a "jbd2: minimize region locked by j_list_lock in journal_get_create_access" under heavy load and this patch has fixed the problem.

It should also be considered for stable kernels after 3.14.

[I've updated the above commit message slightly to give more details.]

Reviewed-by Andreas Dilger <[email protected]>

Quoted 27 lines

---
fs/jbd2/transaction.c | 3 ++-

1 file changed, 2 insertions(+), 1 deletion(-)

fs/jbd2/transaction.c +2 -2

index 5f09370..edb7f59 100644
@@ -1091,6 +1091,7 @@ int jbd2_journal_get_create_access(handle_t
 		JBUFFER_TRACE(jh, "file as BJ_Reserved");
 		spin_lock(&journal->j_list_lock);
 		__jbd2_journal_file_buffer(jh, transaction, BJ_Reserved);
+		spin_unlock(&journal->j_list_lock);
 	} else if (jh->b_transaction == journal->j_committing_transaction) {
 		/* first access by this transaction */
 		jh->b_modified = 0;

@@ -1098,8 +1099,8 @@ int jbd2_journal_get_create_access(handle_t
 		JBUFFER_TRACE(jh, "set next transaction");
 		spin_lock(&journal->j_list_lock);
 		jh->b_next_transaction = transaction;
+		spin_unlock(&journal->j_list_lock);
 	}
-	spin_unlock(&journal->j_list_lock);
 	jbd_unlock_bh_state(bh);
 
 	/*
--

Quoted 1 line

2.3.3

Cheers, Andreas

Theodore Ts'o

maintainer Applied

2016-10-13 03:21 UTC #7 lore raw

Quoted 16 lines · Andreas Dilger

Quoted 10 lines · Taesoo Kim
When 'jh->b_transaction == transaction' (asserted by below)
        J_ASSERT_JH(jh, (jh->b_transaction == transaction || ...
'journal->j_list_lock' will be incorrectly unlocked, since spin_lock() is called only in the 'if' and 'else-if' blocks but not in the missing 'else' case, which results in a hang or an oops....
Signed-off-by Taesoo Kim <[email protected]>
We've hit this repeatedly on kernels with commit v3.14-rc2-30-g6e4862a "jbd2: minimize region locked by j_list_lock in journal_get_create_access" under heavy load and this patch has fixed the problem.

Quoted 5 lines

It should also be considered for stable kernels after 3.14.
[I've updated the above commit message slightly to give more details.]
Reviewed-by Andreas Dilger <[email protected]>

Thanks, applied.

Apologies, this got lost which is why I hadn't handled it earlier.

- Ted