Testing the lws_compare_and_swap_2 syscall

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

        /* 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 [email protected]

John David Anglin wrote...

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

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

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  [email protected]

* Christoph Biedl <[email protected]>:

I'd prefer this (untested) patch:

index 41e60a9c7db2..f8a8754322ae 100644
@@ -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 */

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

Did you continued your tests?
How were the results.

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

Helge

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  [email protected]

Helge Deller wrote...

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.

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