linux-kernel - Re: [PATCH v3 10/12] ext4: make extent status types exclusive

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: <20240904102819.b5ouynw6k5nj3yff@quack3>
Date: Wed, 4 Sep 2024 12:28:19 +0200
From: Jan Kara <jack@...e.cz>
To: Zhang Yi <yi.zhang@...weicloud.com>
Cc: linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, tytso@....edu,
	adilger.kernel@...ger.ca, jack@...e.cz, ritesh.list@...il.com,
	yi.zhang@...wei.com, chengzhihao1@...wei.com, yukuai3@...wei.com
Subject: Re: [PATCH v3 10/12] ext4: make extent status types exclusive

On Tue 13-08-24 20:34:50, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@...wei.com>
> 
> Since we don't add delayed flag in unwritten extents, all of the four
> extent status types EXTENT_STATUS_WRITTEN, EXTENT_STATUS_UNWRITTEN,
> EXTENT_STATUS_DELAYED and EXTENT_STATUS_HOLE are exclusive now, add
> assertion when storing pblock before inserting extent into status tree
> and add comment to the status definition.
> 
> Suggested-by: Jan Kara <jack@...e.cz>
> Signed-off-by: Zhang Yi <yi.zhang@...wei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@...e.cz>

								Honza

> ---
>  fs/ext4/extents_status.h | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/extents_status.h b/fs/ext4/extents_status.h
> index 3ca40f018994..7d7af642f7b2 100644
> --- a/fs/ext4/extents_status.h
> +++ b/fs/ext4/extents_status.h
> @@ -42,6 +42,10 @@ enum {
>  #define ES_SHIFT (sizeof(ext4_fsblk_t)*8 - ES_FLAGS)
>  #define ES_MASK (~((ext4_fsblk_t)0) << ES_SHIFT)
>  
> +/*
> + * Besides EXTENT_STATUS_REFERENCED, all these extent type masks
> + * are exclusive, only one type can be set at a time.
> + */
>  #define EXTENT_STATUS_WRITTEN	(1 << ES_WRITTEN_B)
>  #define EXTENT_STATUS_UNWRITTEN (1 << ES_UNWRITTEN_B)
>  #define EXTENT_STATUS_DELAYED	(1 << ES_DELAYED_B)
> @@ -51,7 +55,9 @@ enum {
>  #define ES_TYPE_MASK	((ext4_fsblk_t)(EXTENT_STATUS_WRITTEN | \
>  			  EXTENT_STATUS_UNWRITTEN | \
>  			  EXTENT_STATUS_DELAYED | \
> -			  EXTENT_STATUS_HOLE) << ES_SHIFT)
> +			  EXTENT_STATUS_HOLE))
> +
> +#define ES_TYPE_VALID(type)	((type) && !((type) & ((type) - 1)))
>  
>  struct ext4_sb_info;
>  struct ext4_extent;
> @@ -156,7 +162,7 @@ static inline unsigned int ext4_es_status(struct extent_status *es)
>  
>  static inline unsigned int ext4_es_type(struct extent_status *es)
>  {
> -	return (es->es_pblk & ES_TYPE_MASK) >> ES_SHIFT;
> +	return (es->es_pblk >> ES_SHIFT) & ES_TYPE_MASK;
>  }
>  
>  static inline int ext4_es_is_written(struct extent_status *es)
> @@ -228,6 +234,8 @@ static inline void ext4_es_store_pblock_status(struct extent_status *es,
>  					       ext4_fsblk_t pb,
>  					       unsigned int status)
>  {
> +	WARN_ON_ONCE(!ES_TYPE_VALID(status & ES_TYPE_MASK));
> +
>  	es->es_pblk = (((ext4_fsblk_t)status << ES_SHIFT) & ES_MASK) |
>  		      (pb & ~ES_MASK);
>  }
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR