[PATCH 0/2] ext4: Annotate two harmless KCSAN data races

[PATCH 0/2] ext4: Annotate two harmless KCSAN data races

[Jan Kara]

Hello,

the two patches in this series annotate two known (and known to be harmless)
data races in ext4. In the first case, let's also add a comment explaining why
the race is harmless.

								Honza

[PATCH 1/2] ext4: Annotate data race in start_this_handle()

[Jan Kara]

Access to journal->j_running_transaction is not protected by appropriate
lock and thus is racy. We are well aware of that and the code handles
the race properly. Just add a comment and data_race() annotation.

Reported-by: syzbot+30774a6acf6a2cf6d535@syzkaller.appspotmail.com
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/transaction.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 9396666b7314..398d1d9209e2 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -349,7 +349,12 @@ static int start_this_handle(journal_t *journal, handle_t *handle,
 	}
 
 alloc_transaction:
-	if (!journal->j_running_transaction) {
+	/*
+	 * This check is racy but it is just an optimization of allocating new
+	 * transaction early if there are high chances we'll need it. If we
+	 * guess wrong, we'll retry or free unused transaction.
+	 */
+	if (!data_race(journal->j_running_transaction)) {
 		/*
 		 * If __GFP_FS is not present, then we may be being called from
 		 * inside the fs writeback layer, so we MUST NOT fail.
-- 
2.26.2

Re: [PATCH 1/2] ext4: Annotate data race in start_this_handle()

[Theodore Ts'o]

On Tue, Apr 06, 2021 at 06:17:59PM +0200, Jan Kara wrote:
> Access to journal->j_running_transaction is not protected by appropriate
> lock and thus is racy. We are well aware of that and the code handles
> the race properly. Just add a comment and data_race() annotation.
> 
> Reported-by: syzbot+30774a6acf6a2cf6d535@syzkaller.appspotmail.com
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

[PATCH 2/2] ext4: Annotate data race in jbd2_journal_dirty_metadata()

[Jan Kara]

Assertion checks in jbd2_journal_dirty_metadata() are known to be racy
but we don't want to be grabbing locks just for them.  We thus recheck
them under b_state_lock only if it looks like they would fail. Annotate
the checks with data_race().

Reported-by: Hao Sun <sunhao.th@gmail.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/transaction.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 398d1d9209e2..e8fc45fd751f 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -1479,8 +1479,8 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh)
 	 * crucial to catch bugs so let's do a reliable check until the
 	 * lockless handling is fully proven.
 	 */
-	if (jh->b_transaction != transaction &&
-	    jh->b_next_transaction != transaction) {
+	if (data_race(jh->b_transaction != transaction &&
+	    jh->b_next_transaction != transaction)) {
 		spin_lock(&jh->b_state_lock);
 		J_ASSERT_JH(jh, jh->b_transaction == transaction ||
 				jh->b_next_transaction == transaction);
@@ -1488,8 +1488,8 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh)
 	}
 	if (jh->b_modified == 1) {
 		/* If it's in our transaction it must be in BJ_Metadata list. */
-		if (jh->b_transaction == transaction &&
-		    jh->b_jlist != BJ_Metadata) {
+		if (data_race(jh->b_transaction == transaction &&
+		    jh->b_jlist != BJ_Metadata)) {
 			spin_lock(&jh->b_state_lock);
 			if (jh->b_transaction == transaction &&
 			    jh->b_jlist != BJ_Metadata)
-- 
2.26.2

Re: [PATCH 2/2] ext4: Annotate data race in jbd2_journal_dirty_metadata()

[Theodore Ts'o]

On Tue, Apr 06, 2021 at 06:18:00PM +0200, Jan Kara wrote:
> Assertion checks in jbd2_journal_dirty_metadata() are known to be racy
> but we don't want to be grabbing locks just for them.  We thus recheck
> them under b_state_lock only if it looks like they would fail. Annotate
> the checks with data_race().
> 
> Reported-by: Hao Sun <sunhao.th@gmail.com>
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted