Re: [PATCH v2 1/3] ext2: merge xattr next entry check to ext2_xattr_entry_valid()

[Jan Kara <jack@suse.cz>]

On Tue 28-05-19 10:59:45, Chengguang Xu wrote:
> We have introduced ext2_xattr_entry_valid() for xattr
> entry sanity check, so it's better to do relevant things
> in one place.
> 
> Signed-off-by: Chengguang Xu <cgxu519@zoho.com.cn>

Thanks! I've applied all three patches to my tree.

								Honza

> ---
>  fs/ext2/xattr.c | 36 ++++++++++++++++--------------------
>  1 file changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index d21dbf297b74..28503979696d 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -145,10 +145,16 @@ ext2_xattr_header_valid(struct ext2_xattr_header *header)
>  }
>  
>  static bool
> -ext2_xattr_entry_valid(struct ext2_xattr_entry *entry, size_t end_offs)
> +ext2_xattr_entry_valid(struct ext2_xattr_entry *entry,
> +		       char *end, size_t end_offs)
>  {
> +	struct ext2_xattr_entry *next;
>  	size_t size;
>  
> +	next = EXT2_XATTR_NEXT(entry);
> +	if ((char *)next >= end)
> +		return false;
> +
>  	if (entry->e_value_block != 0)
>  		return false;
>  
> @@ -214,17 +220,14 @@ ext2_xattr_get(struct inode *inode, int name_index, const char *name,
>  	/* find named attribute */
>  	entry = FIRST_ENTRY(bh);
>  	while (!IS_LAST_ENTRY(entry)) {
> -		struct ext2_xattr_entry *next =
> -			EXT2_XATTR_NEXT(entry);
> -		if ((char *)next >= end)
> -			goto bad_block;
> -		if (!ext2_xattr_entry_valid(entry, inode->i_sb->s_blocksize))
> +		if (!ext2_xattr_entry_valid(entry, end,
> +		    inode->i_sb->s_blocksize))
>  			goto bad_block;
>  		if (name_index == entry->e_name_index &&
>  		    name_len == entry->e_name_len &&
>  		    memcmp(name, entry->e_name, name_len) == 0)
>  			goto found;
> -		entry = next;
> +		entry = EXT2_XATTR_NEXT(entry);
>  	}
>  	if (ext2_xattr_cache_insert(ea_block_cache, bh))
>  		ea_idebug(inode, "cache insert failed");
> @@ -299,13 +302,10 @@ ext2_xattr_list(struct dentry *dentry, char *buffer, size_t buffer_size)
>  	/* check the on-disk data structure */
>  	entry = FIRST_ENTRY(bh);
>  	while (!IS_LAST_ENTRY(entry)) {
> -		struct ext2_xattr_entry *next = EXT2_XATTR_NEXT(entry);
> -
> -		if ((char *)next >= end)
> -			goto bad_block;
> -		if (!ext2_xattr_entry_valid(entry, inode->i_sb->s_blocksize))
> +		if (!ext2_xattr_entry_valid(entry, end,
> +		    inode->i_sb->s_blocksize))
>  			goto bad_block;
> -		entry = next;
> +		entry = EXT2_XATTR_NEXT(entry);
>  	}
>  	if (ext2_xattr_cache_insert(ea_block_cache, bh))
>  		ea_idebug(inode, "cache insert failed");
> @@ -390,7 +390,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
>  	struct super_block *sb = inode->i_sb;
>  	struct buffer_head *bh = NULL;
>  	struct ext2_xattr_header *header = NULL;
> -	struct ext2_xattr_entry *here, *last;
> +	struct ext2_xattr_entry *here = NULL, *last = NULL;
>  	size_t name_len, free, min_offs = sb->s_blocksize;
>  	int not_found = 1, error;
>  	char *end;
> @@ -444,10 +444,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
>  		 */
>  		last = FIRST_ENTRY(bh);
>  		while (!IS_LAST_ENTRY(last)) {
> -			struct ext2_xattr_entry *next = EXT2_XATTR_NEXT(last);
> -			if ((char *)next >= end)
> -				goto bad_block;
> -			if (!ext2_xattr_entry_valid(last, sb->s_blocksize))
> +			if (!ext2_xattr_entry_valid(last, end, sb->s_blocksize))
>  				goto bad_block;
>  			if (last->e_value_size) {
>  				size_t offs = le16_to_cpu(last->e_value_offs);
> @@ -465,7 +462,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
>  				if (not_found <= 0)
>  					here = last;
>  			}
> -			last = next;
> +			last = EXT2_XATTR_NEXT(last);
>  		}
>  		if (not_found > 0)
>  			here = last;
> @@ -476,7 +473,6 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
>  		/* We will use a new extended attribute block. */
>  		free = sb->s_blocksize -
>  			sizeof(struct ext2_xattr_header) - sizeof(__u32);
> -		here = last = NULL;  /* avoid gcc uninitialized warning. */
>  	}
>  
>  	if (not_found) {
> -- 
> 2.20.1
> 
> 
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR