linux-ext4 - Re: [PATCH 2/3] ext2: Merge loops in ext2_xattr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190516075242.GA13274@quack2.suse.cz>
Date:   Thu, 16 May 2019 09:52:42 +0200
From:   Jan Kara <jack@...e.cz>
To:     "cgxu519@...o.com.cn" <cgxu519@...o.com.cn>
Cc:     Jan Kara <jack@...e.cz>, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 2/3] ext2: Merge loops in ext2_xattr_set()

On Thu 16-05-19 09:13:34, cgxu519@...o.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@...e.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@...e.com>
SUSE Labs, CR