linux-kernel - Re: [PATCH 1/3] udf: refactor udf_current

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240920150952.bqzxuhnvgh6zx5rg@quack3>
Date: Fri, 20 Sep 2024 17:09:52 +0200
From: Jan Kara <jack@...e.cz>
To: Zhao Mengmeng <zhaomzhao@....com>
Cc: jack@...e.com, zhaomengmeng@...inos.cn, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] udf: refactor udf_current_aext() to handle error

On Wed 18-09-24 17:36:32, Zhao Mengmeng wrote:
> From: Zhao Mengmeng <zhaomengmeng@...inos.cn>
> 
> As Jan suggested in links below, refactor udf_current_aext() to
> differentiate between error and "hit EOF", it now takes pointer to etype
> to store the extent type, return 0 when get etype success; return -ENODATA
> when hit EOF; return -EINVAL when i_alloc_type invalid.
> 
> Link: https://lore.kernel.org/all/20240912111235.6nr3wuqvktecy3vh@quack3/
> 
> Signed-off-by: Zhao Mengmeng <zhaomengmeng@...inos.cn>

Two comments below.

> @@ -1999,10 +2000,12 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
>  	if (epos->offset + adsize > sb->s_blocksize) {
>  		struct kernel_lb_addr cp_loc;
>  		uint32_t cp_len;
> -		int cp_type;
> +		int8_t cp_type;
>  
>  		epos->offset -= adsize;
> -		cp_type = udf_current_aext(inode, epos, &cp_loc, &cp_len, 0);
> +		err = udf_current_aext(inode, epos, &cp_loc, &cp_len, &cp_type, 0);
> +		if (err < 0)
> +			goto err_out;
>  		cp_len |= ((uint32_t)cp_type) << 30;
>  
>  		__udf_add_aext(inode, &nepos, &cp_loc, cp_len, 1);
> @@ -2017,6 +2020,9 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
>  	*epos = nepos;
>  
>  	return 0;
> +err_out:
> +	brelse(epos->bh);
> +	return err;
>  }

So here I don't think we want to release epos->bh. Rather we need to
release 'bh' itself which we have got because we did't replace epos->bh yet
with it.

> @@ -2167,9 +2173,12 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
>  {
>  	int8_t etype;
>  	unsigned int indirections = 0;
> +	int err = 0;
> +
> +	while ((err = udf_current_aext(inode, epos, eloc, elen, &etype, inc))) {
> +		if (err || etype != (EXT_NEXT_EXTENT_ALLOCDESCS >> 30))
> +			break;
>  
> -	while ((etype = udf_current_aext(inode, epos, eloc, elen, inc)) ==
> -	       (EXT_NEXT_EXTENT_ALLOCDESCS >> 30)) {
>  		udf_pblk_t block;
>  
>  		if (++indirections > UDF_MAX_INDIR_EXTS) {
> @@ -2190,14 +2199,14 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
>  		}
>  	}
>  
> -	return etype;
> +	return err;
>  }

This doesn't look right. Probably it gets fixed up in the following patches
but here should be something like: !err ? etype : -1
to keep udf_next_aext() compatible with its users.

Otherwise the patch looks good.

								Honza

-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR