Linux-ext4 Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] ext4: Annotate two harmless KCSAN data races
@ 2021-04-06 16:17 Jan Kara
  2021-04-06 16:17 ` [PATCH 1/2] ext4: Annotate data race in start_this_handle() Jan Kara
  2021-04-06 16:18 ` [PATCH 2/2] ext4: Annotate data race in jbd2_journal_dirty_metadata() Jan Kara
  0 siblings, 2 replies; 5+ messages in thread
From: Jan Kara @ 2021-04-06 16:17 UTC (permalink / raw)
  To: Ted Tso; +Cc: linux-ext4, 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/2] ext4: Annotate data race in start_this_handle()
  2021-04-06 16:17 [PATCH 0/2] ext4: Annotate two harmless KCSAN data races Jan Kara
@ 2021-04-06 16:17 ` Jan Kara
  2021-04-10  1:24   ` Theodore Ts'o
  2021-04-06 16:18 ` [PATCH 2/2] ext4: Annotate data race in jbd2_journal_dirty_metadata() Jan Kara
  1 sibling, 1 reply; 5+ messages in thread
From: Jan Kara @ 2021-04-06 16:17 UTC (permalink / raw)
  To: Ted Tso; +Cc: linux-ext4, Jan Kara, syzbot+30774a6acf6a2cf6d535

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/2] ext4: Annotate data race in jbd2_journal_dirty_metadata()
  2021-04-06 16:17 [PATCH 0/2] ext4: Annotate two harmless KCSAN data races Jan Kara
  2021-04-06 16:17 ` [PATCH 1/2] ext4: Annotate data race in start_this_handle() Jan Kara
@ 2021-04-06 16:18 ` Jan Kara
  2021-04-10  1:24   ` Theodore Ts'o
  1 sibling, 1 reply; 5+ messages in thread
From: Jan Kara @ 2021-04-06 16:18 UTC (permalink / raw)
  To: Ted Tso; +Cc: linux-ext4, Jan Kara, Hao Sun

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] ext4: Annotate data race in start_this_handle()
  2021-04-06 16:17 ` [PATCH 1/2] ext4: Annotate data race in start_this_handle() Jan Kara
@ 2021-04-10  1:24   ` Theodore Ts'o
  0 siblings, 0 replies; 5+ messages in thread
From: Theodore Ts'o @ 2021-04-10  1:24 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-ext4, syzbot+30774a6acf6a2cf6d535

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] ext4: Annotate data race in jbd2_journal_dirty_metadata()
  2021-04-06 16:18 ` [PATCH 2/2] ext4: Annotate data race in jbd2_journal_dirty_metadata() Jan Kara
@ 2021-04-10  1:24   ` Theodore Ts'o
  0 siblings, 0 replies; 5+ messages in thread
From: Theodore Ts'o @ 2021-04-10  1:24 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-ext4, Hao Sun

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-06 16:17 [PATCH 0/2] ext4: Annotate two harmless KCSAN data races Jan Kara
2021-04-06 16:17 ` [PATCH 1/2] ext4: Annotate data race in start_this_handle() Jan Kara
2021-04-10  1:24   ` Theodore Ts'o
2021-04-06 16:18 ` [PATCH 2/2] ext4: Annotate data race in jbd2_journal_dirty_metadata() Jan Kara
2021-04-10  1:24   ` Theodore Ts'o

Linux-ext4 Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-ext4/0 linux-ext4/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-ext4 linux-ext4/ https://lore.kernel.org/linux-ext4 \
		linux-ext4@vger.kernel.org
	public-inbox-index linux-ext4

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-ext4


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git