Testing the lws_compare_and_swap_2 syscall

Testing the lws_compare_and_swap_2 syscall

[Christoph Biedl]

Hello,

looking into John's recent fix for lws_compare_and_swap_2 on 32bit
systems I got the feeling things still aren't right yet. To defeat
or prove that, also since I'd like to learn more about this ... I wrote
a small program the uses that syscall, and things break galore.

Could you please check the code below[1] for obvious usage errors? Note
the entire cmpxchg2 function was copied from gcc, and the disassembly
output provided by objdump looks correct as far as I can tell.

The program takes four numerical parameters that correspond to the
four parameters of the syscall.

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. This should never happen.

Regards,
    Christoph

[1]
======================================================================
#include <errno.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>

/* borrowed from __kernel_cmpxchg2 in libgcc/config/pa/linux-atomic.c in the gcc sources */
static inline long
cmpxchg2 (void *mem, const void *oldval, const void *newval,
                   int val_size)
{
  register unsigned long lws_mem asm("r26") = (unsigned long) (mem);
  register unsigned long lws_old asm("r25") = (unsigned long) oldval;
  register unsigned long lws_new asm("r24") = (unsigned long) newval;
  register int lws_size asm("r23") = val_size;
  register long lws_ret   asm("r28");
  register long lws_errno asm("r21");
  asm volatile (        "ble    0xb0(%%sr2, %%r0)       \n\t"
                        "ldi    %6, %%r20               \n\t"
        : "=r" (lws_ret), "=r" (lws_errno), "+r" (lws_mem),
          "+r" (lws_old), "+r" (lws_new), "+r" (lws_size)
        : "i" (2)
        : "r1", "r20", "r22", "r29", "r31", "fr4", "memory"
  );

  /* If the kernel LWS call is successful, lws_ret contains 0.  */
  if (__builtin_expect (lws_ret == 0, 1))
    return 0;

  if (__builtin_expect (lws_errno == -EFAULT || lws_errno == -ENOSYS, 0))
    __builtin_trap ();

  /* If the kernel LWS call fails with no error, return -EBUSY */
  if (__builtin_expect (!lws_errno, 0))
    return -EBUSY;

  return lws_errno;
}



int main (int argc, char **argv) {

    if (argc != 5) {
        printf ("usage <mem> <old> <new> <size>\n");
        exit (1);
    }

    uint64_t a = atoi (argv[1]);
    uint64_t b = atoi (argv[2]);
    uint64_t c = atoi (argv[3]);
    unsigned long size = atoi (argv[4]);

    printf ("a = 0x%016llx, b = 0x%016llx, c = 0x%016llx\n",
        a,
        b,
        c
    );

    unsigned long r = cmpxchg2 (&a, &b, &c, size);

    printf ("a = 0x%016llx\n", a);
    printf ("r = 0x%lx\n", r);

    return 0;
}
======================================================================


[2]
| Backtrace:
|
| Kernel Fault: Code=26 (Data memory access rights trap) regs=000000007ca8f738 (Addr=0000000000000002)
| CPU: 0 PID: 1289 Comm: a.out Not tainted 4.12.0-2-parisc64-smp #1 Debian 4.12.13-1
| task: 000000007ca8eec0 task.stack: 000000007cb68000
|
|      YZrvWESTHLNXBCVMcbcbcbcbOGFRQPDI
| PSW: 00001000000001101111111100001111 Not tainted
| r00-03  000000ff0806ff0f 0000000040cf2000 0000000000010987 00000000f93963c0
| r04-07  00000000f853fc70 00000000ffffffff 00000000ffffffff 00000000ffffffff
| r08-11  00000000fffffffe 00000000ffffffff 00000000fffffffd 00000000000ed000
| r12-15  00000000ffffffff 0000000000911d28 0000000000000000 0000000000117be8
| r16-19  00000000009ce448 0000000000000000 0000000000000001 00000000f9396328
| r20-23  0000000000000002 00000000000004d4 00000000f844a08c 0000000000000004
| r24-27  00000000f9396330 00000000f9396328 00000000f9396320 0000000000011100
| r28-31  0000000040cf2528 0000000000000010 00000000f9396400 00000000000106e7
| sr00-03  00000000003a7800 0000000000000000 0000000000000000 00000000003a7800
| sr04-07  00000000003a7800 00000000003a7800 00000000003a7800 00000000003a7800
|
| IASQ: 0000000000000000 0000000000000000 IAOQ: 0000000000000578 000000000000057c
|  IIR: 0e8095dc    ISR: 0000000000000000  IOR: 0000000000000002
|  CPU:        0   CR30: 000000007cb68000 CR31: 0000000011111111
|  ORIG_R28: 0000000000000000
|  IAOQ[0]: 0x578
|  IAOQ[1]: 0x57c
|  RP(r2): 0x10987
| Backtrace:
|
| Kernel panic - not syncing: Kernel Fault
| ---[ end Kernel panic - not syncing: Kernel Fault

Re: Testing the lws_compare_and_swap_2 syscall

[John David Anglin]

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.

Dave
--
John David Anglin	dave.anglin@bell.net

Re: Testing the lws_compare_and_swap_2 syscall

[Christoph Biedl]

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.

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?


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.

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

Cheers,
    Christoph

--- a/src/arch/parisc/kernel/syscall.S
+++ b/src/arch/parisc/kernel/syscall.S
@@ -796,30 +796,30 @@
 	ldo	1(%r0),%r28
 
 	/* 8bit CAS */
-13:	ldb,ma	0(%r26), %r29
-	sub,=	%r29, %r25, %r0
+13:	ldb	0(%r26), %r29
+	sub,<>	%r29, %r25, %r0
 	b,n	cas2_end
-14:	stb,ma	%r24, 0(%r26)
+14:	stb	%r24, 0(%r26)
 	b	cas2_end
 	copy	%r0, %r28
 	nop
 	nop
 
 	/* 16bit CAS */
-15:	ldh,ma	0(%r26), %r29
-	sub,=	%r29, %r25, %r0
+15:	ldh	0(%r26), %r29
+	sub,<>	%r29, %r25, %r0
 	b,n	cas2_end
-16:	sth,ma	%r24, 0(%r26)
+16:	sth	%r24, 0(%r26)
 	b	cas2_end
 	copy	%r0, %r28
 	nop
 	nop
 
 	/* 32bit CAS */
-17:	ldw,ma	0(%r26), %r29
-	sub,=	%r29, %r25, %r0
+17:	ldw	0(%r26), %r29
+	sub,<>	%r29, %r25, %r0
 	b,n	cas2_end
-18:	stw,ma	%r24, 0(%r26)
+18:	stw	%r24, 0(%r26)
 	b	cas2_end
 	copy	%r0, %r28
 	nop
@@ -827,21 +827,22 @@
 
 	/* 64bit CAS */
 #ifdef CONFIG_64BIT
-19:	ldd,ma	0(%r26), %r29
-	sub,*=	%r29, %r25, %r0
+19:	ldd	0(%r26), %r29
+	sub,<>	%r29, %r25, %r0
 	b,n	cas2_end
-20:	std,ma	%r24, 0(%r26)
+20:	std	%r24, 0(%r26)
 	copy	%r0, %r28
 #else
 	/* Compare first word */
 19:	ldw	0(%r26), %r29
 	sub,=	%r29, %r22, %r0
-	b,n	cas2_end
+	b,n	cas2_64set
 	/* Compare second word */
 20:	ldw	4(%r26), %r29
-	sub,=	%r29, %r23, %r0
+	sub,<>	%r29, %r23, %r0
 	b,n	cas2_end
 	/* Perform the store */
+cas2_64set:
 21:	fstdx	%fr4, 0(%r26)
 	copy	%r0, %r28
 #endif

Re: Testing the lws_compare_and_swap_2 syscall

[John David Anglin]

On 2017-10-25 8:22 PM, Christoph Biedl wrote:
> 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.
The other uses of ",ma" are not a problem as the increment value is 0.  
So, the pointer
is unchanged.  The double word case had an increment of 4 messing up the 
pointer.

Technically, ",ma" with a zero increment provides an ordered load. 
However, the completer
probably isn't necessary as I believe all loads and stores are ordered 
on real hardware.

The "=" completer in the "sub" instructions looks correct to me. When 
*mem and old are
equal, the "b,n     cas2_end" instruction is nullified and new is stored 
in mem.  See comment
from code:

         /*
                 prev = *addr;
                 if ( prev == old )
                   *addr = new;
                 return prev;
         */

In your first test example, old and new should be exchanged in mem. Pass 
should be
mem == new.  Second case should be mem == old.

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: Testing the lws_compare_and_swap_2 syscall

[Helge Deller]

* Christoph Biedl <linux-kernel.bfrz@manchmal.in-ulm.de>:
> 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:

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 */


> 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

Re: Testing the lws_compare_and_swap_2 syscall

[John David Anglin]

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  dave.anglin@bell.net

Re: Testing the lws_compare_and_swap_2 syscall

[John David Anglin]

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

-- 
John David Anglin  dave.anglin@bell.net

Re: Testing the lws_compare_and_swap_2 syscall

[Christoph Biedl]

Helge Deller wrote...

> * Christoph Biedl <linux-kernel.bfrz@manchmal.in-ulm.de>:

> > 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