| Message ID | 20260928202159.2463182-1-cgoettsche@seltendoof.de |
|---|---|
| State | New |
| Headers | show |
| Series | jbd2: set JBD2_LOADED under j_state_lock | expand |
> 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!
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
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; }