NetBSD Problem Report #38482

From steve@mctavish.co.uk  Mon Apr 21 20:56:23 2008
Return-Path: <steve@mctavish.co.uk>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by narn.NetBSD.org (Postfix) with ESMTP id 44DD063B293
	for <gnats-bugs@gnats.NetBSD.org>; Mon, 21 Apr 2008 20:56:23 +0000 (UTC)
Message-Id: <20080421205613.A2E59CC6F5@oor-wullie.mctavish.co.uk>
Date: Mon, 21 Apr 2008 21:56:13 +0100 (BST)
To: gnats-bugs@gnats.NetBSD.org
Subject: C compiler can generate non-restartable code within a RAS
X-Send-Pr-Version: 3.95

>Number:         38482
>Category:       lib
>Synopsis:       C compiler can generate non-restartable code within a RAS.
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    lib-bug-people
>State:          analyzed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Apr 21 21:00:00 +0000 2008
>Closed-Date:    
>Last-Modified:  Thu Jun 03 10:29:50 +0000 2021
>Originator:     Steve Woodford
>Release:        NetBSD 4.99.60
>Organization:
>Environment:
Architecture: arm, hppa, m68000, mips, sh3, sparc, vax
Machine: arm, hppa, m68000, mips, sh3, sparc, vax
>Description:
I've recently noticed occasional SEGVs with threaded programs on ARM.
This turns out to be caused by dereferencing a bad pointer value in the
code between the RAS_START and RAS_END in
common/lib/libc/atomic/atomic_init_testset.c:_atomic_cas_up().

Examining the assembly code produced by Gcc shows the reason:

 108:   e1a03000        mov     r3, r0
0000010c <_atomic_cas_ras_start>:
 10c:   e5900000        ldr     r0, [r0]
 110:   e1500001        cmp     r0, r1
 114:   11a0f00e        movne   pc, lr
 118:   e5832000        str     r2, [r3]
0000011c <_atomic_cas_ras_end>:
 11c:   e1a0f00e        mov     pc, lr

Register r0 is modified within the sequence, therefore a bogus pointer
dereference is almost inevitable if the sequence is restarted.

It is likely all other archs which lack native CAS are at risk from
similar lossage. A quick check shows arm, hppa, m68000, mips, sh3, sparc,
and vax all use atomic_init_testset.c. I've only examined the compiled
code on ARM so I can't verify the other archs are similarly affected.
Even if an arch isn't currently affected, any future toolchain update or
minor change to the source for _atomic_cas_up() could easily break it.

>How-To-Repeat:
Run threaded code on affected archs (ARM in my case).
Observe occasional, seemingly random, SEGVs.

>Fix:
Rather than fighting the compiler, I propose we implement _atomic_cas_up()
in asm code for each affected arch. I can do ARM and m68000.

In addition, the rasctl(2) manual page should be updated to strongly
recommend that RASs are implemented in asm code only. Perhaps we should go
so far as removing the RAS_START/RAS_END macros from the API, though that
may preclude using in-line asm...

An audit of the tree for similar code sequences would also be a good idea.
For example: pthread_lock.c.

>Release-Note:

>Audit-Trail:
From: Steve Woodford <scw@netbsd.org>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org,
 gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org
