Date: Thu, 11 May 1995 16:40:02 -0700 From: Tatu Ylonen <ylo@fx7.cs.hut.fi> To: freebsd-bugs Subject: i386/395: CRITICAL PROBLEM: spl functions implemented incorrectly Message-ID: <199505112340.QAA19464@freefall.cdrom.com> In-Reply-To: Your message of Fri, 12 May 1995 02:41:07 GMT <199505120241.CAA00890@fx7.cs.hut.fi>
next in thread | previous in thread | raw e-mail | index | archive | help
>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:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199505112340.QAA19464>