linux-ext4 - Re: [PATCH] ext4: Refactor breaking condition for xattr_find

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <aGyKC3ig4N74PPcX@vaxr-BM6660-BM6360>
Date: Tue, 8 Jul 2025 11:01:31 +0800
From: I Hsin Cheng <richard120310@...il.com>
To: Theodore Ts'o <tytso@....edu>
Cc: adilger.kernel@...ger.ca, linux-ext4@...r.kernel.org,
	linux-kernel@...r.kernel.org, skhan@...uxfoundation.org,
	linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH] ext4: Refactor breaking condition for xattr_find_entry()

On Mon, Jul 07, 2025 at 10:24:53PM -0400, Theodore Ts'o wrote:
> On Tue, Jul 08, 2025 at 10:00:13AM +0800, I Hsin Cheng wrote:
> > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> > index 8d15acbacc20..1993622e3c74 100644
> > --- a/fs/ext4/xattr.c
> > +++ b/fs/ext4/xattr.c
> > @@ -338,7 +338,7 @@ xattr_find_entry(struct inode *inode, struct ext4_xattr_entry **pentry,
> >  			cmp = name_len - entry->e_name_len;
> >  		if (!cmp)
> >  			cmp = memcmp(name, entry->e_name, name_len);
> > -		if (cmp <= 0 && (sorted || cmp == 0))
> > +		if (!cmp || (cmp < 0 && sorted))
> 
> This is *not* identical.  Suppose memcmp returns a positive value
> (say, 1).  Previously, the conditional would be false.  With your
> change, !cmp would be true, so the overall conditional would be true.
> 
> So this does not appear to be a valid transformation.
> 
> (Note that valid transformations will be done by the compiler
> automatically, without needing to make code changes.)
> 
>    	     	 	      - Ted


Hi Ted,

> This is *not* identical.  Suppose memcmp returns a positive value
> (say, 1).  Previously, the conditional would be false.  With your
> change, !cmp would be true, so the overall conditional would be true.

I would argue that "!cmp" is only true when "cmp" is zero, otherwise
it'll be false no matter the number is positive or negative.

With some transformation according to Demorgan's Law, the following
expressions are equivalent
* "cmp <= 0 && (sorted || cmp == 0)"
* "(cmp <= 0 && sorted) || (cmp <= 0 && cmp == 0)"
* "(cmp <= 0 && sorted) || (cmp == 0)"
* "(cmp == 0) || (cmp <= 0 && sorted)"

Because when "cmp == 0" (which is "!cmp"), the condition is going to
take shortcut, so we can further simplify "(cmp <= 0 && sorted)" to
"(cmp < 0 && sorted)", since "cmp" isn't going to be 0 when entering
this part.

When you put any non-zero value for "cmp", "!cmp" is going to be false
so it will further check whether "(cmp < 0 && sorted)".

This is my derivation flow, let me know if there's anything wrong in it.

> (Note that valid transformations will be done by the compiler
> automatically, without needing to make code changes.)

Makes sense, thanks for the head up, but I think we do have some
benefits from it when compiling with -O2 optimization level?

As the bloat-o-meter indicates the code generation size can actually be
shrinked.

Best regards,
I Hsin Cheng