From: Arnd Bergmann <arnd@arndb.de> To: Linus Torvalds <torvalds@linux-foundation.org> Cc: Will Deacon <will@kernel.org>, Peter Zijlstra <peterz@infradead.org>, Michael Ellerman <mpe@ellerman.id.au>, Daniel Axtens <dja@axtens.net>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, linuxppc-dev <linuxppc-dev@lists.ozlabs.org>, Christophe Leroy <christophe.leroy@c-s.fr>, linux-arch <linux-arch@vger.kernel.org>, Mark Rutland <mark.rutland@arm.com>, Segher Boessenkool <segher@kernel.crashing.org>, Christian Borntraeger <borntraeger@de.ibm.com> Subject: Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops)) Date: Fri, 13 Dec 2019 14:17:08 +0100 [thread overview] Message-ID: <CAK8P3a2QYpT_u3D7c_w+hoyeO-Stkj5MWyU_LgGOqnMtKLEudg@mail.gmail.com> (raw) In-Reply-To: <CAHk-=wiMuHmWzQ7-CRQB6o+SHtA-u-Rp6VZwPcqDbjAaug80rQ@mail.gmail.com> On Thu, Dec 12, 2019 at 9:50 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Thu, Dec 12, 2019 at 11:34 AM Will Deacon <will@kernel.org> wrote: > > The root of my concern in all of this, and what started me looking at it in > > the first place, is the interaction with 'typeof()'. Inheriting 'volatile' > > for a pointer means that local variables in macros declared using typeof() > > suddenly start generating *hideous* code, particularly when pointless stack > > spills get stackprotector all excited. > > Yeah, removing volatile can be a bit annoying. > > For the particular case of the bitops, though, it's not an issue. > Since you know the type there, you can just cast it. > > And if we had the rule that READ_ONCE() was an arithmetic type, you could do > > typeof(0+(*p)) __var; > > since you might as well get the integer promotion anyway (on the > non-volatile result). > > But that doesn't work with structures or unions, of course. > > I'm not entirely sure we have READ_ONCE() with a struct. I do know we > have it with 64-bit entities on 32-bit machines, but that's ok with > the "0+" trick. I'll have my randconfig builder look for instances, so far I found one, see below. My feeling is that it would be better to enforce at least the size being a 1/2/4/8, to avoid cases where someone thinks the access is atomic, but it falls back on a memcpy. Arnd diff --git a/drivers/xen/time.c b/drivers/xen/time.c index 0968859c29d0..adb492c0aa34 100644 --- a/drivers/xen/time.c +++ b/drivers/xen/time.c @@ -64,7 +64,7 @@ static void xen_get_runstate_snapshot_cpu_delta( do { state_time = get64(&state->state_entry_time); rmb(); /* Hypervisor might update data. */ - *res = READ_ONCE(*state); + memcpy(res, state, sizeof(*res)); rmb(); /* Hypervisor might update data. */ } while (get64(&state->state_entry_time) != state_time || (state_time & XEN_RUNSTATE_UPDATE)); diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 5e88e7e33abe..f4ae360efdba 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -179,6 +179,8 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, #include <uapi/linux/types.h> +extern void __broken_access_once(void *, const void *, unsigned long); + #define __READ_ONCE_SIZE \ ({ \ switch (size) { \ @@ -187,9 +189,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, case 4: *(__u32 *)res = *(volatile __u32 *)p; break; \ case 8: *(__u64 *)res = *(volatile __u64 *)p; break; \ default: \ - barrier(); \ - __builtin_memcpy((void *)res, (const void *)p, size); \ - barrier(); \ + __broken_access_once((void *)res, (const void *)p, size); \ } \ }) @@ -225,9 +225,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s case 4: *(volatile __u32 *)p = *(__u32 *)res; break; case 8: *(volatile __u64 *)p = *(__u64 *)res; break; default: - barrier(); - __builtin_memcpy((void *)p, (const void *)res, size); - barrier(); + __broken_access_once((void *)p, (const void *)res, size); } }
WARNING: multiple messages have this Message-ID (diff)
From: Arnd Bergmann <arnd@arndb.de> To: Linus Torvalds <torvalds@linux-foundation.org> Cc: linux-arch <linux-arch@vger.kernel.org>, Will Deacon <will@kernel.org>, Peter Zijlstra <peterz@infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Christian Borntraeger <borntraeger@de.ibm.com>, Mark Rutland <mark.rutland@arm.com>, linuxppc-dev <linuxppc-dev@lists.ozlabs.org>, Daniel Axtens <dja@axtens.net> Subject: Re: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops)) Date: Fri, 13 Dec 2019 14:17:08 +0100 [thread overview] Message-ID: <CAK8P3a2QYpT_u3D7c_w+hoyeO-Stkj5MWyU_LgGOqnMtKLEudg@mail.gmail.com> (raw) In-Reply-To: <CAHk-=wiMuHmWzQ7-CRQB6o+SHtA-u-Rp6VZwPcqDbjAaug80rQ@mail.gmail.com> On Thu, Dec 12, 2019 at 9:50 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Thu, Dec 12, 2019 at 11:34 AM Will Deacon <will@kernel.org> wrote: > > The root of my concern in all of this, and what started me looking at it in > > the first place, is the interaction with 'typeof()'. Inheriting 'volatile' > > for a pointer means that local variables in macros declared using typeof() > > suddenly start generating *hideous* code, particularly when pointless stack > > spills get stackprotector all excited. > > Yeah, removing volatile can be a bit annoying. > > For the particular case of the bitops, though, it's not an issue. > Since you know the type there, you can just cast it. > > And if we had the rule that READ_ONCE() was an arithmetic type, you could do > > typeof(0+(*p)) __var; > > since you might as well get the integer promotion anyway (on the > non-volatile result). > > But that doesn't work with structures or unions, of course. > > I'm not entirely sure we have READ_ONCE() with a struct. I do know we > have it with 64-bit entities on 32-bit machines, but that's ok with > the "0+" trick. I'll have my randconfig builder look for instances, so far I found one, see below. My feeling is that it would be better to enforce at least the size being a 1/2/4/8, to avoid cases where someone thinks the access is atomic, but it falls back on a memcpy. Arnd diff --git a/drivers/xen/time.c b/drivers/xen/time.c index 0968859c29d0..adb492c0aa34 100644 --- a/drivers/xen/time.c +++ b/drivers/xen/time.c @@ -64,7 +64,7 @@ static void xen_get_runstate_snapshot_cpu_delta( do { state_time = get64(&state->state_entry_time); rmb(); /* Hypervisor might update data. */ - *res = READ_ONCE(*state); + memcpy(res, state, sizeof(*res)); rmb(); /* Hypervisor might update data. */ } while (get64(&state->state_entry_time) != state_time || (state_time & XEN_RUNSTATE_UPDATE)); diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 5e88e7e33abe..f4ae360efdba 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -179,6 +179,8 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, #include <uapi/linux/types.h> +extern void __broken_access_once(void *, const void *, unsigned long); + #define __READ_ONCE_SIZE \ ({ \ switch (size) { \ @@ -187,9 +189,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, case 4: *(__u32 *)res = *(volatile __u32 *)p; break; \ case 8: *(__u64 *)res = *(volatile __u64 *)p; break; \ default: \ - barrier(); \ - __builtin_memcpy((void *)res, (const void *)p, size); \ - barrier(); \ + __broken_access_once((void *)res, (const void *)p, size); \ } \ }) @@ -225,9 +225,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s case 4: *(volatile __u32 *)p = *(__u32 *)res; break; case 8: *(volatile __u64 *)p = *(__u64 *)res; break; default: - barrier(); - __builtin_memcpy((void *)p, (const void *)res, size); - barrier(); + __broken_access_once((void *)p, (const void *)res, size); } }
next prev parent reply other threads:[~2019-12-13 20:37 UTC|newest] Thread overview: 86+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-12-06 12:46 [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops) Michael Ellerman 2019-12-06 12:46 ` Michael Ellerman 2019-12-06 13:16 ` Peter Zijlstra 2019-12-06 13:16 ` Peter Zijlstra 2019-12-10 5:38 ` Michael Ellerman 2019-12-10 5:38 ` Michael Ellerman 2019-12-10 10:15 ` Peter Zijlstra 2019-12-10 10:15 ` Peter Zijlstra 2019-12-11 0:29 ` Michael Ellerman 2019-12-11 0:29 ` Michael Ellerman 2019-12-12 5:42 ` READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops)) Michael Ellerman 2019-12-12 5:42 ` Michael Ellerman 2019-12-12 8:01 ` Peter Zijlstra 2019-12-12 8:01 ` Peter Zijlstra 2019-12-12 10:07 ` Will Deacon 2019-12-12 10:07 ` Will Deacon 2019-12-12 10:46 ` Peter Zijlstra 2019-12-12 10:46 ` Peter Zijlstra 2019-12-12 17:04 ` Will Deacon 2019-12-12 17:04 ` Will Deacon 2019-12-12 17:16 ` Will Deacon 2019-12-12 17:16 ` Will Deacon 2019-12-12 17:41 ` Linus Torvalds 2019-12-12 17:41 ` Linus Torvalds 2019-12-12 17:50 ` Segher Boessenkool 2019-12-12 17:50 ` Segher Boessenkool 2019-12-12 18:06 ` Will Deacon 2019-12-12 18:06 ` Will Deacon 2019-12-12 18:29 ` Christian Borntraeger 2019-12-12 18:29 ` Christian Borntraeger 2019-12-12 18:43 ` Linus Torvalds 2019-12-12 18:43 ` Linus Torvalds 2019-12-12 19:34 ` Will Deacon 2019-12-12 19:34 ` Will Deacon 2019-12-12 20:21 ` Peter Zijlstra 2019-12-12 20:21 ` Peter Zijlstra 2019-12-12 20:53 ` Peter Zijlstra 2019-12-12 20:53 ` Peter Zijlstra 2019-12-13 10:47 ` Luc Van Oostenryck 2019-12-13 10:47 ` Luc Van Oostenryck 2019-12-13 12:56 ` Peter Zijlstra 2019-12-13 12:56 ` Peter Zijlstra 2019-12-13 14:28 ` Luc Van Oostenryck 2019-12-13 14:28 ` Luc Van Oostenryck 2019-12-12 20:49 ` Linus Torvalds 2019-12-12 20:49 ` Linus Torvalds 2019-12-13 13:17 ` Arnd Bergmann [this message] 2019-12-13 13:17 ` Arnd Bergmann 2019-12-13 21:32 ` Arnd Bergmann 2019-12-13 21:32 ` Arnd Bergmann 2019-12-13 22:01 ` Linus Torvalds 2019-12-13 22:01 ` Linus Torvalds 2019-12-16 10:28 ` Will Deacon 2019-12-16 10:28 ` Will Deacon 2019-12-16 11:47 ` Peter Zijlstra 2019-12-16 11:47 ` Peter Zijlstra 2019-12-16 12:06 ` Arnd Bergmann 2019-12-16 12:06 ` Arnd Bergmann 2019-12-17 17:07 ` Will Deacon 2019-12-17 17:07 ` Will Deacon 2019-12-17 18:04 ` Linus Torvalds 2019-12-17 18:04 ` Linus Torvalds 2019-12-17 18:05 ` Linus Torvalds 2019-12-17 18:05 ` Linus Torvalds 2019-12-17 18:31 ` Will Deacon 2019-12-17 18:31 ` Will Deacon 2019-12-17 18:32 ` Linus Torvalds 2019-12-17 18:32 ` Linus Torvalds 2019-12-18 12:17 ` Michael Ellerman 2019-12-18 12:17 ` Michael Ellerman 2019-12-19 12:11 ` Will Deacon 2019-12-19 12:11 ` Will Deacon 2019-12-18 10:22 ` Christian Borntraeger 2019-12-18 10:22 ` Christian Borntraeger 2019-12-18 10:35 ` Will Deacon 2019-12-18 10:35 ` Will Deacon 2019-12-13 12:07 ` Michael Ellerman 2019-12-13 12:07 ` Michael Ellerman 2019-12-13 13:53 ` Segher Boessenkool 2019-12-13 13:53 ` Segher Boessenkool 2019-12-13 21:06 ` Michael Ellerman 2019-12-13 21:06 ` Michael Ellerman 2019-12-12 15:10 ` Segher Boessenkool 2019-12-12 15:10 ` Segher Boessenkool 2019-12-06 22:15 ` [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops) pr-tracker-bot 2019-12-06 22:15 ` pr-tracker-bot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAK8P3a2QYpT_u3D7c_w+hoyeO-Stkj5MWyU_LgGOqnMtKLEudg@mail.gmail.com \ --to=arnd@arndb.de \ --cc=borntraeger@de.ibm.com \ --cc=christophe.leroy@c-s.fr \ --cc=dja@axtens.net \ --cc=linux-arch@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mark.rutland@arm.com \ --cc=mpe@ellerman.id.au \ --cc=peterz@infradead.org \ --cc=segher@kernel.crashing.org \ --cc=torvalds@linux-foundation.org \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.