Subject: Re: lib/38482: C compiler can generate non-restartable code within a RAS
Date: Mon, 21 Apr 2008 22:38:52 +0100

 --Boundary-00=_tlQDInHx5ZUGTgu
 Content-Type: text/plain;
   charset="iso-8859-1"
 Content-Transfer-Encoding: 7bit
 Content-Disposition: inline

 A patch which addresses this problem for ARM and m68000 is attached.

 The patch adds -D__HAVE_ASM_ATOMIC_CAS_UP to CPPFLAGS in the ARM and 
 m68000 Makefile.inc fragments, which atomic_init_testset.c uses to 
 determine if an asm version of atomic_cas_up() is present.

 Once all affected archs have been fixed, -D__HAVE_ASM_ATOMIC_CAS_UP can 
 be removed.

 The namespace police may want to give this the once-over. Unless I hear 
 any objections, I'll commit it in a few days.

 Cheers, Steve

 --Boundary-00=_tlQDInHx5ZUGTgu
 Content-Type: text/x-diff;
   charset="iso-8859-1";
   name="atomic_cas.diff"
 Content-Transfer-Encoding: 7bit
 Content-Disposition: attachment;
 	filename="atomic_cas.diff"

 diff -ruN -x CVS /export/netbsd/pristine/common/lib/libc/arch/arm/atomic/Makefile.inc common/lib/libc/arch/arm/atomic/Makefile.inc
 --- /export/netbsd/pristine/common/lib/libc/arch/arm/atomic/Makefile.inc	2008-04-21 22:26:12.000000000 +0100
 +++ common/lib/libc/arch/arm/atomic/Makefile.inc	2008-04-21 22:12:51.000000000 +0100
 @@ -12,5 +12,7 @@
  .if defined(LIB) && (${LIB} == "c")

  SRCS+=	atomic_init_testset.c
 +SRCS+=	atomic_cas_up.S
 +CPPFLAGS+= -D__HAVE_ASM_ATOMIC_CAS_UP

  .endif
 diff -ruN -x CVS /export/netbsd/pristine/common/lib/libc/arch/arm/atomic/atomic_cas_up.S common/lib/libc/arch/arm/atomic/atomic_cas_up.S
 --- /export/netbsd/pristine/common/lib/libc/arch/arm/atomic/atomic_cas_up.S	1970-01-01 01:00:00.000000000 +0100
 +++ common/lib/libc/arch/arm/atomic/atomic_cas_up.S	2008-04-21 22:11:57.000000000 +0100
 @@ -0,0 +1,54 @@
 +/*	$NetBSD$	*/
 +
 +/*-
 + * Copyright (c) 2008 The NetBSD Foundation, Inc.
 + * All rights reserved.
 + *
 + * This code is derived from software contributed to The NetBSD Foundation
 + * by Steve C. Woodford.
 + *
 + * Redistribution and use in source and binary forms, with or without
 + * modification, are permitted provided that the following conditions
 + * are met:
 + * 1. Redistributions of source code must retain the above copyright
 + *    notice, this list of conditions and the following disclaimer.
 + * 2. Redistributions in binary form must reproduce the above copyright
 + *    notice, this list of conditions and the following disclaimer in the
 + *    documentation and/or other materials provided with the distribution.
 + * 3. All advertising materials mentioning features or use of this software
 + *    must display the following acknowledgement:
 + *        This product includes software developed by the NetBSD
 + *        Foundation, Inc. and its contributors.
 + * 4. Neither the name of The NetBSD Foundation nor the names of its
 + *    contributors may be used to endorse or promote products derived
 + *    from this software without specific prior written permission.
 + *
 + * THIS SOFTWARE IS PROVIDED BY THE NETBSD FOUNDATION, INC. AND CONTRIBUTORS
 + * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
 + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
 + * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE FOUNDATION OR CONTRIBUTORS
 + * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
 + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
 + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
 + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
 + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
 + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 + * POSSIBILITY OF SUCH DAMAGE.
 + */
 +
 +#include <machine/asm.h>
 +
 +	.globl	_C_LABEL(_atomic_cas_ras_start)
 +	.type	_C_LABEL(_atomic_cas_ras_start),_ASM_TYPE_FUNCTION
 +
 +	.globl	_C_LABEL(_atomic_cas_ras_end)
 +	.type	_C_LABEL(_atomic_cas_ras_end),_ASM_TYPE_FUNCTION
 +
 +ENTRY_NP(_atomic_cas_up)
 +	mov	r3, r0
 +_C_LABEL(_atomic_cas_ras_start):
 +	ldr	r0, [r3]
 +	cmp	r0, r1
 +	streq	r2, [r3]
 +_C_LABEL(_atomic_cas_ras_end):
 +	RET
 diff -ruN -x CVS /export/netbsd/pristine/common/lib/libc/arch/m68k/atomic/Makefile.inc common/lib/libc/arch/m68k/atomic/Makefile.inc
 --- /export/netbsd/pristine/common/lib/libc/arch/m68k/atomic/Makefile.inc	2008-04-21 22:26:12.000000000 +0100
 +++ common/lib/libc/arch/m68k/atomic/Makefile.inc	2008-04-21 22:13:04.000000000 +0100
 @@ -33,6 +33,8 @@
  .else

  SRCS+=	atomic_init_testset.c
 +SRCS+=	atomic_cas_68000.S
 +CPPFLAGS+= -D__HAVE_ASM_ATOMIC_CAS_UP

  .endif
  .endif
 diff -ruN -x CVS /export/netbsd/pristine/common/lib/libc/arch/m68k/atomic/atomic_cas_68000.S common/lib/libc/arch/m68k/atomic/atomic_cas_68000.S
 --- /export/netbsd/pristine/common/lib/libc/arch/m68k/atomic/atomic_cas_68000.S	1970-01-01 01:00:00.000000000 +0100
 +++ common/lib/libc/arch/m68k/atomic/atomic_cas_68000.S	2008-04-21 22:14:12.000000000 +0100
 @@ -0,0 +1,56 @@
 +/*	$NetBSD$	*/
 +
 +/*-
 + * Copyright (c) 2008 The NetBSD Foundation, Inc.
 + * All rights reserved.
 + *
 + * This code is derived from software contributed to The NetBSD Foundation
 + * by Steve C. Woodford.
 + *
 + * Redistribution and use in source and binary forms, with or without
 + * modification, are permitted provided that the following conditions
 + * are met:
 + * 1. Redistributions of source code must retain the above copyright
 + *    notice, this list of conditions and the following disclaimer.
 + * 2. Redistributions in binary form must reproduce the above copyright
 + *    notice, this list of conditions and the following disclaimer in the
 + *    documentation and/or other materials provided with the distribution.
 + * 3. All advertising materials mentioning features or use of this software
 + *    must display the following acknowledgement:
 + *	This product includes software developed by the NetBSD
 + *	Foundation, Inc. and its contributors.
 + * 4. Neither the name of The NetBSD Foundation nor the names of its
 + *    contributors may be used to endorse or promote products derived
 + *    from this software without specific prior written permission.
 + *      
 + * THIS SOFTWARE IS PROVIDED BY THE NETBSD FOUNDATION, INC. AND CONTRIBUTORS
 + * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
 + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
 + * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE FOUNDATION OR CONTRIBUTORS
 + * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
 + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
 + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
 + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
 + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
 + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 + * POSSIBILITY OF SUCH DAMAGE.
 + */
 +
 +#include "atomic_op_asm.h"
 +
 +	.text
 +
 +	.globl	_C_LABEL(_atomic_cas_ras_start)
 +	.globl	_C_LABEL(_atomic_cas_ras_end)
 +
 +ENTRY_NOPROFILE(_atomic_cas_up)
 +	movl	%sp@(4), %a0		/* Fetch ptr */
 +
 +_C_LABEL(_atomic_cas_ras_start):
 +	movl	%a0@, d0		/* d0 = *ptr */
 +	cmp	%sp@(8), d0		/* Same as old? */
 +	jne	1f			/* Nope */
 +	movl	%sp@(12), %a0@		/* *ptr = new */
 +_C_LABEL(_atomic_cas_ras_end):
 +1:	rts
 +
 diff -ruN -x CVS /export/netbsd/pristine/common/lib/libc/atomic/atomic_init_testset.c common/lib/libc/atomic/atomic_init_testset.c
 --- /export/netbsd/pristine/common/lib/libc/atomic/atomic_init_testset.c	2008-04-21 22:26:12.000000000 +0100
 +++ common/lib/libc/atomic/atomic_init_testset.c	2008-04-21 22:13:38.000000000 +0100
 @@ -66,6 +66,9 @@

  RAS_DECL(_atomic_cas);

 +#ifdef	__HAVE_ASM_ATOMIC_CAS_UP
 +extern uint32_t _atomic_cas_up(volatile uint32_t *, uint32_t, uint32_t);
 +#else
  static uint32_t
  _atomic_cas_up(volatile uint32_t *ptr, uint32_t old, uint32_t new)
  {
 @@ -81,6 +84,7 @@

  	return ret;
  }
 +#endif

  static uint32_t
  _atomic_cas_mp(volatile uint32_t *ptr, uint32_t old, uint32_t new)

 --Boundary-00=_tlQDInHx5ZUGTgu--

From: Alan Barrett <apb@cequrux.com>
To: gnats-bugs@NetBSD.org
Cc: netbsd-bugs@NetBSD.org
Subject: Re: lib/38482: C compiler can generate non-restartable code within
	a RAS
Date: Tue, 22 Apr 2008 11:08:29 +0200

 On Mon, 21 Apr 2008, scw@NetBSD.org wrote:
 > I've recently noticed occasional SEGVs with threaded programs on ARM.
 > This turns out to be caused by dereferencing a bad pointer value in the
 > code between the RAS_START and RAS_END in
 > common/lib/libc/atomic/atomic_init_testset.c:_atomic_cas_up().

 Does it help to declare "volatile uint32_t ret;" ?

 --apb (Alan Barrett)

From: Steve Woodford <scw@netbsd.org>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org,
 gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org
Subject: Re: lib/38482: C compiler can generate non-restartable code within  a RAS
Date: Tue, 22 Apr 2008 10:15:24 +0100

 On Tuesday 22 April 2008 10:10:05 Alan Barrett wrote:

 >  Does it help to declare "volatile uint32_t ret;" ?

 It does, on ARM, with the current toolchain. But 1) it generates less 
 efficient code, and 2) the next version of Gcc may well break it again.

 Cheers, Steve

From: Andrew Doran <ad@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: lib/38482: C compiler can generate non-restartable code within a RAS
Date: Tue, 22 Apr 2008 12:08:15 +0100

 On Mon, Apr 21, 2008 at 09:00:01PM +0000, scw@netbsd.org wrote:

 > Rather than fighting the compiler, I propose we implement _atomic_cas_up()
 > in asm code for each affected arch. I can do ARM and m68000.

 Yes please. I can do mips and perhaps alpha. IIRC the assembler should
 recognise the .hidden keyword so none of the symbols in the atomic CAS
 file need to be visible outside libc.

 > In addition, the rasctl(2) manual page should be updated to strongly
 > recommend that RASs are implemented in asm code only. Perhaps we should go
 > so far as removing the RAS_START/RAS_END macros from the API, though that
 > may preclude using in-line asm...

 I also agree with this. C is too hit-and-miss..

 > An audit of the tree for similar code sequences would also be a good idea.
 > For example: pthread_lock.c.

 I think we may already have assembly sequences in libpthread for some
 architectures.

 Thanks,
 Andrew

State-Changed-From-To: open->analyzed
State-Changed-By: scw@NetBSD.org
State-Changed-When: Tue, 29 Apr 2008 22:10:36 +0100
State-Changed-Why:
_atomic_cas_up() is in the process of being implemented in asm code for each
affected arch.


From: Steve Woodford <scw@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/38482 CVS commit: src/lib/libc/sys
Date: Tue, 29 Apr 2008 21:06:28 +0000 (UTC)

 Module Name:	src
 Committed By:	scw
 Date:		Tue Apr 29 21:06:28 UTC 2008

 Modified Files:
 	src/lib/libc/sys: rasctl.2

 Log Message:
 - Document RAS_START_ASM, RAS_END_ASM, RAS_START_ASM_HIDDEN, and
   RAS_END_ASM_HIDDEN.
 - Advise against implementing RASs in C.

 See PR lib/38482 for details.

 While here, remove clauses 3 and 4 from TNF license.


 To generate a diff of this commit:
 cvs rdiff -r1.12 -r1.13 src/lib/libc/sys/rasctl.2

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Andrew Doran <ad@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/38482 CVS commit: src/common/lib/libc/arch/mips/atomic
Date: Wed, 30 Apr 2008 00:17:34 +0000 (UTC)

 Module Name:	src
 Committed By:	ad
 Date:		Wed Apr 30 00:17:34 UTC 2008

 Modified Files:
 	src/common/lib/libc/arch/mips/atomic: Makefile.inc
 Added Files:
 	src/common/lib/libc/arch/mips/atomic: atomic_cas_up.S

 Log Message:
 Assembly _atomic_cas_up() for mips. PR lib/38482.


 To generate a diff of this commit:
 cvs rdiff -r1.6 -r1.7 src/common/lib/libc/arch/mips/atomic/Makefile.inc
 cvs rdiff -r0 -r1.1 src/common/lib/libc/arch/mips/atomic/atomic_cas_up.S

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/38482 CVS commit: src/common/lib/libc/arch/sparc/atomic
Date: Wed, 28 Nov 2012 21:39:59 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Wed Nov 28 21:39:59 UTC 2012

 Modified Files:
 	src/common/lib/libc/arch/sparc/atomic: Makefile.inc
 Added Files:
 	src/common/lib/libc/arch/sparc/atomic: atomic_cas_up.S

 Log Message:
 Provide an assembler version of _atomic_cas_up for sparc - the C code
 does not compile to something usable in a RAS. See PR 38482.


 To generate a diff of this commit:
 cvs rdiff -u -r1.14 -r1.15 src/common/lib/libc/arch/sparc/atomic/Makefile.inc
 cvs rdiff -u -r0 -r1.1 src/common/lib/libc/arch/sparc/atomic/atomic_cas_up.S

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/38482: C compiler can generate non-restartable code within a
 RAS.
Date: Sat, 22 May 2021 03:47:23 +0000

 Though some the commits didn't make it to this PR, it looks like at
 this point only sh3 and vax are missing an atomic_cas_up.S.

 The mips code appears to be missing 16- and 8- bit CAS and be still
 using the C code from atomic_init_testset.c; path of least resistance
 is to use atomic_cas_by_cas32.c for both cases and not just mips64,
 but writing narrow RAS versions is straightforward.

 (also it seems like we ought to support real CAS for multiprocessor
 mips32+ targets, but that's a different issue)

 It would probably be good to get the missing bits written and remove
 the dodgy C-based RAS code.

 -- 
 David A. Holland
 dholland@netbsd.org

>Unformatted:

Home	PR Database Search