Testing the lws_compare_and_swap_2 syscall

Commit Message

Helge Deller Nov. 6, 2017, 9:27 p.m. UTC

* Christoph Biedl <[e-mail]>:
> John David Anglin wrote...
> 
> > On 2017-10-24, at 4:03 PM, Christoph Biedl wrote:
> > 
> > > To start with, using the invalid value 4 as size parameter does not
> > > return ENOSYS as I'd expect but crashes my system[2], using both 32 and
> > > 64 bit kernel, no root privileges required.
> > 
> >         /* Check the validity of the size pointer */
> >         subi,>>= 4, %r23, %r0
> >         b,n     lws_exit_nosys
> > 
> > The condition in the subi instruction should be ">>".  The branch is incorrectly nullified when
> > %r23 is 4.

I'd prefer this (untested) patch:



> Ups, way too obvious. This does the trick, or: Tested-By:
> Should I try to get a CVE number for this, or is parisc considered
> *that* historical nobody actually cares?

I think a CVE is not needed for parisc. There are no real productive
users (I assume).


> Now, using the given cmpxchg2 function, the following code tests this
> LWS for 32bit: The first test is for *mem == *old, the second for
> *mem != *old. Is there anything wrong with this?
> 
>     uint32_t mem;
>     uint32_t old;
>     uint32_t new;
> 
>     mem = 1;
>     old = 1;
>     new = 3;
>     cmpxchg2 (&mem, &old, &new, 2);
>     if (mem == old) {
>         printf ("PASS: mem unchanged\n");
>     } else {
>         printf ("FAIL: got = 0x%x, expected = 0x%x\n", mem, old);
>     }
> 
>     mem = 1;
>     old = 3;
>     new = 3;
>     cmpxchg2 (&mem, &old, &new, 2);
>     if (mem == new) {
>         printf ("PASS: mem changed\n");
>     } else {
>         printf ("FAIL: got = 0x%x, expected = 0x%x\n", mem, new);
>     }
> 
> My problem here: Both tests fail, and so do more complex ones that test
> the other data sizes as well.
> 
> After staring at lws_compare_and_swap_2 a long time it seems there are
> two issues: First, there is more usage of ",ma" so an update of mem/r26
> hits the wrong place. After that, all results are the wrong way around.
> I'm frightened to tell but I fear the logic in lws_compare_and_swap_2
> is inverted in each and every place. I'm happy to be convinced
> otherwise. But for the time being it seems the patch below is an
> improvement.
> 
> Status: My system boots and all my tests pass. However, I find smartd
> stalling in 100% CPU, might be coincidence. Now I'm putting some more
> load onto the box to see whether it's less crashy then it used to be the
> previous weeks.

Did you continued your tests?
How were the results.

> Aside, there is another ",ma" modifier in lws_compare_and_swap that I
> fail to understand. Haven't checked yet in detail yet, though.

I'd suggest to keep the other ",ma" modifiers for now.

Helge
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

John David Anglin Nov. 6, 2017, 10:45 p.m. UTC | #1

On 2017-11-06 4:27 PM, Helge Deller wrote:
>   	/* Check the validity of the size pointer */
> -	subi,>>= 4, %r23, %r0
> -	b,n	lws_exit_nosys
> +	cmpib,COND(<<),n 3, %r23, lws_exit_nosys
I don't believe that we want to use COND here (i.e., we want a 32-bit 
check).  We might not
need to trim the upper 32-bits r23.

The reason the code uses nullification is the fast path occurs when the 
"b,n" is nullified.   So we
avoid the branch prediction penalty.  I'd have to check whether the fast 
path with the cmpib instruction
is the taken branch or not.

Have no object to changing "4" to "3" instead of changing the condition.

Dave

John David Anglin Nov. 7, 2017, 9:31 p.m. UTC | #2

On 2017-11-06 5:45 PM, John David Anglin wrote:
> On 2017-11-06 4:27 PM, Helge Deller wrote:
>>       /* Check the validity of the size pointer */
>> -    subi,>>= 4, %r23, %r0
>> -    b,n    lws_exit_nosys
>> +    cmpib,COND(<<),n 3, %r23, lws_exit_nosys
> I don't believe that we want to use COND here (i.e., we want a 32-bit 
> check).  We might not
> need to trim the upper 32-bits r23.
>
> The reason the code uses nullification is the fast path occurs when 
> the "b,n" is nullified.   So we
> avoid the branch prediction penalty.  I'd have to check whether the 
> fast path with the cmpib instruction
> is the taken branch or not.
For cmpib with "<<" condition, the hint for a backward branch is likely 
taken.  The branch to
lws_exit_nosys is backward.

Dave

Christoph Biedl Nov. 7, 2017, 9:46 p.m. UTC | #3

Helge Deller wrote...

> * Christoph Biedl <[e-mail]>:

> > Status: My system boots and all my tests pass. However, I find smartd
> > stalling in 100% CPU, might be coincidence. Now I'm putting some more
> > load onto the box to see whether it's less crashy then it used to be the
> > previous weeks.
>
> Did you continued your tests?
> How were the results.

Basically I gave up on this. My change made things worse, and while I
still didn't get the point (read: For my understanding there's a
discrepancy between the syscall's description and what actually
happens), it's certainly better to keep things the way they are.

> > Aside, there is another ",ma" modifier in lws_compare_and_swap that I
> > fail to understand. Haven't checked yet in detail yet, though.
>
> I'd suggest to keep the other ",ma" modifiers for now.

Since, as I presume, this has no impact on performance, this boils down
to aid a human reviewing the code in understanding what's happening
here.

    Christoph
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/parisc/kernel/syscall.S b/arch/parisc/kernel/syscall.S
index 41e60a9c7db2..f8a8754322ae 100644
--- a/arch/parisc/kernel/syscall.S
+++ b/arch/parisc/kernel/syscall.S
@@ -698,8 +698,7 @@  lws_compare_and_swap_2:
 #endif
 
 	/* Check the validity of the size pointer */
-	subi,>>= 4, %r23, %r0
-	b,n	lws_exit_nosys
+	cmpib,COND(<<),n 3, %r23, lws_exit_nosys
 
 	/* Jump to the functions which will load the old and new values into
 	   registers depending on the their size */