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: <9a493354-444a-4d2e-820e-fb6eaeb31b74@126.com>
Date: Mon, 23 Sep 2024 09:35:42 +0800
From: Zhao Mengmeng <zhaomzhao@....com>
To: Jan Kara <jack@...e.cz>
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 2024/9/20 23:09, Jan Kara wrote:
> 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.
Yes, should be bh as we haven't done `*epos = nepos`. Will fix.
 
>> @@ -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
> 

Will do