Re: [PATCH] udf: fix WARN_ON_ONCE in udf_map_block on concurrent expand failure

[Jan Kara]

On Sat 19-09-26 15:44:31, golasch18@xxxxxx wrote:
> From: Lukas Blesser <golasch18@xxxxxx>
> 
> udf_direct_IO() falls back to buffered I/O for in-ICB files, but it
> checks i_alloc_type without holding i_data_sem. If a concurrent write
> fails to expand the file, udf_expand_file_adinicb() restores inline
> allocation, so udf_map_block() can observe ICBTAG_FLAG_AD_IN_ICB after
> the caller checked and hit the WARN_ON_ONCE() for a transient state.
> 
> Move the allocation-type check under the down_read() udf_map_block()
> already takes on the lookup path and return -EFSCORRUPTED gracefully
> instead of warning.
> 
> Reported-by: syzbot+de2fef01040748da6822@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=de2fef01040748da6822
> Signed-off-by: Lukas Blesser <golasch18@xxxxxx>

There are two problems with this:

1) This removes the assert from the path allocating blocks.

2) This doesn't completely close the race window - the ICBTAG_FLAG_AD_IN_ICB
check in udf_direct_IO() should have failed and falled back to buffered IO.
Fixing that for reads is doable but I don't think it's worth it.

For now, I'd be inclined to just remove direct IO support from UDF and
always fallback to buffered IO. UDF isn't really used for any
performance-sensitive applications where direct IO matters. Once UDF is
converted to iomap, we can add the direct IO support back.

								Honza

> ---
>  fs/udf/inode.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index e45e546a7..cea8d5c4d 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -352,9 +352,6 @@ static int udf_map_block(struct inode *inode, struct udf_map_rq *map)
>  	int ret;
>  	struct udf_inode_info *iinfo = UDF_I(inode);
>  
> -	if (WARN_ON_ONCE(iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB))
> -		return -EFSCORRUPTED;
> -
>  	map->oflags = 0;
>  	if (!(map->iflags & UDF_MAP_CREATE)) {
>  		struct kernel_lb_addr eloc;
> @@ -364,6 +361,18 @@ static int udf_map_block(struct inode *inode, struct udf_map_rq *map)
>  		int8_t etype;
>  
>  		down_read(&iinfo->i_data_sem);
> +		/*
> +		 * The allocation type may have changed back to in-ICB
> +		 * after a caller checked it without holding i_data_sem:
> +		 * e.g. a concurrent expand failure restores inline
> +		 * allocation in udf_expand_file_adinicb(). Recheck here
> +		 * under i_data_sem and fail gracefully instead of
> +		 * warning about a transient state.
> +		 */
> +		if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB) {
> +			ret = -EFSCORRUPTED;
> +			goto out_read;
> +		}
>  		ret = inode_bmap(inode, map->lblk, &epos, &eloc, &elen, &offset,
>  				 &etype);
>  		if (ret < 0)
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR