Re: [PATCH] udf: don't let a stale metadata buffer overwrite expanded file data

[Jan Kara]

On Sat 26-09-26 10:35:12, Matthias Goergens wrote:
> Since commit 62333e480d12 ("udf: Fix data loss when converting inline
> inodes to out of line"), udf_expand_file_adinicb() allocates the data
> block itself with udf_map_block() and then writes the folio with
> filemap_fdatawrite().  Writeback maps the block through
> udf_get_block_wb(), which does not allocate, so the buffer is never new
> and nothing calls clean_bdev_aliases() for the block, as the generic
> code does for every block get_block() reports as new.
> 
> A recently freed block can still have a dirty buffer in the block device
> page cache.  The common case is the file entry of a deleted file:
> udf_evict_inode() updates it with sync_inode_metadata(), which for an
> inode that is not IS_SYNC only marks the buffer dirty, and then frees
> the block.  If that block becomes the data block of an expanded file,
> the next writeback of the block device writes the old file entry over
> the file's data.
> 
> On a fresh mkudffs image, creating 20 small files, deleting them and
> then creating five files with "printf hello >f; truncate -s 3000 f"
> leaves all five with a deleted file's extended file entry as their first
> block after sync and remount.  This happened in 400 of 400 runs, on
> bitmap and table images alike.  When the file is extended by writing
> instead of truncating, the write dirties the data again and the outcome
> depends on writeback order: the data was corrupted in about 7% of runs.
> 
> Clean the aliases of the new block, as block_write_begin() and
> mpage_writepages() would have done.
> 
> Fixes: 62333e480d12 ("udf: Fix data loss when converting inline inodes to out of line")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Matthias Goergens <matthias.goergens@xxxxxxxxx>

Thanks for the fix! The problematic commit was actually merged in 7.3-rc1
so there's no need to CC stable, I'll push it to Linus for rc6.

								Honza

> ---
> This is a regression in v7.3-rc1, and 62333e480d12 is also in 7.2.6 and
> 6.18.52.  On 7.2.7 and 6.18.54 the truncate case corrupts the data in
> 400 of 400 runs, as on mainline, and the write case in 10 and 7 of 400
> runs; with this patch, which applies cleanly to both, there was no
> corruption in 400 runs of either case.  Before 62333e480d12, extending
> the file by writing was safe (no corruption in 400 runs with it
> reverted): the write allocated the block through udf_get_block(), which
> marks it new, so block_write_begin() cleaned the alias.  Truncating up
> lost the data before 62333e480d12 as well, which is what that commit
> fixed.
> 
> To reproduce, run this as root in a VM; cmp reports every corrupted
> file:
> 
> #!/bin/sh
> img=/tmp/udf.img mnt=/tmp/udf.mnt
> truncate -s 1M $img
> mkudffs $img >/dev/null
> mkdir -p $mnt
> mount -t udf -o loop $img $mnt
> for i in $(seq 20); do echo small >$mnt/s$i; done
> rm $mnt/s*
> for i in $(seq 5); do
> 	printf hello >$mnt/f$i
> 	truncate -s 3000 $mnt/f$i
> done
> sync
> umount $mnt
> mount -t udf -o loop $img $mnt
> printf hello >/tmp/expect
> truncate -s 3000 /tmp/expect
> for i in $(seq 5); do cmp $mnt/f$i /tmp/expect && echo "f$i ok"; done
> umount $mnt
> 
>  fs/udf/inode.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index e45e546a739a..804554d26595 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -442,6 +442,13 @@ int udf_expand_file_adinicb(struct inode *inode)
>  	err = udf_map_block(inode, &map);
>  	if (err < 0)
>  		goto restore;
> +	/*
> +	 * The block may have held metadata that is still dirty in the block
> +	 * device page cache (e.g. the file entry of a deleted inode). Make
> +	 * sure writeback of that buffer cannot overwrite our data.
> +	 */
> +	if (map.oflags & UDF_BLK_NEW)
> +		clean_bdev_aliases(inode->i_sb->s_bdev, map.pblk, 1);
>  
>  	folio_mark_dirty(folio);
>  	folio_unlock(folio);
> 
> base-commit: 165768bb70265b5c38cf0b73fafd75be235f8b14
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR