[PATCH 0/3 v2] ext2: Cleanup ext2_xattr_set()

[PATCH 0/3 v2] ext2: Cleanup ext2_xattr_set()

[Jan Kara]

Hello,

this series contains updated patch from Chengguang and two additional cleanups
to xattr code in ext2_xattr_set() to remove some duplicated code and useless
checks.

Changes since v1:
* fixed patch 2 to maintain sorting of xattrs in ext2_xattr_set
* made loops in ext2_xattr_get() and ext2_xattr_list() also check all seen
  xattr entries

								Honza

[PATCH 1/3] ext2: introduce helper for xattr entry validation

[Jan Kara]

From: Chengguang Xu <cgxu519@zoho.com.cn>

Introduce helper function ext2_xattr_entry_valid()
for xattr entry validation and clean up the entry
check related code.

Reviewed-by: Andreas Dilger <adilger@dilger.ca>
Signed-off-by: Chengguang Xu <cgxu519@zoho.com.cn>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/xattr.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
index db27260d6a5b..fb2e008d4406 100644
--- a/fs/ext2/xattr.c
+++ b/fs/ext2/xattr.c
@@ -144,6 +144,22 @@ ext2_xattr_header_valid(struct ext2_xattr_header *header)
 	return true;
 }
 
+static bool
+ext2_xattr_entry_valid(struct ext2_xattr_entry *entry, size_t end_offs)
+{
+	size_t size;
+
+	if (entry->e_value_block != 0)
+		return false;
+
+	size = le32_to_cpu(entry->e_value_size);
+	if (size > end_offs ||
+	    le16_to_cpu(entry->e_value_offs) + size > end_offs)
+		return false;
+
+	return true;
+}
+
 /*
  * ext2_xattr_get()
  *
@@ -213,14 +229,10 @@ ext2_xattr_get(struct inode *inode, int name_index, const char *name,
 	error = -ENODATA;
 	goto cleanup;
 found:
-	/* check the buffer size */
-	if (entry->e_value_block != 0)
-		goto bad_block;
-	size = le32_to_cpu(entry->e_value_size);
-	if (size > inode->i_sb->s_blocksize ||
-	    le16_to_cpu(entry->e_value_offs) + size > inode->i_sb->s_blocksize)
+	if (!ext2_xattr_entry_valid(entry, inode->i_sb->s_blocksize))
 		goto bad_block;
 
+	size = le32_to_cpu(entry->e_value_size);
 	if (ext2_xattr_cache_insert(ea_block_cache, bh))
 		ea_idebug(inode, "cache insert failed");
 	if (buffer) {
@@ -481,12 +493,10 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
 		if (flags & XATTR_CREATE)
 			goto cleanup;
 		if (!here->e_value_block && here->e_value_size) {
-			size_t size = le32_to_cpu(here->e_value_size);
-
-			if (le16_to_cpu(here->e_value_offs) + size > 
-			    sb->s_blocksize || size > sb->s_blocksize)
+			if (!ext2_xattr_entry_valid(here, sb->s_blocksize))
 				goto bad_block;
-			free += EXT2_XATTR_SIZE(size);
+			free += EXT2_XATTR_SIZE(
+					le32_to_cpu(here->e_value_size));
 		}
 		free += EXT2_XATTR_LEN(name_len);
 	}
-- 
2.16.4

[PATCH 2/3] ext2: Merge loops in ext2_xattr_set()

[Jan Kara]

There are two very similar loops when searching xattr to set. Just merge
them.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/xattr.c | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
index fb2e008d4406..f9fda6d16d78 100644
--- a/fs/ext2/xattr.c
+++ b/fs/ext2/xattr.c
@@ -436,28 +436,12 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
 			error = -EIO;
 			goto cleanup;
 		}
-		/* Find the named attribute. */
-		here = FIRST_ENTRY(bh);
-		while (!IS_LAST_ENTRY(here)) {
-			struct ext2_xattr_entry *next = EXT2_XATTR_NEXT(here);
-			if ((char *)next >= end)
-				goto bad_block;
-			if (!here->e_value_block && here->e_value_size) {
-				size_t offs = le16_to_cpu(here->e_value_offs);
-				if (offs < min_offs)
-					min_offs = offs;
-			}
-			not_found = name_index - here->e_name_index;
-			if (!not_found)
-				not_found = name_len - here->e_name_len;
-			if (!not_found)
-				not_found = memcmp(name, here->e_name,name_len);
-			if (not_found <= 0)
-				break;
-			here = next;
-		}
-		last = here;
-		/* We still need to compute min_offs and last. */
+		/*
+		 * Find the named attribute. If not found, 'here' will point
+		 * to entry where the new attribute should be inserted to
+		 * maintain sorting.
+		 */
+		last = FIRST_ENTRY(bh);
 		while (!IS_LAST_ENTRY(last)) {
 			struct ext2_xattr_entry *next = EXT2_XATTR_NEXT(last);
 			if ((char *)next >= end)
@@ -467,8 +451,21 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
 				if (offs < min_offs)
 					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);
+				}
+				if (not_found <= 0)
+					here = last;
+			}
 			last = next;
 		}
+		if (not_found > 0)
+			here = last;
 
 		/* Check whether we have enough space left. */
 		free = min_offs - ((char*)last - (char*)header) - sizeof(__u32);
-- 
2.16.4

[PATCH 3/3] ext2: Strengthen xattr block checks

[Jan Kara]

