[PATCH] jbd2: set JBD2_LOADED under j_state_lock

[PATCH] jbd2: set JBD2_LOADED under j_state_lock

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

j_flags is protected by j_state_lock, with only quick racy checks done
without it.  jbd2_journal_load() however sets JBD2_LOADED with a plain
read-modify-write after journal_reset() has already started kjournald2,
which tests and may update j_flags under the lock.  KCSAN reports this
while mounting the root file system:

  BUG: KCSAN: data-race in jbd2_journal_load / kjournald2

  write to 0xffff9c6b012c6000 of 8 bytes by task 1 on cpu 3:
   jbd2_journal_load+0x279/0x3c0
   ext4_fill_super+0x34dc/0x5740
   get_tree_bdev_flags+0x21d/0x340
   ...
  read to 0xffff9c6b012c6000 of 8 bytes by task 77 on cpu 0:
   kjournald2+0xc8/0x3e0
   kthread+0x1bd/0x200
   ...

  value changed: 0x00000000000000a8 -> 0x00000000000000b8

A concurrent update of another flag, e.g. JBD2_ABORT, could be lost.
Take j_state_lock when setting JBD2_LOADED, like the other writers of
j_flags do.

Fixes: 470decc613ab ("[PATCH] jbd2: initial copy of files from jbd")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 fs/jbd2/journal.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 00f5a98f3d4f..fa91093a2284 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -2091,7 +2091,10 @@ int jbd2_journal_load(journal_t *journal)
 		return err;
 	}
 
+	/* kjournald2 is already running, see journal_reset(). */
+	write_lock(&journal->j_state_lock);
 	journal->j_flags |= JBD2_LOADED;
+	write_unlock(&journal->j_state_lock);
 	return 0;
 }
 
-- 
2.55.0

Re: [PATCH] jbd2: set JBD2_LOADED under j_state_lock

[sashiko-bot]

> From: Christian Göttsche <cgzones@googlemail.com>
> 
> j_flags is protected by j_state_lock, with only quick racy checks done
> without it.  jbd2_journal_load() however sets JBD2_LOADED with a plain
> read-modify-write after journal_reset() has already started kjournald2,
> which tests and may update j_flags under the lock.  KCSAN reports this
> while mounting the root file system:
> 
>   BUG: KCSAN: data-race in jbd2_journal_load / kjournald2
> 
>   write to 0xffff9c6b012c6000 of 8 bytes by task 1 on cpu 3:
>    jbd2_journal_load+0x279/0x3c0
>    ext4_fill_super+0x34dc/0x5740
>    get_tree_bdev_flags+0x21d/0x340
>    ...
>   read to 0xffff9c6b012c6000 of 8 bytes by task 77 on cpu 0:
>    kjournald2+0xc8/0x3e0
>    kthread+0x1bd/0x200
>    ...
> 
>   value changed: 0x00000000000000a8 -> 0x00000000000000b8
> 
> A concurrent update of another flag, e.g. JBD2_ABORT, could be lost.
> Take j_state_lock when setting JBD2_LOADED, like the other writers of
> j_flags do.
> 
> Fixes: 470decc613ab ("[PATCH] jbd2: initial copy of files from jbd")
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928202159.2463182-1-cgoettsche@seltendoof.de?part=1

Re: [PATCH] jbd2: set JBD2_LOADED under j_state_lock

[Jan Kara]

On Mon 28-09-26 22:21:59, Christian Göttsche wrote:
> From: Christian Göttsche <cgzones@googlemail.com>
> 
> j_flags is protected by j_state_lock, with only quick racy checks done
> without it.  jbd2_journal_load() however sets JBD2_LOADED with a plain
> read-modify-write after journal_reset() has already started kjournald2,
> which tests and may update j_flags under the lock.  KCSAN reports this
> while mounting the root file system:
> 
>   BUG: KCSAN: data-race in jbd2_journal_load / kjournald2
> 
>   write to 0xffff9c6b012c6000 of 8 bytes by task 1 on cpu 3:
>    jbd2_journal_load+0x279/0x3c0
>    ext4_fill_super+0x34dc/0x5740
>    get_tree_bdev_flags+0x21d/0x340
>    ...
>   read to 0xffff9c6b012c6000 of 8 bytes by task 77 on cpu 0:
>    kjournald2+0xc8/0x3e0
>    kthread+0x1bd/0x200
>    ...
> 
>   value changed: 0x00000000000000a8 -> 0x00000000000000b8
> 
> A concurrent update of another flag, e.g. JBD2_ABORT, could be lost.
> Take j_state_lock when setting JBD2_LOADED, like the other writers of
> j_flags do.
> 
> Fixes: 470decc613ab ("[PATCH] jbd2: initial copy of files from jbd")
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Good point. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

One nit below:

> ---
>  fs/jbd2/journal.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 00f5a98f3d4f..fa91093a2284 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -2091,7 +2091,10 @@ int jbd2_journal_load(journal_t *journal)
>  		return err;
>  	}
>  
> +	/* kjournald2 is already running, see journal_reset(). */

Maybe I'd write here:
	/* kjournald was already started in journal_reset() so we need to lock */ 

> +	write_lock(&journal->j_state_lock);
>  	journal->j_flags |= JBD2_LOADED;
> +	write_unlock(&journal->j_state_lock);
>  	return 0;

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR