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

[PATCH 0/3] 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.

								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 | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
index fb2e008d4406..26a049ca89fb 100644
--- a/fs/ext2/xattr.c
+++ b/fs/ext2/xattr.c
@@ -437,27 +437,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
 			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. */
+		last = FIRST_ENTRY(bh);
 		while (!IS_LAST_ENTRY(last)) {
 			struct ext2_xattr_entry *next = EXT2_XATTR_NEXT(last);
 			if ((char *)next >= end)
@@ -467,8 +447,18 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
 				if (offs < min_offs)
 					min_offs = offs;
 			}
+			if (not_found) {
+				if (name_index == last->e_name_index &&
+				    name_len == last->e_name_len &&
+				    !memcmp(name, last->e_name,name_len)) {
+					not_found = 0;
+					here = last;
+				}
+			}
 			last = next;
 		}
+		if (not_found)
+			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 | 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

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

[cgxu519]

On Wed, 2019-05-15 at 16:01 +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>
> ---
>  fs/ext2/xattr.c | 32 +++++++++++---------------------
>  1 file changed, 11 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index fb2e008d4406..26a049ca89fb 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -437,27 +437,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> char *name,
>  			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. */
> +		last = FIRST_ENTRY(bh);
>  		while (!IS_LAST_ENTRY(last)) {
>  			struct ext2_xattr_entry *next = EXT2_XATTR_NEXT(last);
>  			if ((char *)next >= end)
> @@ -467,8 +447,18 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> char *name,
>  				if (offs < min_offs)
>  					min_offs = offs;
>  			}
> +			if (not_found) {
> +				if (name_index == last->e_name_index &&
> +				    name_len == last->e_name_len &&
> +				    !memcmp(name, last->e_name,name_len)) {
> +					not_found = 0;
> +					here = last;
> +				}
> +			}
>  			last = next;
>  		}
> +		if (not_found)
> +			here = last;

Entry name is sorted so I think for new entry we should find right place for it
not just appending to last.

Thanks,
Chengguang

>  
>  		/* 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 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);

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

[Jan Kara]

On Thu 16-05-19 09:13:34, cgxu519@zoho.com.cn wrote:
> On Wed, 2019-05-15 at 16:01 +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>
> > ---
> >  fs/ext2/xattr.c | 32 +++++++++++---------------------
> >  1 file changed, 11 insertions(+), 21 deletions(-)
> > 
> > diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> > index fb2e008d4406..26a049ca89fb 100644
> > --- a/fs/ext2/xattr.c
> > +++ b/fs/ext2/xattr.c
> > @@ -437,27 +437,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> > char *name,
> >  			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. */
> > +		last = FIRST_ENTRY(bh);
> >  		while (!IS_LAST_ENTRY(last)) {
> >  			struct ext2_xattr_entry *next = EXT2_XATTR_NEXT(last);
> >  			if ((char *)next >= end)
> > @@ -467,8 +447,18 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> > char *name,
> >  				if (offs < min_offs)
> >  					min_offs = offs;
> >  			}
> > +			if (not_found) {
> > +				if (name_index == last->e_name_index &&
> > +				    name_len == last->e_name_len &&
> > +				    !memcmp(name, last->e_name,name_len)) {
> > +					not_found = 0;
> > +					here = last;
> > +				}
> > +			}
> >  			last = next;
> >  		}
> > +		if (not_found)
> > +			here = last;
> 
> Entry name is sorted so I think for new entry we should find right place for it
> not just appending to last.

Ah, good catch! I actually didn't find a place which would use the fact
that names are sorted (and that's why xfstests passed fine as well) but
you're right that the old code worked that way and we should keep that.
Thanks!

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

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