| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 3 Dec 2014 16:07:49 -0800
From: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To: Christian Borntraeger <borntraeger@...ibm.com>
Cc: linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
torvalds@...ux-foundation.org
Subject: Re: [PATCH 1/9] kernel: Provide READ_ONCE and ASSIGN_ONCE
On Wed, Dec 03, 2014 at 11:30:13PM +0100, Christian Borntraeger wrote:
> ACCESS_ONCE does not work reliably on non-scalar types. For
> example gcc 4.6 and 4.7 might remove the volatile tag for such
> accesses during the SRA (scalar replacement of aggregates) step
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145)
>
> Let's provide READ_ONCE/ASSIGN_ONCE that will do all accesses via
> scalar types as suggested by Linus Torvalds. Accesses larger than
> the machines word size cannot be guaranteed to be atomic. These
> macros will use memcpy and emit a build warning.
>
> Signed-off-by: Christian Borntraeger <borntraeger@...ibm.com>
With or without some nits below addressed:
Reviewed-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
> ---
> include/linux/compiler.h | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 64 insertions(+)
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index d5ad7b1..947710e 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -186,6 +186,70 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
> # define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __LINE__)
> #endif
>
> +#include <linux/types.h>
> +
> +void data_access_exceeds_word_size(void)
> +__compiletime_warning("data access exceeds word size and won't be atomic");
> +
> +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
You could get rid of the #ifdef above by doing "case sizeof(long)"
and switching the casts from u64 to unsigned long.
> + default:
> + barrier();
> + __builtin_memcpy((void *)res, (const void *)p, size);
> + data_access_exceeds_word_size();
> + barrier();
> + }
> +}
> +
> +static __always_inline void __assign_once_size(volatile void *p, void *res, int size)
> +{
> + switch (size) {
> + case 1: *(volatile u8 *)p = *(u8 *)res; break;
> + case 2: *(volatile u16 *)p = *(u16 *)res; break;
> + case 4: *(volatile u32 *)p = *(u32 *)res; break;
> +#ifdef CONFIG_64BIT
> + case 8: *(volatile u64 *)p = *(u64 *)res; break;
> +#endif
Ditto.
> + default:
> + barrier();
> + __builtin_memcpy((void *)p, (const void *)res, size);
> + data_access_exceeds_word_size();
> + barrier();
> + }
> +}
> +
> +/*
> + * Prevent the compiler from merging or refetching reads or writes. The compiler
> + * is also forbidden from reordering successive instances of READ_ONCE,
> + * ASSIGN_ONCE and ACCESS_ONCE (see below), but only when the compiler is aware
> + * of some particular ordering. One way to make the compiler aware of ordering
> + * is to put the two invocations of READ_ONCE, ASSIGN_ONCE or ACCESS_ONCE() in
> + * different C statements.
> + *
> + * In contrast to ACCESS_ONCE these two macros will also work on aggregate data
> + * types like structs or unions. If the size of the accessed data type exceeds
> + * the word size of the machine (e.g. 32bit or 64bit), the access might happen
> + * in multiple chunks, though.
This last sentence might be more clear if it was something like the
following:
If the size of the accessed data type exceeeds the word size of
the machine (e.g., 32 bits or 64 bits), ACCESS_ONCE() might
carry out the access in multiple chunks, but READ_ONCE() and
ASSIGN_ONCE() will give a link-time error.
> + *
> + * These macros do absolutely -nothing- to prevent the CPU from reordering,
> + * merging, or refetching absolutely anything at any time. Their main intended
> + * use is to mediate communication between process-level code and irq/NMI
> + * handlers, all running on the same CPU.
This last sentence is now obsolete. How about something like this?
Their two major use cases are: (1) Mediating communication
between process-level code and irq/NMI handlers, all running
on the same CPU, and (2) Ensuring that the compiler does not
fold, spindle, or otherwise mutilate accesses that either do
not require ordering or that interact with an explicit memory
barrier or atomic instruction that provides the required ordering.
> + */
> +
> +#define READ_ONCE(p) \
> + ({ typeof(p) __val; __read_once_size(&p, &__val, sizeof(__val)); __val; })
> +
> +#define ASSIGN_ONCE(val, p) \
> + ({ typeof(p) __val; __val = val; __assign_once_size(&p, &__val, sizeof(__val)); __val; })
> +
> #endif /* __KERNEL__ */
>
> #endif /* __ASSEMBLY__ */
> --
> 1.9.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists