* [PATCH 0/3] ext2: Cleanup ext2_xattr_set()
@ 2019-05-15 14:01 Jan Kara
2019-05-15 14:01 ` [PATCH 1/3] ext2: introduce helper for xattr entry validation Jan Kara
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Jan Kara @ 2019-05-15 14:01 UTC (permalink / raw)
To: linux-ext4; +Cc: Chengguang Xu, 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
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/3] ext2: introduce helper for xattr entry validation 2019-05-15 14:01 [PATCH 0/3] ext2: Cleanup ext2_xattr_set() Jan Kara @ 2019-05-15 14:01 ` Jan Kara 2019-05-15 14:01 ` [PATCH 2/3] ext2: Merge loops in ext2_xattr_set() Jan Kara 2019-05-15 14:01 ` [PATCH 3/3] ext2: Strengthen xattr block checks Jan Kara 2 siblings, 0 replies; 10+ messages in thread From: Jan Kara @ 2019-05-15 14:01 UTC (permalink / raw) To: linux-ext4; +Cc: Chengguang Xu, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] ext2: Merge loops in ext2_xattr_set() 2019-05-15 14:01 [PATCH 0/3] ext2: Cleanup ext2_xattr_set() Jan Kara 2019-05-15 14:01 ` [PATCH 1/3] ext2: introduce helper for xattr entry validation Jan Kara @ 2019-05-15 14:01 ` Jan Kara 2019-05-16 1:13 ` cgxu519 2019-05-15 14:01 ` [PATCH 3/3] ext2: Strengthen xattr block checks Jan Kara 2 siblings, 1 reply; 10+ messages in thread From: Jan Kara @ 2019-05-15 14:01 UTC (permalink / raw) To: linux-ext4; +Cc: Chengguang Xu, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ext2: Merge loops in ext2_xattr_set() 2019-05-15 14:01 ` [PATCH 2/3] ext2: Merge loops in ext2_xattr_set() Jan Kara @ 2019-05-16 1:13 ` cgxu519 2019-05-16 7:52 ` Jan Kara 0 siblings, 1 reply; 10+ messages in thread From: cgxu519 @ 2019-05-16 1:13 UTC (permalink / raw) To: Jan Kara, linux-ext4 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); ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ext2: Merge loops in ext2_xattr_set() 2019-05-16 1:13 ` cgxu519 @ 2019-05-16 7:52 ` Jan Kara 0 siblings, 0 replies; 10+ messages in thread From: Jan Kara @ 2019-05-16 7:52 UTC (permalink / raw) To: cgxu519; +Cc: Jan Kara, linux-ext4 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] ext2: Strengthen xattr block checks 2019-05-15 14:01 [PATCH 0/3] ext2: Cleanup ext2_xattr_set() Jan Kara 2019-05-15 14:01 ` [PATCH 1/3] ext2: introduce helper for xattr entry validation Jan Kara 2019-05-15 14:01 ` [PATCH 2/3] ext2: Merge loops in ext2_xattr_set() Jan Kara @ 2019-05-15 14:01 ` Jan Kara 2019-05-16 1:16 ` cgxu519 2 siblings, 1 reply; 10+ messages in thread From: Jan Kara @ 2019-05-15 14:01 UTC (permalink / raw) To: linux-ext4; +Cc: Chengguang Xu, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] ext2: Strengthen xattr block checks 2019-05-15 14:01 ` [PATCH 3/3] ext2: Strengthen xattr block checks Jan Kara @ 2019-05-16 1:16 ` cgxu519 2019-05-16 8:29 ` Jan Kara 0 siblings, 1 reply; 10+ messages in thread From: cgxu519 @ 2019-05-16 1:16 UTC (permalink / raw) To: Jan Kara, linux-ext4 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); ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] ext2: Strengthen xattr block checks 2019-05-16 1:16 ` cgxu519 @ 2019-05-16 8:29 ` Jan Kara 0 siblings, 0 replies; 10+ messages in thread From: Jan Kara @ 2019-05-16 8:29 UTC (permalink / raw) To: cgxu519; +Cc: Jan Kara, linux-ext4 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 0/3 v2] ext2: Cleanup ext2_xattr_set() @ 2019-05-16 10:03 Jan Kara 2019-05-16 10:03 ` [PATCH 2/3] ext2: Merge loops in ext2_xattr_set() Jan Kara 0 siblings, 1 reply; 10+ messages in thread From: Jan Kara @ 2019-05-16 10:03 UTC (permalink / raw) To: linux-ext4; +Cc: cgxu519, 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] ext2: Merge loops in ext2_xattr_set() 2019-05-16 10:03 [PATCH 0/3 v2] ext2: Cleanup ext2_xattr_set() Jan Kara @ 2019-05-16 10:03 ` Jan Kara 2019-05-16 11:11 ` cgxu519 0 siblings, 1 reply; 10+ messages in thread From: Jan Kara @ 2019-05-16 10:03 UTC (permalink / raw) To: linux-ext4; +Cc: cgxu519, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ext2: Merge loops in ext2_xattr_set() 2019-05-16 10:03 ` [PATCH 2/3] ext2: Merge loops in ext2_xattr_set() Jan Kara @ 2019-05-16 11:11 ` cgxu519 0 siblings, 0 replies; 10+ messages in thread From: cgxu519 @ 2019-05-16 11:11 UTC (permalink / raw) To: Jan Kara, linux-ext4 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); ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-05-16 11:11 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-15 14:01 [PATCH 0/3] ext2: Cleanup ext2_xattr_set() Jan Kara 2019-05-15 14:01 ` [PATCH 1/3] ext2: introduce helper for xattr entry validation Jan Kara 2019-05-15 14:01 ` [PATCH 2/3] ext2: Merge loops in ext2_xattr_set() Jan Kara 2019-05-16 1:13 ` cgxu519 2019-05-16 7:52 ` Jan Kara 2019-05-15 14:01 ` [PATCH 3/3] ext2: Strengthen xattr block checks Jan Kara 2019-05-16 1:16 ` cgxu519 2019-05-16 8:29 ` Jan Kara 2019-05-16 10:03 [PATCH 0/3 v2] ext2: Cleanup ext2_xattr_set() Jan Kara 2019-05-16 10:03 ` [PATCH 2/3] ext2: Merge loops in ext2_xattr_set() Jan Kara 2019-05-16 11:11 ` cgxu519
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.