From owner-freebsd-bugs  Thu May 11 16:40:03 1995
Return-Path: bugs-owner
Received: (from majordom@localhost)
          by freefall.cdrom.com (8.6.10/8.6.6) id QAA19471
          for bugs-outgoing; Thu, 11 May 1995 16:40:03 -0700
Received: (from gnats@localhost)
          by freefall.cdrom.com (8.6.10/8.6.6) id QAA19464
          ; Thu, 11 May 1995 16:40:02 -0700
Date: Thu, 11 May 1995 16:40:02 -0700
Message-Id: <199505112340.QAA19464@freefall.cdrom.com>
From: Tatu Ylonen <ylo@fx7.cs.hut.fi>
Reply-To: Tatu Ylonen <ylo@fx7.cs.hut.fi>
To: freebsd-bugs
Subject: i386/395: CRITICAL PROBLEM: spl functions implemented incorrectly
In-Reply-To: Your message of Fri, 12 May 1995 02:41:07 GMT
	<199505120241.CAA00890@fx7.cs.hut.fi>
Sender: bugs-owner@FreeBSD.org
Precedence: bulk


>Number:         395
>Category:       i386
>Synopsis:       All spl functions implemented incorrectly in both FreeBSD and NetBSD
>Confidential:   no
>Severity:       critical
>Priority:       high
>Responsible:    freebsd-bugs (FreeBSD bugs mailing list)
>State:          open
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Thu May 11 16:40:01 1995
>Originator:     Tatu Ylonen
>Organization:
Helsinki University of Technology, FInland
>Release:        FreeBSD 2.1.0-Development i386
>Environment:

	FreeBSD 2.1.0-Development i386 (Supped May 5, 1995).
	Recent NetBSD has similar problems.

>Description:

	Spl functions (splbio, splclock, splhigh, splimp, splnet,
	splsoftclock, splsofttty, splstatclock, spltty, spl0, splx)
	are defined in /usr/src/sys/i386/include/spl.h as inline
	functions that modify the ordinary variable cpl (extern
	unsigned cpl in the same header).

	This means that the compiler has full knowledge about the
	semantics, side-effects, and dependencies of these functions.
	The compiler inlines these functions, and can mix (reorder) the
	resulting code with other code in the function that calls the
	spl function.
	  - The value may not get actually written to cpl until the
	    end of the function calling the spl function, or when a
	    function with unknown dependencies is called.  This may be
	    *after* the protected code is executed, or the compiler
	    might even entirely optimize the code out because splx
	    assigns the value later in the function.
	  - It would not help to simply make cpl volatile, because the
	    compiler would still be allowed to cache intermediate
	    values or results of common subexpressions in registers
	    across the lock/unlock requests.

	A simple solution is to make the spl functions real functions
	stored in a separate file, and only have prototypes in spl.h.
	This way, the compiler does not know anything about the
	function, and will have to assume the worst: it might access
	or modify any memory, which means the compiler will have to
	flush any modifications to memory before the call, and not
	make any assumptions about common subexpressions being
	preserved across the call.  There is a slight performance
	penalty due to the real call, but it is not significant and
	not easy to avoid reliably.

	Also, be aware that if the OS is ever to be ported to a
	multiprocessor, the functions will need to be rewritten.  The
	instructions they now use are not atomic in a multiprocessor
	environment.

>How-To-Repeat:

	The real effects of this problem are not predictable or
	deterministic.  They may depend on compiler version or
	optimization levels.  General unreliability, mysterious
	problems, and random panics are all likely.  I started to
	wonder about this because sysv message queues were losing
	messages (that's another story - I'll write about it
	later).  This could also explain inode lockups problems I
	had with an earlier release.

	Actually, I am surprised the kernel works at all, even fairly
	well.

>Fix:
	
	Move all spl functions (splbio, splclock, splhigh, splimp,
	splnet, splsoftclock, splsofttty, splstatclock, spltty, spl0,
	splx) away from spl.h, make them real (global) functions,
	and leave only prototypes in spl.h.  It might not be a bad
	idea to make cpl volatile to be sure; I am not sure if it
	is necessary.

	My quick fix is as follows; someone else can do it more
	cleanly (I think the spl code could be in a separate .c file,
	but I simply stored it in a random .c file).

	i386/include/spl.h:
	  - Move the GENSPL macro and its uses, and spl0 and splx to
	    i386/386/sys_machdep.c.  Edit the macro and functions to
	    make them normal non-static, non-inline functions.
	  - Add the following prototypes:
		int splbio(void);
		int splclock(void);
		int splhigh(void);
		int splimp(void);
		int splnet(void);
		int splsoftclock(void);
		int splsofttty(void);
		int splstatclock(void);
		int spltty(void);
		void spl0(void);
		void splx(int old);

	i386/i386/sys_machdep.c:
	  - I moved the spl code here.  I think it would be cleaner to
	    have a separate .c file for it.
>Audit-Trail:
>Unformatted: