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

[Jan Kara <jack@suse.cz>]

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