Check every entry in xattr block for validity in ext2_xattr_set() to
detect on disk corruption early. Also since e_value_block field in xattr
entry is never != 0 in a valid filesystem, just remove checks for it
once we have established entries are valid.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/xattr.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
index f9fda6d16d78..d21dbf297b74 100644
--- a/fs/ext2/xattr.c
+++ b/fs/ext2/xattr.c
@@ -218,6 +218,8 @@ ext2_xattr_get(struct inode *inode, int name_index, const char *name,
 			EXT2_XATTR_NEXT(entry);
 		if ((char *)next >= end)
 			goto bad_block;
+		if (!ext2_xattr_entry_valid(entry, 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)
@@ -229,9 +231,6 @@ ext2_xattr_get(struct inode *inode, int name_index, const char *name,
 	error = -ENODATA;
 	goto cleanup;
 found:
-	if (!ext2_xattr_entry_valid(entry, inode->i_sb->s_blocksize))
-		goto bad_block;
-
 	size = le32_to_cpu(entry->e_value_size);
 	if (ext2_xattr_cache_insert(ea_block_cache, bh))
 		ea_idebug(inode, "cache insert failed");
@@ -304,6 +303,8 @@ ext2_xattr_list(struct dentry *dentry, char *buffer, size_t buffer_size)
 
 		if ((char *)next >= end)
 			goto bad_block;
+		if (!ext2_xattr_entry_valid(entry, inode->i_sb->s_blocksize))
+			goto bad_block;
 		entry = next;
 	}
 	if (ext2_xattr_cache_insert(ea_block_cache, bh))
@@ -446,7 +447,9 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
 			struct ext2_xattr_entry *next = EXT2_XATTR_NEXT(last);
 			if ((char *)next >= end)
 				goto bad_block;
-			if (!last->e_value_block && last->e_value_size) {
+			if (!ext2_xattr_entry_valid(last, sb->s_blocksize))
+				goto bad_block;
+			if (last->e_value_size) {
 				size_t offs = le16_to_cpu(last->e_value_offs);
 				if (offs < min_offs)
 					min_offs = offs;
@@ -489,12 +492,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
 		error = -EEXIST;
 		if (flags & XATTR_CREATE)
 			goto cleanup;
-		if (!here->e_value_block && here->e_value_size) {
-			if (!ext2_xattr_entry_valid(here, sb->s_blocksize))
-				goto bad_block;
-			free += EXT2_XATTR_SIZE(
-					le32_to_cpu(here->e_value_size));
-		}
+		free += EXT2_XATTR_SIZE(le32_to_cpu(here->e_value_size));
 		free += EXT2_XATTR_LEN(name_len);
 	}
 	error = -ENOSPC;
@@ -559,7 +557,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
 		here->e_name_len = name_len;
 		memcpy(here->e_name, name, name_len);
 	} else {
-		if (!here->e_value_block && here->e_value_size) {
+		if (here->e_value_size) {
 			char *first_val = (char *)header + min_offs;
 			size_t offs = le16_to_cpu(here->e_value_offs);
 			char *val = (char *)header + offs;
@@ -586,7 +584,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
 			last = ENTRY(header+1);
 			while (!IS_LAST_ENTRY(last)) {
 				size_t o = le16_to_cpu(last->e_value_offs);
-				if (!last->e_value_block && o < offs)
+				if (o < offs)
 					last->e_value_offs =
 						cpu_to_le16(o + size);
 				last = EXT2_XATTR_NEXT(last);
-- 
2.16.4

Re: [PATCH 2/3] ext2: Merge loops in ext2_xattr_set()

[cgxu519]

On Thu, 2019-05-16 at 12:03 +0200, Jan Kara wrote:
> There are two very similar loops when searching xattr to set. Just merge
> them.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Reviewed-by: Chengguang Xu <cgxu519@zoho.com.cn>


> ---
>  fs/ext2/xattr.c | 41 +++++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index fb2e008d4406..f9fda6d16d78 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -436,28 +436,12 @@ ext2_xattr_set(struct inode *inode, int name_index,
> const char *name,
>  			error = -EIO;
>  			goto cleanup;
>  		}
> -		/* Find the named attribute. */
> -		here = FIRST_ENTRY(bh);
> -		while (!IS_LAST_ENTRY(here)) {
> -			struct ext2_xattr_entry *next = EXT2_XATTR_NEXT(here);
> -			if ((char *)next >= end)
> -				goto bad_block;
> -			if (!here->e_value_block && here->e_value_size) {
> -				size_t offs = le16_to_cpu(here->e_value_offs);
> -				if (offs < min_offs)
> -					min_offs = offs;
> -			}
> -			not_found = name_index - here->e_name_index;
> -			if (!not_found)
> -				not_found = name_len - here->e_name_len;
> -			if (!not_found)
> -				not_found = memcmp(name, here->e_name,name_len);
> -			if (not_found <= 0)
> -				break;
> -			here = next;
> -		}
> -		last = here;
> -		/* We still need to compute min_offs and last. */
> +		/*
> +		 * Find the named attribute. If not found, 'here' will point
> +		 * to entry where the new attribute should be inserted to
> +		 * maintain sorting.
> +		 */
> +		last = FIRST_ENTRY(bh);
>  		while (!IS_LAST_ENTRY(last)) {
>  			struct ext2_xattr_entry *next = EXT2_XATTR_NEXT(last);
>  			if ((char *)next >= end)
> @@ -467,8 +451,21 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> char *name,
>  				if (offs < min_offs)
>  					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);
> +				}
> +				if (not_found <= 0)
> +					here = last;
> +			}
>  			last = next;
>  		}
> +		if (not_found > 0)
> +			here = last;
>  
>  		/* Check whether we have enough space left. */
>  		free = min_offs - ((char*)last - (char*)header) - sizeof(__u32);

Re: [PATCH 3/3] ext2: Strengthen xattr block checks

[cgxu519]

On Thu, 2019-05-16 at 12:03 +0200, Jan Kara wrote:
> Check every entry in xattr block for validity in ext2_xattr_set() to
> detect on disk corruption early. Also since e_value_block field in xattr
> entry is never != 0 in a valid filesystem, just remove checks for it
> once we have established entries are valid.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Reviewed-by: Chengguang Xu <cgxu519@zoho.com.cn>

> ---
>  fs/ext2/xattr.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index f9fda6d16d78..d21dbf297b74 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -218,6 +218,8 @@ ext2_xattr_get(struct inode *inode, int name_index, const
> char *name,
>  			EXT2_XATTR_NEXT(entry);
>  		if ((char *)next >= end)
>  			goto bad_block;
> +		if (!ext2_xattr_entry_valid(entry, 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)
> @@ -229,9 +231,6 @@ ext2_xattr_get(struct inode *inode, int name_index, const
> char *name,
>  	error = -ENODATA;
>  	goto cleanup;
>  found:
> -	if (!ext2_xattr_entry_valid(entry, inode->i_sb->s_blocksize))
> -		goto bad_block;
> -
>  	size = le32_to_cpu(entry->e_value_size);
>  	if (ext2_xattr_cache_insert(ea_block_cache, bh))
>  		ea_idebug(inode, "cache insert failed");
> @@ -304,6 +303,8 @@ ext2_xattr_list(struct dentry *dentry, char *buffer,
> size_t buffer_size)
>  
>  		if ((char *)next >= end)
>  			goto bad_block;
> +		if (!ext2_xattr_entry_valid(entry, inode->i_sb->s_blocksize))
> +			goto bad_block;
>  		entry = next;
>  	}
>  	if (ext2_xattr_cache_insert(ea_block_cache, bh))
> @@ -446,7 +447,9 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> char *name,
>  			struct ext2_xattr_entry *next = EXT2_XATTR_NEXT(last);
>  			if ((char *)next >= end)
>  				goto bad_block;
> -			if (!last->e_value_block && last->e_value_size) {
> +			if (!ext2_xattr_entry_valid(last, sb->s_blocksize))
> +				goto bad_block;
> +			if (last->e_value_size) {
>  				size_t offs = le16_to_cpu(last->e_value_offs);
>  				if (offs < min_offs)
>  					min_offs = offs;
> @@ -489,12 +492,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> char *name,
>  		error = -EEXIST;
>  		if (flags & XATTR_CREATE)
>  			goto cleanup;
> -		if (!here->e_value_block && here->e_value_size) {
> -			if (!ext2_xattr_entry_valid(here, sb->s_blocksize))
> -				goto bad_block;
> -			free += EXT2_XATTR_SIZE(
> -					le32_to_cpu(here->e_value_size));
> -		}
> +		free += EXT2_XATTR_SIZE(le32_to_cpu(here->e_value_size));
>  		free += EXT2_XATTR_LEN(name_len);
>  	}
>  	error = -ENOSPC;
> @@ -559,7 +557,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> char *name,
>  		here->e_name_len = name_len;
>  		memcpy(here->e_name, name, name_len);
>  	} else {
> -		if (!here->e_value_block && here->e_value_size) {
> +		if (here->e_value_size) {
>  			char *first_val = (char *)header + min_offs;
>  			size_t offs = le16_to_cpu(here->e_value_offs);
>  			char *val = (char *)header + offs;
> @@ -586,7 +584,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> char *name,
>  			last = ENTRY(header+1);
>  			while (!IS_LAST_ENTRY(last)) {
>  				size_t o = le16_to_cpu(last->e_value_offs);
> -				if (!last->e_value_block && o < offs)
> +				if (o < offs)
>  					last->e_value_offs =
>  						cpu_to_le16(o + size);
>  				last = EXT2_XATTR_NEXT(last);

Re: [PATCH 3/3] ext2: Strengthen xattr block checks

[Jan Kara]

On Thu 16-05-19 09:16:06, cgxu519@zoho.com.cn wrote:
> On Wed, 2019-05-15 at 16:01 +0200, Jan Kara wrote:
> > Check every entry in xattr block for validity in ext2_xattr_set() to
> > detect on disk corruption early. Also since e_value_block field in xattr
> > entry is never != 0 in a valid filesystem, just remove checks for it
> > once we have established entries are valid.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> Could we do the entry check in the loop of get/list operation too?

Yes, makes sense. Will add for v2. Thanks for review.

								Honza

> 
> Thanks,
> Chengguang
> 
> > ---
> >  fs/ext2/xattr.c | 15 ++++++---------
> >  1 file changed, 6 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> > index 26a049ca89fb..04a4148d04b3 100644
> > --- a/fs/ext2/xattr.c
> > +++ b/fs/ext2/xattr.c
> > @@ -442,7 +442,9 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> > char *name,
> >  			struct ext2_xattr_entry *next = EXT2_XATTR_NEXT(last);
> >  			if ((char *)next >= end)
> >  				goto bad_block;
> > -			if (!last->e_value_block && last->e_value_size) {
> > +			if (!ext2_xattr_entry_valid(last, sb->s_blocksize))
> > +				goto bad_block;
> > +			if (last->e_value_size) {
> >  				size_t offs = le16_to_cpu(last->e_value_offs);
> >  				if (offs < min_offs)
> >  					min_offs = offs;
> > @@ -482,12 +484,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> > char *name,
> >  		error = -EEXIST;
> >  		if (flags & XATTR_CREATE)
> >  			goto cleanup;
> > -		if (!here->e_value_block && here->e_value_size) {
> > -			if (!ext2_xattr_entry_valid(here, sb->s_blocksize))
> > -				goto bad_block;
> > -			free += EXT2_XATTR_SIZE(
> > -					le32_to_cpu(here->e_value_size));
> > -		}
> > +		free += EXT2_XATTR_SIZE(le32_to_cpu(here->e_value_size));
> >  		free += EXT2_XATTR_LEN(name_len);
> >  	}
> >  	error = -ENOSPC;
> > @@ -552,7 +549,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> > char *name,
> >  		here->e_name_len = name_len;
> >  		memcpy(here->e_name, name, name_len);
> >  	} else {
> > -		if (!here->e_value_block && here->e_value_size) {
> > +		if (here->e_value_size) {
> >  			char *first_val = (char *)header + min_offs;
> >  			size_t offs = le16_to_cpu(here->e_value_offs);
> >  			char *val = (char *)header + offs;
> > @@ -579,7 +576,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> > char *name,
> >  			last = ENTRY(header+1);
> >  			while (!IS_LAST_ENTRY(last)) {
> >  				size_t o = le16_to_cpu(last->e_value_offs);
> > -				if (!last->e_value_block && o < offs)
> > +				if (o < offs)
> >  					last->e_value_offs =
> >  						cpu_to_le16(o + size);
> >  				last = EXT2_XATTR_NEXT(last);
> 
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/3] ext2: Strengthen xattr block checks

[cgxu519]

On Wed, 2019-05-15 at 16:01 +0200, Jan Kara wrote:
> Check every entry in xattr block for validity in ext2_xattr_set() to
> detect on disk corruption early. Also since e_value_block field in xattr
> entry is never != 0 in a valid filesystem, just remove checks for it
> once we have established entries are valid.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Could we do the entry check in the loop of get/list operation too?

Thanks,
Chengguang

> ---
>  fs/ext2/xattr.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index 26a049ca89fb..04a4148d04b3 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -442,7 +442,9 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> char *name,
>  			struct ext2_xattr_entry *next = EXT2_XATTR_NEXT(last);
>  			if ((char *)next >= end)
>  				goto bad_block;
> -			if (!last->e_value_block && last->e_value_size) {
> +			if (!ext2_xattr_entry_valid(last, sb->s_blocksize))
> +				goto bad_block;
> +			if (last->e_value_size) {
>  				size_t offs = le16_to_cpu(last->e_value_offs);
>  				if (offs < min_offs)
>  					min_offs = offs;
> @@ -482,12 +484,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> char *name,
>  		error = -EEXIST;
>  		if (flags & XATTR_CREATE)
>  			goto cleanup;
> -		if (!here->e_value_block && here->e_value_size) {
> -			if (!ext2_xattr_entry_valid(here, sb->s_blocksize))
> -				goto bad_block;
> -			free += EXT2_XATTR_SIZE(
> -					le32_to_cpu(here->e_value_size));
> -		}
> +		free += EXT2_XATTR_SIZE(le32_to_cpu(here->e_value_size));
>  		free += EXT2_XATTR_LEN(name_len);
>  	}
>  	error = -ENOSPC;
> @@ -552,7 +549,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> char *name,
>  		here->e_name_len = name_len;
>  		memcpy(here->e_name, name, name_len);
>  	} else {
> -		if (!here->e_value_block && here->e_value_size) {
> +		if (here->e_value_size) {
>  			char *first_val = (char *)header + min_offs;
>  			size_t offs = le16_to_cpu(here->e_value_offs);
>  			char *val = (char *)header + offs;
> @@ -579,7 +576,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> char *name,
>  			last = ENTRY(header+1);
>  			while (!IS_LAST_ENTRY(last)) {
>  				size_t o = le16_to_cpu(last->e_value_offs);
> -				if (!last->e_value_block && o < offs)
> +				if (o < offs)
>  					last->e_value_offs =
>  						cpu_to_le16(o + size);
>  				last = EXT2_XATTR_NEXT(last);

[PATCH 3/3] ext2: Strengthen xattr block checks

[Jan Kara]

Check every entry in xattr block for validity in ext2_xattr_set() to
detect on disk corruption early. Also since e_value_block field in xattr
entry is never != 0 in a valid filesystem, just remove checks for it
once we have established entries are valid.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/xattr.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
index 26a049ca89fb..04a4148d04b3 100644
--- a/fs/ext2/xattr.c
+++ b/fs/ext2/xattr.c
@@ -442,7 +442,9 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
 			struct ext2_xattr_entry *next = EXT2_XATTR_NEXT(last);
 			if ((char *)next >= end)
 				goto bad_block;
-			if (!last->e_value_block && last->e_value_size) {
+			if (!ext2_xattr_entry_valid(last, sb->s_blocksize))
+				goto bad_block;
+			if (last->e_value_size) {
 				size_t offs = le16_to_cpu(last->e_value_offs);
 				if (offs < min_offs)
 					min_offs = offs;
@@ -482,12 +484,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
 		error = -EEXIST;
 		if (flags & XATTR_CREATE)
 			goto cleanup;
-		if (!here->e_value_block && here->e_value_size) {
-			if (!ext2_xattr_entry_valid(here, sb->s_blocksize))
-				goto bad_block;
-			free += EXT2_XATTR_SIZE(
-					le32_to_cpu(here->e_value_size));
-		}
+		free += EXT2_XATTR_SIZE(le32_to_cpu(here->e_value_size));
 		free += EXT2_XATTR_LEN(name_len);
 	}
 	error = -ENOSPC;
@@ -552,7 +549,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
 		here->e_name_len = name_len;
 		memcpy(here->e_name, name, name_len);
 	} else {
-		if (!here->e_value_block && here->e_value_size) {
+		if (here->e_value_size) {
 			char *first_val = (char *)header + min_offs;
 			size_t offs = le16_to_cpu(here->e_value_offs);
 			char *val = (char *)header + offs;
@@ -579,7 +576,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
 			last = ENTRY(header+1);
 			while (!IS_LAST_ENTRY(last)) {
 				size_t o = le16_to_cpu(last->e_value_offs);
-				if (!last->e_value_block && o < offs)
+				if (o < offs)
 					last->e_value_offs =
 						cpu_to_le16(o + size);
 				last = EXT2_XATTR_NEXT(last);
-- 
2.16.4