Re: [BUG] invalid superblock checksum possibly due to race

[Jan Kara <jack@suse.cz>]

Hello!

On Wed 24-06-20 16:56:18, Costa Sapuntzakis wrote:
> Our workload: taking snapshots repeatedly of an active ext4 filesystem
> (vdbench fwiw). e2fsck discovered a snapshot that had a corrupted
> superblock after journal replay. Diffing the corrupted superblock to
> the superblock before journal replay revealed that only s_last_orphan
> and the checksum had changed.
> 
> The following race could explain it:
> 
> Thread 1 (T1): ext4_orphan_del -> update s_last_orphan to value A ->
> ext4_handle_dirty_super -> ext4_superblock_csum_set -- PAUSE right
> before setting es->s_checksum
> 
> T2: ext4_orphan_del -> update s_last_orphan to value B ->
> ext4_handle_dirty_super -> ext4_superblock_csum_set runs to completion
> 
> T1: Resume and assign es->s_checksum
> 
> Is there higher level synchronization going on that makes this race benign?

Thanks for report and the analysis. What you describe indeed seems
possible.

> If not, a spinlock around the calculation and assignment should fix it.

Yes, probably ext4_superblock_csum_set() should use

lock_buffer(EXT4_SB(sb)->s_sbh)

to synchronize updating of superblock checksum. Will you send a patch?

> The spinlock still has the race where s_last_orphan is being updated
> while the checksum is calculated. But the last thread to set
> s_last_orphan will also eventually try to recalculate the checksum and
> set it right (though it's possible some other thread will do it for
> it). And I'm guessing/hoping jbd2 won't flush the superblock to the
> journal and close a transaction until the references from
> journal_get_write_access drain. The checksum is recalculated before
> the get_write_access reference is dropped.

Yes, jbd2 layer will make sure that inconsistent block contents will not
make it to disk in this case.

								Honza

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