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

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

[Chengguang Xu]

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>
---
 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

[PATCH v2 2/3] ext2: introduce new helper for xattr entry comparison

[Chengguang Xu]

Introduce new helper ext2_xattr_cmp_entry() for xattr
entry comparison.

Signed-off-by: Chengguang Xu <cgxu519@zoho.com.cn>
---
 fs/ext2/xattr.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
index 28503979696d..59356cd2a842 100644
--- a/fs/ext2/xattr.c
+++ b/fs/ext2/xattr.c
@@ -166,6 +166,21 @@ ext2_xattr_entry_valid(struct ext2_xattr_entry *entry,
 	return true;
 }
 
+static int
+ext2_xattr_cmp_entry(int name_index, size_t name_len, const char *name,
+		     struct ext2_xattr_entry *entry)
+{
+	int cmp;
+
+	cmp = name_index - entry->e_name_index;
+	if (!cmp)
+		cmp = name_len - entry->e_name_len;
+	if (!cmp)
+		cmp = memcmp(name, entry->e_name, name_len);
+
+	return cmp;
+}
+
 /*
  * ext2_xattr_get()
  *
@@ -452,13 +467,9 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
 					min_offs = offs;
 			}
 			if (not_found > 0) {
-				not_found = name_index - last->e_name_index;
-				if (!not_found)
-					not_found = name_len - last->e_name_len;
-				if (!not_found) {
-					not_found = memcmp(name, last->e_name,
-							   name_len);
-				}
+				not_found = ext2_xattr_cmp_entry(name_index,
+								 name_len,
+								 name, last);
 				if (not_found <= 0)
 					here = last;
 			}
-- 
2.20.1

[PATCH v2 3/3] ext2: optimize ext2_xattr_get()

[Chengguang Xu]

Since xattr entry names are sorted, we don't have
to continue when current entry name is greater than
target.

Signed-off-by: Chengguang Xu <cgxu519@zoho.com.cn>
---
v1->v2:
- Introduce new helper for xattr entry comparison

 fs/ext2/xattr.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
index 59356cd2a842..839e71e78673 100644
--- a/fs/ext2/xattr.c
+++ b/fs/ext2/xattr.c
@@ -199,7 +199,7 @@ ext2_xattr_get(struct inode *inode, int name_index, const char *name,
 	struct ext2_xattr_entry *entry;
 	size_t name_len, size;
 	char *end;
-	int error;
+	int error, not_found;
 	struct mb_cache *ea_block_cache = EA_BLOCK_CACHE(inode);
 
 	ea_idebug(inode, "name=%d.%s, buffer=%p, buffer_size=%ld",
@@ -238,10 +238,14 @@ ext2_xattr_get(struct inode *inode, int name_index, const char *name,
 		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)
+
+		not_found = ext2_xattr_cmp_entry(name_index, name_len, name,
+						 entry);
+		if (!not_found)
 			goto found;
+		if (not_found < 0)
+			break;
+
 		entry = EXT2_XATTR_NEXT(entry);
 	}
 	if (ext2_xattr_cache_insert(ea_block_cache, bh))
-- 
2.20.1

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

[Jan Kara]

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