diff mbox series

x86/mm: Simplify redundant overlap calculation

Message ID 20240123163623.1342917-1-dave.hansen@linux.intel.com
State New
Headers
Series x86/mm: Simplify redundant overlap calculation |

Commit Message

Dave Hansen Jan. 23, 2024, 4:36 p.m. UTC 
  There have been a couple of reports that the two sides of the
overlaps() calculation are redundant.  I spent way too much time
looking at this, but I became convinced that they are redundant
when a little test program of mine produced identical disassembly
for both versions of the check.

Remove the second condition.  It is exactly the same as the first.

Fixes: 91ee8f5c1f50 ("x86/mm/cpa: Allow range check for static protections")
Reported-by: David Binderman <dcb314@hotmail.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: x86@kernel.org
---
 arch/x86/mm/pat/set_memory.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Sohil Mehta Jan. 23, 2024, 7:19 p.m. UTC | #1 
On 1/23/2024 9:00 AM, Dave Hansen wrote:
> On 1/23/24 08:54, David Binderman wrote:
>>> Remove the second condition.  It is exactly the same as the first.
>> I don't think the first condition is sufficient. I suspect something like
>>
>>        return (r2_start <= r1_start && r1_start <= r2_end) ||
>>                (r2_start <= r1_end && r1_end <= r2_end);
>>

This check seems accurate however Dave's single line check below also
looks accurate to me. See the analysis below.

>> Given the range [r2_start .. r2_end], then if r1_start or r1_end
>> are in that range, you have overlap.
>>
>> Unless you know different.
> 
> First of all, I've gotten these bounds checks wrong in code more times
> than I can count.  I have zero trust that I'll get them right. :)
> 
> But the compiler seems to know different at least:
> 
> int  overlaps1(unsigned long r1_start, unsigned long r1_end,
> 	      unsigned long r2_start, unsigned long r2_end)
> {
> 	return  (r1_start <= r2_end && r1_end >= r2_start) ||
> 		(r2_start <= r1_end && r2_end >= r1_start);
> }

Dave, I think if you change the order of the && in the 2nd check it
makes it easier to visually realize that both of these checks are indeed
the same:

(r1_start <= r2_end  )	&& (r1_end   >= r2_start)
			||
(r2_end   >= r1_start)	&& (r2_start <= r1_end  )

The first operation in () on both lines is exactly the same. Same is
true for the second operation after the &&.

> 
> int  overlaps2(unsigned long r1_start, unsigned long r1_end,
> 	      unsigned long r2_start, unsigned long r2_end)
> {
> 	return (r1_start <= r2_end && r1_end >= r2_start);
> }
> 

I completely agree that overlap1() and overlap2() are expected to
generate the same output for any input.

However, the next question is whether overlap2() is enough to detect
there is indeed an overlap between the ranges. I find that would be true
based on the assumption that the end is always greater than or equal to
the start in both ranges.

I have now spent way too much time on this. But if you rearrange the
check in overlaps2() as below then I find it easier to put it in words:

(r1_start <= r2_end && r2_start <= r1_end)

"Both of the ranges have to start before either of ranges end for there
to be an overlap".

Sohil
Sohil Mehta Jan. 23, 2024, 7:28 p.m. UTC | #2 
On 1/23/2024 8:36 AM, Dave Hansen wrote:
> There have been a couple of reports that the two sides of the
> overlaps() calculation are redundant.  I spent way too much time
> looking at this, but I became convinced that they are redundant
> when a little test program of mine produced identical disassembly
> for both versions of the check.
> 
> Remove the second condition.  It is exactly the same as the first.
> 
> Fixes: 91ee8f5c1f50 ("x86/mm/cpa: Allow range check for static protections")
> Reported-by: David Binderman <dcb314@hotmail.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: x86@kernel.org
> ---
>  arch/x86/mm/pat/set_memory.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 

Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>

> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> index e9b448d1b1b70..fdc00516c0b54 100644
> --- a/arch/x86/mm/pat/set_memory.c
> +++ b/arch/x86/mm/pat/set_memory.c
> @@ -435,8 +435,7 @@ static void cpa_flush(struct cpa_data *data, int cache)
>  static bool overlaps(unsigned long r1_start, unsigned long r1_end,
>  		     unsigned long r2_start, unsigned long r2_end)
>  {
> -	return (r1_start <= r2_end && r1_end >= r2_start) ||
> -		(r2_start <= r1_end && r2_end >= r1_start);
> +	return (r1_start <= r2_end && r1_end >= r2_start);
>  }
>  
>  #ifdef CONFIG_PCI_BIOS
diff mbox series

Patch

diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index e9b448d1b1b70..fdc00516c0b54 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c
@@ -435,8 +435,7 @@  static void cpa_flush(struct cpa_data *data, int cache)
 static bool overlaps(unsigned long r1_start, unsigned long r1_end,
 		     unsigned long r2_start, unsigned long r2_end)
 {
-	return (r1_start <= r2_end && r1_end >= r2_start) ||
-		(r2_start <= r1_end && r2_end >= r1_start);
+	return (r1_start <= r2_end && r1_end >= r2_start);
 }
 
 #ifdef CONFIG_PCI_BIOS