Re: [PATCH v2 6.1] udf: Fold udf_getblk() into udf_bread()

[Jan Kara <jack@suse.cz>]

On Wed 04-12-24 09:32:26, Jakub Acs wrote:
> commit 32f123a3f34283f9c6446de87861696f0502b02e upstream.
> 
> udf_getblk() has a single call site. Fold it there.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> 
> [acsjakub: backport-adjusting changes]
> udf_getblk() has changed between 6.1 and the backported commit namely
> in commit 541e047b14c8 ("udf: Use udf_map_block() in udf_getblk()")
> 
> Backport using the form of udf_getblk present in 6.1., that means use
> udf_get_block() instead of udf_map_block() and use dummy in buffer_new()
> and buffer_mapped().
> 
> Closes: https://syzkaller.appspot.com/bug?extid=a38e34ca637c224f4a79
> Signed-off-by: Jakub Acs <acsjakub@amazon.de>
> ---
> While doing the backport I have noticed potential side effect of the
> upstream commit (present in the mainline):
> 
> If we take the if-branch of 'if (map.oflags & UDF_BLK_NEW)', we will
> return the bh without the 'if (bh_read(bh, 0) >= 0)' check. Prior to
> the folding, the check wouldn't be skipped, was this intentional by the
> upstream commit?

Absolutely. bh_read() is pointless if you fill in the buffer contents
yourself (as we do in the 'if (map.oflags & UDF_BLK_NEW)' branch).

								Honza

> ---
>  fs/udf/inode.c | 46 +++++++++++++++++++++-------------------------
>  1 file changed, 21 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index d7d6ccd0af06..626450101412 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -369,29 +369,6 @@ static int udf_get_block(struct inode *inode, sector_t block,
>  	return err;
>  }
>  
> -static struct buffer_head *udf_getblk(struct inode *inode, udf_pblk_t block,
> -				      int create, int *err)
> -{
> -	struct buffer_head *bh;
> -	struct buffer_head dummy;
> -
> -	dummy.b_state = 0;
> -	dummy.b_blocknr = -1000;
> -	*err = udf_get_block(inode, block, &dummy, create);
> -	if (!*err && buffer_mapped(&dummy)) {
> -		bh = sb_getblk(inode->i_sb, dummy.b_blocknr);
> -		if (buffer_new(&dummy)) {
> -			lock_buffer(bh);
> -			memset(bh->b_data, 0x00, inode->i_sb->s_blocksize);
> -			set_buffer_uptodate(bh);
> -			unlock_buffer(bh);
> -			mark_buffer_dirty_inode(bh, inode);
> -		}
> -		return bh;
> -	}
> -
> -	return NULL;
> -}
>  
>  /* Extend the file with new blocks totaling 'new_block_bytes',
>   * return the number of extents added
> @@ -1108,10 +1085,29 @@ struct buffer_head *udf_bread(struct inode *inode, udf_pblk_t block,
>  			      int create, int *err)
>  {
>  	struct buffer_head *bh = NULL;
> +	struct buffer_head dummy;
>  
> -	bh = udf_getblk(inode, block, create, err);
> -	if (!bh)
> +	dummy.b_state = 0;
> +	dummy.b_blocknr = -1000;
> +
> +	*err = udf_get_block(inode, block, &dummy, create);
> +	if (*err || !buffer_mapped(&dummy))
> +		return NULL
> +
> +	bh = sb_getblk(inode->i_sb, dummy.b_blocknr);
> +	if (!bh) {
> +		*err = -ENOMEM;
>  		return NULL;
> +	}
> +
> +	if (buffer_new(&dummy)) {
> +		lock_buffer(bh);
> +		memset(bh->b_data, 0x00, inode->i_sb->s_blocksize);
> +		set_buffer_uptodate(bh);
> +		unlock_buffer(bh);
> +		mark_buffer_dirty_inode(bh, inode);
> +		return bh;
> +	}
>  
>  	if (bh_read(bh, 0) >= 0)
>  		return bh;
> 
> base-commit: e4d90d63d385228b1e0bcf31cc15539bbbc28f7f
> -- 
> 2.40.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR