ACCESS_ONCE() and compiler bugs

The ACCESS_ONCE() macro is used throughout the kernel to ensure that code generated by the compiler will access the indicated variable once (and only once); see this article for details on how it works and when its use is necessary. When that article was written (2012), there were 200 invocations of ACCESS_ONCE() in the kernel; now there are over 700 of them. Like many low-level techniques for concurrency management, ACCESS_ONCE() relies on trickery that is best hidden from view. And, like such techniques, it may break if the compiler changes behavior or, as has been seen recently, contains a bug.

Back in November, Christian Borntraeger posted a message regarding the interactions between ACCESS_ONCE() and an obscure GCC bug. To understand the problem, it is worth looking at the macro, which is defined simply in current kernels (in <linux/compiler.h>):

    #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))

In short, ACCESS_ONCE() forces the variable to be treated as being a volatile type, even though it (like almost all variables in the kernel) is not declared that way. The problem reported by Christian is that GCC 4.6 and 4.7 will drop the volatile modifier if the variable passed into it is not of a scalar type. It works fine if x is an int, for example, but not if x has a more complicated type. For example, ACCESS_ONCE() is often used with page table entries, which are defined as having the pte_t type:

    typedef struct {
	unsigned long pte;
    } pte_t;

In this case, the volatile semantics will be lost in buggy compilers, leading to buggy kernels. Christian started by looking for ways to work around the problem, only to be informed that normal kernel practice is to avoid working around compiler bugs whenever possible; instead, the buggy versions should simply be blacklisted in the kernel build system. But 4.6 and 4.7 are installed on a lot of systems; blacklisting them would inconvenience many users. And, as Linus put it, there can be reasons for approaches other than blacklisting:

One way of being less fragile would be to change the affected ACCESS_ONCE() calls to point to the scalar parts of the relevant non-scalar types. So, if code does something like:

    pte_t p = ACCESS_ONCE(pte);

It could be changed to something like:

    unsigned long p = ACCESS_ONCE(pte->pte);

This type of change requires auditing all ACCESS_ONCE() calls, though, to find the ones using non-scalar types; that would be a lengthy and error-prone process that would not prevent the addition of new bugs in the future.

Another approach to the problem explored by Christian was to remove a number of problematic ACCESS_ONCE() calls and just put in a compiler barrier with barrier() instead. In many cases, a barrier is sufficient, but in others it is not. Once again, a detailed audit is required, and there is nothing preventing new code from adding buggy ACCESS_ONCE() calls.

So Christian headed down the path of changing ACCESS_ONCE() to simply disallow the use of non-scalar types altogether. In the most recent version of the patch set, ACCESS_ONCE() looks like this:

    #define __ACCESS_ONCE(x) ({ \
	       __maybe_unused typeof(x) __var = 0; \
	       (volatile typeof(x) *)&(x); })
    #define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))

This version will cause compilation failures if a non-scalar type is passed into the macro. But what about the situations where a non-scalar type needs to be used? For these cases, Christian has introduced two new macros, READ_ONCE() and ASSIGN_ONCE(). The definition of the former looks like this:

    static __always_inline void __read_once_size(volatile void *p, void *res, int size)
    {
    	switch (size) {
    	case 1: *(u8 *)res = *(volatile u8 *)p; break;
    	case 2: *(u16 *)res = *(volatile u16 *)p; break;
    	case 4: *(u32 *)res = *(volatile u32 *)p; break;
    #ifdef CONFIG_64BIT
    	case 8: *(u64 *)res = *(volatile u64 *)p; break;
    #endif
        }
    }
    
    #define READ_ONCE(p) \
          ({ typeof(p) __val; __read_once_size(&p, &__val, sizeof(__val)); __val; })

Essentially, it works by forcing the use of scalar types, even if the variable passed in does not have such a type. Providing a single access macro that worked on both the left-hand and right-hand sides of an assignment turned out to not be trivial, so the separate ASSIGN_ONCE() was provided for the left-hand side case.

Christian's patch set replaces ACCESS_ONCE() calls with READ_ONCE() or ASSIGN_ONCE() in cases where the latter are needed. Comments in the code suggest that those macros should be preferred to ACCESS_ONCE() in the future, but most existing ACCESS_ONCE() calls have not been changed. Developers using ACCESS_ONCE() to access non-scalar types in the future will get an unpleasant surprise from the compiler, though.

This version of the patch has received few comments and seems likely to make it into the mainline in the near future; backports to the stable series are also probably on the agenda. There are times when it is best to simply avoid versions of the compiler with known bugs altogether. But, as can be seen here, compiler bugs can also be seen as a signal that things could be done better in the kernel, leading to more robust code overall.

Index entries for this article
Kernel	ACCESS_ONCE()

---

to post comments

#define ACCESS_ONCE(x)                                             \
   __builtin_choose_expr(sizeof(x) == 1, *(volatile u8 *)&x,       \
        __builtin_choose_expr(sizeof(x) == 2, *(volatile u16 *)&x, \
        __builtin_choose_expr(sizeof(x) == 4, *(volatile u32 *)&x, \
        __builtin_choose_expr(sizeof(x) == 8, *(volatile u64 *)&x, \
        (void)0))))