Re: [PATCH] ext4: Fix superblock checksum calculation race

Jan Kara
2020-09-14T08:54:30Z

On Fri 11-09-20 15:16:03, Constantine Sapuntzakis wrote:
> The race condition could cause the persisted superblock checksum
> to not match the contents of the superblock, causing the
> superblock to be considered corrupt.
>
> An example of the race follows. A first thread is interrupted in the
> middle of a checksum calculation. Then, another thread changes the
> superblock, calculates a new checksum, and sets it. Then, the first
> thread resumes and sets the checksum based on the older superblock.
>
> To fix, serialize the superblock checksum calculation using the buffer
> header lock. While a spinlock is sufficient, the buffer header is
> already there and there is precedent for locking it (e.g. in
> ext4_commit_super).
>
> Tested the patch by booting up a kernel with the patch, creating
> a filesystem and some files (including some orphans), and then
> unmounting and remounting the file system.
>
> Suggested-by: Jan Kara <jack@suse.cz>

Thanks for the patch! Please add your Signed-off-by line to the patch to
certify that you've written the patch and agree with it being included in
the kernel (see "Developer's Certificate of Origin 1.1" in
Documentation/process/submitting-patches.rst for more details). Without it
the patch cannot be included. Otherwise it looks good so feel free to add:

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

Thanks
Honza

> ---
> fs/ext4/super.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index ea425b49b345..3f7fdce5ab05 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -201,7 +201,18 @@ void ext4_superblock_csum_set(struct super_block *sb)
> if (!ext4_has_metadata_csum(sb))
> return;
>
> + /*
> + * Locking the superblock prevents the scenario
> + * where:
> + * 1) a first thread pauses during checksum calculation.
> + * 2) a second thread updates the superblock, recalculates
> + * the checksum, and updates s_checksum
> + * 3) the first thread resumes and finishes its checksum calculation
> + * and updates s_checksum with a potentially stale or torn value.
> + */
> + lock_buffer(EXT4_SB(sb)->s_sbh);
> es->s_checksum = ext4_superblock_csum(sb, es);
> + unlock_buffer(EXT4_SB(sb)->s_sbh);
> }
>
> ext4_fsblk_t ext4_block_bitmap(struct super_block *sb,
> --
> 2.17.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR