linux-kernel - Re: [PATCH v2 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: <20240927115509.a6ie4b75c65gjvfu@quack3>
Date: Fri, 27 Sep 2024 13:55:09 +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 v2 1/3] udf: refactor udf_current_aext() to handle error

On Thu 26-09-24 20:07:51, 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. Add two macroes to
> test return value.
> 
> Link: https://lore.kernel.org/all/20240912111235.6nr3wuqvktecy3vh@quack3/
> Signed-off-by: Zhao Mengmeng <zhaomengmeng@...inos.cn>
> Suggested-by: Jan Kara <jack@...e.cz>
...
> @@ -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)) {

This looks wrong. If udf_current_aext() succeeds, you'll immediately abort
the loop now. I'd rather code this as:

	while (1) {
		err = udf_current_aext(inode, epos, eloc, elen, &etype, inc);
		if (err || etype != (EXT_NEXT_EXTENT_ALLOCDESCS >> 30))
			break;
		...
	}

> diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
> index 88692512a466..a902652450dd 100644
> --- a/fs/udf/udfdecl.h
> +++ b/fs/udf/udfdecl.h
> @@ -43,6 +43,9 @@ extern __printf(3, 4) void _udf_warn(struct super_block *sb,
>  #define UDF_NAME_LEN		254
>  #define UDF_NAME_LEN_CS0	255
>  
> +#define UDF_EXT_EOF(err)        ((err) == -ENODATA)
> +#define UDF_EXT_ERR(err)        (((err) < 0) && (!UDF_EXT_EOF(err)))
> +

So I agree the explicit ENODATA checks are a bit ugly but these macros
aren't really much better. How about the following calling convention:

On error, ret < 0, on EOF ret == 0, on success ret == 1. This is a similar
convention as e.g. for read(2) so it is well understood and easy test for
various combinations.

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