Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1600848 > unrolled thread
| Started by | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| First post | 2017-03-14 20:30 +0100 |
| Last post | 2017-03-24 13:50 +0100 |
| Articles | 14 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH 0/3] x86, kasan: add KASAN checks to atomic operations Dmitry Vyukov <dvyukov@google.com> - 2017-03-14 20:30 +0100
Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Mark Rutland <mark.rutland@arm.com> - 2017-03-20 18:20 +0100
Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Mark Rutland <mark.rutland@arm.com> - 2017-03-21 11:50 +0100
Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Dmitry Vyukov <dvyukov@google.com> - 2017-03-21 19:10 +0100
Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Arnd Bergmann <arnd@arndb.de> - 2017-03-21 22:30 +0100
Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Dmitry Vyukov <dvyukov@google.com> - 2017-03-22 11:50 +0100
Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Arnd Bergmann <arnd@arndb.de> - 2017-03-22 12:40 +0100
Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Dmitry Vyukov <dvyukov@google.com> - 2017-03-22 13:30 +0100
Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Arnd Bergmann <arnd@arndb.de> - 2017-03-22 13:50 +0100
Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Ingo Molnar <mingo@kernel.org> - 2017-03-24 08:00 +0100
Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Dmitry Vyukov <dvyukov@google.com> - 2017-03-24 08:20 +0100
Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Dmitry Vyukov <dvyukov@google.com> - 2017-03-24 09:50 +0100
Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Ingo Molnar <mingo@kernel.org> - 2017-03-24 12:00 +0100
Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations Dmitry Vyukov <dvyukov@google.com> - 2017-03-24 13:50 +0100
| From | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| Date | 2017-03-14 20:30 +0100 |
| Subject | [PATCH 0/3] x86, kasan: add KASAN checks to atomic operations |
| Message-ID | <tl0iR-4M7-3@gated-at.bofh.it> |
KASAN uses compiler instrumentation to intercept all memory accesses. But it does not see memory accesses done in assembly code. One notable user of assembly code is atomic operations. Frequently, for example, an atomic reference decrement is the last access to an object and a good candidate for a racy use-after-free. Atomic operations are defined in arch files, but KASAN instrumentation is required for several archs that support KASAN. Later we will need similar hooks for KMSAN (uninit use detector) and KTSAN (data race detector). This change introduces wrappers around atomic operations that can be used to add KASAN/KMSAN/KTSAN instrumentation across several archs, and adds KASAN checks to them. This patch uses the wrappers only for x86 arch. Arm64 will be switched later. And we also plan to instrument bitops in a similar way. Within a day it has found its first bug: BUG: KASAN: use-after-free in atomic_dec_and_test arch/x86/include/asm/atomic.h:123 [inline] at addr ffff880079c30158 Write of size 4 by task syz-executor6/25698 CPU: 2 PID: 25698 Comm: syz-executor6 Not tainted 4.10.0+ #302 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 Call Trace: kasan_check_write+0x14/0x20 mm/kasan/kasan.c:344 atomic_dec_and_test arch/x86/include/asm/atomic.h:123 [inline] put_task_struct include/linux/sched/task.h:93 [inline] put_ctx+0xcf/0x110 kernel/events/core.c:1131 perf_event_release_kernel+0x3ad/0xc90 kernel/events/core.c:4322 perf_release+0x37/0x50 kernel/events/core.c:4338 __fput+0x332/0x800 fs/file_table.c:209 ____fput+0x15/0x20 fs/file_table.c:245 task_work_run+0x197/0x260 kernel/task_work.c:116 exit_task_work include/linux/task_work.h:21 [inline] do_exit+0xb38/0x29c0 kernel/exit.c:880 do_group_exit+0x149/0x420 kernel/exit.c:984 get_signal+0x7e0/0x1820 kernel/signal.c:2318 do_signal+0xd2/0x2190 arch/x86/kernel/signal.c:808 exit_to_usermode_loop+0x200/0x2a0 arch/x86/entry/common.c:157 syscall_return_slowpath arch/x86/entry/common.c:191 [inline] do_syscall_64+0x6fc/0x930 arch/x86/entry/common.c:286 entry_SYSCALL64_slow_path+0x25/0x25 RIP: 0033:0x4458d9 RSP: 002b:00007f3f07187cf8 EFLAGS: 00000246 ORIG_RAX: 00000000000000ca RAX: fffffffffffffe00 RBX: 00000000007080c8 RCX: 00000000004458d9 RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00000000007080c8 RBP: 00000000007080a8 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000 R13: 0000000000000000 R14: 00007f3f071889c0 R15: 00007f3f07188700 Object at ffff880079c30140, in cache task_struct size: 5376 Allocated: PID = 25681 kmem_cache_alloc_node+0x122/0x6f0 mm/slab.c:3662 alloc_task_struct_node kernel/fork.c:153 [inline] dup_task_struct kernel/fork.c:495 [inline] copy_process.part.38+0x19c8/0x4aa0 kernel/fork.c:1560 copy_process kernel/fork.c:1531 [inline] _do_fork+0x200/0x1010 kernel/fork.c:1994 SYSC_clone kernel/fork.c:2104 [inline] SyS_clone+0x37/0x50 kernel/fork.c:2098 do_syscall_64+0x2e8/0x930 arch/x86/entry/common.c:281 return_from_SYSCALL_64+0x0/0x7a Freed: PID = 25681 __cache_free mm/slab.c:3514 [inline] kmem_cache_free+0x71/0x240 mm/slab.c:3774 free_task_struct kernel/fork.c:158 [inline] free_task+0x151/0x1d0 kernel/fork.c:370 copy_process.part.38+0x18e5/0x4aa0 kernel/fork.c:1931 copy_process kernel/fork.c:1531 [inline] _do_fork+0x200/0x1010 kernel/fork.c:1994 SYSC_clone kernel/fork.c:2104 [inline] SyS_clone+0x37/0x50 kernel/fork.c:2098 do_syscall_64+0x2e8/0x930 arch/x86/entry/common.c:281 return_from_SYSCALL_64+0x0/0x7a Dmitry Vyukov (3): kasan: allow kasan_check_read/write() to accept pointers to volatiles asm-generic, x86: wrap atomic operations asm-generic: add KASAN instrumentation to atomic operations arch/x86/include/asm/atomic.h | 100 ++++++------ arch/x86/include/asm/atomic64_32.h | 86 ++++++----- arch/x86/include/asm/atomic64_64.h | 90 +++++------ arch/x86/include/asm/cmpxchg.h | 12 +- arch/x86/include/asm/cmpxchg_32.h | 8 +- arch/x86/include/asm/cmpxchg_64.h | 4 +- include/asm-generic/atomic-instrumented.h | 246 ++++++++++++++++++++++++++++++ include/linux/kasan-checks.h | 10 +- mm/kasan/kasan.c | 4 +- 9 files changed, 411 insertions(+), 149 deletions(-) create mode 100644 include/asm-generic/atomic-instrumented.h -- 2.12.0.367.g23dc2f6d3c-goog
[toc] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2017-03-20 18:20 +0100 |
| Subject | Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations |
| Message-ID | <tn98l-7Ut-9@gated-at.bofh.it> |
| In reply to | #1600848 |
Hi,
On Tue, Mar 14, 2017 at 08:24:13PM +0100, Dmitry Vyukov wrote:
> /**
> - * atomic_read - read atomic variable
> + * arch_atomic_read - read atomic variable
> * @v: pointer of type atomic_t
> *
> * Atomically reads the value of @v.
> */
> -static __always_inline int atomic_read(const atomic_t *v)
> +static __always_inline int arch_atomic_read(const atomic_t *v)
> {
> - return READ_ONCE((v)->counter);
> + /*
> + * We use READ_ONCE_NOCHECK() because atomic_read() contains KASAN
> + * instrumentation. Double instrumentation is unnecessary.
> + */
> + return READ_ONCE_NOCHECK((v)->counter);
> }
Just to check, we do this to avoid duplicate reports, right?
If so, double instrumentation isn't solely "unnecessary"; it has a
functional difference, and we should explicitly describe that in the
comment.
... or are duplicate reports supressed somehow?
[...]
> +static __always_inline void arch_atomic_set(atomic_t *v, int i)
> {
> + /*
> + * We could use WRITE_ONCE_NOCHECK() if it exists, similar to
> + * READ_ONCE_NOCHECK() in arch_atomic_read(). But there is no such
> + * thing at the moment, and introducing it for this case does not
> + * worth it.
> + */
> WRITE_ONCE(v->counter, i);
> }
If we are trying to avoid duplicate reports, we should do the same here.
[...]
> +static __always_inline short int atomic_inc_short(short int *v)
> +{
> + return arch_atomic_inc_short(v);
> +}
This is x86-specific, and AFAICT, not used anywhere.
Given that it is arch-specific, I don't think it should be instrumented
here. If it isn't used, we could get rid of it entirely...
Thanks,
Mark.
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2017-03-21 11:50 +0100 |
| Subject | Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations |
| Message-ID | <tnpwu-28I-7@gated-at.bofh.it> |
| In reply to | #1604819 |
On Tue, Mar 21, 2017 at 12:25:06PM +0300, Andrey Ryabinin wrote:
> On 03/20/2017 08:17 PM, Mark Rutland wrote:
> > Hi,
> >
> > On Tue, Mar 14, 2017 at 08:24:13PM +0100, Dmitry Vyukov wrote:
> >> /**
> >> - * atomic_read - read atomic variable
> >> + * arch_atomic_read - read atomic variable
> >> * @v: pointer of type atomic_t
> >> *
> >> * Atomically reads the value of @v.
> >> */
> >> -static __always_inline int atomic_read(const atomic_t *v)
> >> +static __always_inline int arch_atomic_read(const atomic_t *v)
> >> {
> >> - return READ_ONCE((v)->counter);
> >> + /*
> >> + * We use READ_ONCE_NOCHECK() because atomic_read() contains KASAN
> >> + * instrumentation. Double instrumentation is unnecessary.
> >> + */
> >> + return READ_ONCE_NOCHECK((v)->counter);
> >> }
> >
> > Just to check, we do this to avoid duplicate reports, right?
> >
> > If so, double instrumentation isn't solely "unnecessary"; it has a
> > functional difference, and we should explicitly describe that in the
> > comment.
> >
> > ... or are duplicate reports supressed somehow?
>
> They are not suppressed yet. But I think we should just switch kasan
> to single shot mode, i.e. report only the first error. Single bug
> quite often has multiple invalid memory accesses causing storm in
> dmesg. Also write OOB might corrupt metadata so the next report will
> print bogus alloc/free stacktraces.
> In most cases we need to look only at the first report, so reporting
> anything after the first is just counterproductive.
FWIW, that sounds sane to me.
Given that, I agree with your comment regarding READ_ONCE{,_NOCHECK}().
If anyone really wants all the reports, we could have a boot-time option
to do that.
Thanks,
Mark.
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| Date | 2017-03-21 19:10 +0100 |
| Subject | Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations |
| Message-ID | <tnwoi-6UG-11@gated-at.bofh.it> |
| In reply to | #1605498 |
On Tue, Mar 21, 2017 at 11:41 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Mar 21, 2017 at 12:25:06PM +0300, Andrey Ryabinin wrote:
>> On 03/20/2017 08:17 PM, Mark Rutland wrote:
>> > Hi,
>> >
>> > On Tue, Mar 14, 2017 at 08:24:13PM +0100, Dmitry Vyukov wrote:
>> >> /**
>> >> - * atomic_read - read atomic variable
>> >> + * arch_atomic_read - read atomic variable
>> >> * @v: pointer of type atomic_t
>> >> *
>> >> * Atomically reads the value of @v.
>> >> */
>> >> -static __always_inline int atomic_read(const atomic_t *v)
>> >> +static __always_inline int arch_atomic_read(const atomic_t *v)
>> >> {
>> >> - return READ_ONCE((v)->counter);
>> >> + /*
>> >> + * We use READ_ONCE_NOCHECK() because atomic_read() contains KASAN
>> >> + * instrumentation. Double instrumentation is unnecessary.
>> >> + */
>> >> + return READ_ONCE_NOCHECK((v)->counter);
>> >> }
>> >
>> > Just to check, we do this to avoid duplicate reports, right?
>> >
>> > If so, double instrumentation isn't solely "unnecessary"; it has a
>> > functional difference, and we should explicitly describe that in the
>> > comment.
>> >
>> > ... or are duplicate reports supressed somehow?
>>
>> They are not suppressed yet. But I think we should just switch kasan
>> to single shot mode, i.e. report only the first error. Single bug
>> quite often has multiple invalid memory accesses causing storm in
>> dmesg. Also write OOB might corrupt metadata so the next report will
>> print bogus alloc/free stacktraces.
>> In most cases we need to look only at the first report, so reporting
>> anything after the first is just counterproductive.
>
> FWIW, that sounds sane to me.
>
> Given that, I agree with your comment regarding READ_ONCE{,_NOCHECK}().
>
> If anyone really wants all the reports, we could have a boot-time option
> to do that.
I don't mind changing READ_ONCE_NOCHECK to READ_ONCE. But I don't have
strong preference either way.
We could do:
#define arch_atomic_read_is_already_instrumented 1
and then skip instrumentation in asm-generic if it's defined. But I
don't think it's worth it.
There is no functional difference, it's only an optimization (now
somewhat questionable). As Andrey said, one can get a splash of
reports anyway, and it's the first one that is important. We use KASAN
with panic_on_warn=1 so we don't even see the rest.
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-03-21 22:30 +0100 |
| Subject | Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations |
| Message-ID | <tnzvQ-BH-5@gated-at.bofh.it> |
| In reply to | #1605866 |
On Tue, Mar 21, 2017 at 7:06 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Mar 21, 2017 at 11:41 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> On Tue, Mar 21, 2017 at 12:25:06PM +0300, Andrey Ryabinin wrote:
>
> I don't mind changing READ_ONCE_NOCHECK to READ_ONCE. But I don't have
> strong preference either way.
>
> We could do:
> #define arch_atomic_read_is_already_instrumented 1
> and then skip instrumentation in asm-generic if it's defined. But I
> don't think it's worth it.
>
> There is no functional difference, it's only an optimization (now
> somewhat questionable). As Andrey said, one can get a splash of
> reports anyway, and it's the first one that is important. We use KASAN
> with panic_on_warn=1 so we don't even see the rest.
I'm getting couple of new stack size warnings that are all the result
of the _NOCHECK.
/git/arm-soc/mm/page_alloc.c: In function 'show_free_areas':
/git/arm-soc/mm/page_alloc.c:4685:1: error: the frame size of 3368
bytes is larger than 3072 bytes [-Werror=frame-larger-than=]
}
/git/arm-soc/lib/atomic64_test.c: In function 'test_atomic':
/git/arm-soc/lib/atomic64_test.c:148:1: error: the frame size of 6528
bytes is larger than 3072 bytes [-Werror=frame-larger-than=]
}
^
/git/arm-soc/lib/atomic64_test.c: In function 'test_atomic64':
/git/arm-soc/lib/atomic64_test.c:243:1: error: the frame size of 7112
bytes is larger than 3072 bytes [-Werror=frame-larger-than=]
This is with my previous set of patches already applied, so
READ_ONCE should not cause problems. Reverting
the READ_ONCE_NOCHECK() in atomic_read() and atomic64_read()
back to READ_ONCE()
I also get a build failure as a result of your patch, but this one is
not addressed by using READ_ONCE():
In file included from /git/arm-soc/arch/x86/include/asm/atomic.h:7:0,
from /git/arm-soc/include/linux/atomic.h:4,
from /git/arm-soc/arch/x86/include/asm/thread_info.h:53,
from /git/arm-soc/include/linux/thread_info.h:25,
from /git/arm-soc/arch/x86/include/asm/preempt.h:6,
from /git/arm-soc/include/linux/preempt.h:80,
from /git/arm-soc/include/linux/spinlock.h:50,
from /git/arm-soc/include/linux/mmzone.h:7,
from /git/arm-soc/include/linux/gfp.h:5,
from /git/arm-soc/include/linux/mm.h:9,
from /git/arm-soc/mm/slub.c:12:
/git/arm-soc/mm/slub.c: In function '__slab_free':
/git/arm-soc/arch/x86/include/asm/cmpxchg.h:174:2: error: 'asm'
operand has impossible constraints
asm volatile(pfx "cmpxchg%c4b %2; sete %0" \
^
/git/arm-soc/arch/x86/include/asm/cmpxchg.h:183:2: note: in expansion
of macro '__cmpxchg_double'
__cmpxchg_double(LOCK_PREFIX, p1, p2, o1, o2, n1, n2)
^~~~~~~~~~~~~~~~
/git/arm-soc/include/asm-generic/atomic-instrumented.h:236:2: note: in
expansion of macro 'arch_cmpxchg_double'
arch_cmpxchg_double(____p1, (p2), (o1), (o2), (n1), (n2)); \
^~~~~~~~~~~~~~~~~~~
/git/arm-soc/mm/slub.c:385:7: note: in expansion of macro 'cmpxchg_double'
if (cmpxchg_double(&page->freelist, &page->counters,
^~~~~~~~~~~~~~
/git/arm-soc/scripts/Makefile.build:308: recipe for target 'mm/slub.o' failed
http://pastebin.com/raw/qXVpi9Ev has the defconfig file I used, and I get the
error with any gcc version I tried (4.9 through 7.0.1).
Arnd
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| Date | 2017-03-22 11:50 +0100 |
| Subject | Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations |
| Message-ID | <tnM03-11I-35@gated-at.bofh.it> |
| In reply to | #1606009 |
On Tue, Mar 21, 2017 at 10:20 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tue, Mar 21, 2017 at 7:06 PM, Dmitry Vyukov <dvyukov@google.com> wrote: >> On Tue, Mar 21, 2017 at 11:41 AM, Mark Rutland <mark.rutland@arm.com> wrote: >>> On Tue, Mar 21, 2017 at 12:25:06PM +0300, Andrey Ryabinin wrote: >> >> I don't mind changing READ_ONCE_NOCHECK to READ_ONCE. But I don't have >> strong preference either way. >> >> We could do: >> #define arch_atomic_read_is_already_instrumented 1 >> and then skip instrumentation in asm-generic if it's defined. But I >> don't think it's worth it. >> >> There is no functional difference, it's only an optimization (now >> somewhat questionable). As Andrey said, one can get a splash of >> reports anyway, and it's the first one that is important. We use KASAN >> with panic_on_warn=1 so we don't even see the rest. > > I'm getting couple of new stack size warnings that are all the result > of the _NOCHECK. > > /git/arm-soc/mm/page_alloc.c: In function 'show_free_areas': > /git/arm-soc/mm/page_alloc.c:4685:1: error: the frame size of 3368 > bytes is larger than 3072 bytes [-Werror=frame-larger-than=] > } > /git/arm-soc/lib/atomic64_test.c: In function 'test_atomic': > /git/arm-soc/lib/atomic64_test.c:148:1: error: the frame size of 6528 > bytes is larger than 3072 bytes [-Werror=frame-larger-than=] > } > ^ > /git/arm-soc/lib/atomic64_test.c: In function 'test_atomic64': > /git/arm-soc/lib/atomic64_test.c:243:1: error: the frame size of 7112 > bytes is larger than 3072 bytes [-Werror=frame-larger-than=] > > This is with my previous set of patches already applied, so > READ_ONCE should not cause problems. Reverting > the READ_ONCE_NOCHECK() in atomic_read() and atomic64_read() > back to READ_ONCE() > > I also get a build failure as a result of your patch, but this one is > not addressed by using READ_ONCE(): > > In file included from /git/arm-soc/arch/x86/include/asm/atomic.h:7:0, > from /git/arm-soc/include/linux/atomic.h:4, > from /git/arm-soc/arch/x86/include/asm/thread_info.h:53, > from /git/arm-soc/include/linux/thread_info.h:25, > from /git/arm-soc/arch/x86/include/asm/preempt.h:6, > from /git/arm-soc/include/linux/preempt.h:80, > from /git/arm-soc/include/linux/spinlock.h:50, > from /git/arm-soc/include/linux/mmzone.h:7, > from /git/arm-soc/include/linux/gfp.h:5, > from /git/arm-soc/include/linux/mm.h:9, > from /git/arm-soc/mm/slub.c:12: > /git/arm-soc/mm/slub.c: In function '__slab_free': > /git/arm-soc/arch/x86/include/asm/cmpxchg.h:174:2: error: 'asm' > operand has impossible constraints > asm volatile(pfx "cmpxchg%c4b %2; sete %0" \ > ^ > /git/arm-soc/arch/x86/include/asm/cmpxchg.h:183:2: note: in expansion > of macro '__cmpxchg_double' > __cmpxchg_double(LOCK_PREFIX, p1, p2, o1, o2, n1, n2) > ^~~~~~~~~~~~~~~~ > /git/arm-soc/include/asm-generic/atomic-instrumented.h:236:2: note: in > expansion of macro 'arch_cmpxchg_double' > arch_cmpxchg_double(____p1, (p2), (o1), (o2), (n1), (n2)); \ > ^~~~~~~~~~~~~~~~~~~ > /git/arm-soc/mm/slub.c:385:7: note: in expansion of macro 'cmpxchg_double' > if (cmpxchg_double(&page->freelist, &page->counters, > ^~~~~~~~~~~~~~ > /git/arm-soc/scripts/Makefile.build:308: recipe for target 'mm/slub.o' failed > > http://pastebin.com/raw/qXVpi9Ev has the defconfig file I used, and I get the > error with any gcc version I tried (4.9 through 7.0.1). Initially I've tested with my stock gcc 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04.3) and amusingly it works. But I can reproduce the bug with 7.0.1. Filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80148 Will think about kernel fix. Thanks!
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-03-22 12:40 +0100 |
| Subject | Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations |
| Message-ID | <tnMMq-1H2-29@gated-at.bofh.it> |
| In reply to | #1606355 |
On Wed, Mar 22, 2017 at 11:42 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Mar 21, 2017 at 10:20 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Tue, Mar 21, 2017 at 7:06 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> Initially I've tested with my stock gcc 4.8.4 (Ubuntu
> 4.8.4-2ubuntu1~14.04.3) and amusingly it works. But I can reproduce
> the bug with 7.0.1.
It's probably because gcc-4.8 didn't support KASAN yet, so the added
check had no effect.
Arnd
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| Date | 2017-03-22 13:30 +0100 |
| Subject | Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations |
| Message-ID | <tnNyO-2kr-15@gated-at.bofh.it> |
| In reply to | #1606394 |
On Wed, Mar 22, 2017 at 12:30 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wed, Mar 22, 2017 at 11:42 AM, Dmitry Vyukov <dvyukov@google.com> wrote: >> On Tue, Mar 21, 2017 at 10:20 PM, Arnd Bergmann <arnd@arndb.de> wrote: >>> On Tue, Mar 21, 2017 at 7:06 PM, Dmitry Vyukov <dvyukov@google.com> wrote: >> >> Initially I've tested with my stock gcc 4.8.4 (Ubuntu >> 4.8.4-2ubuntu1~14.04.3) and amusingly it works. But I can reproduce >> the bug with 7.0.1. > > It's probably because gcc-4.8 didn't support KASAN yet, so the added > check had no effect. I've tested without KASAN with both compilers.
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-03-22 13:50 +0100 |
| Subject | Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations |
| Message-ID | <tnNSa-2sZ-31@gated-at.bofh.it> |
| In reply to | #1606416 |
On Wed, Mar 22, 2017 at 1:14 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Wed, Mar 22, 2017 at 12:30 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Wed, Mar 22, 2017 at 11:42 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>> On Tue, Mar 21, 2017 at 10:20 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> On Tue, Mar 21, 2017 at 7:06 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>
>>> Initially I've tested with my stock gcc 4.8.4 (Ubuntu
>>> 4.8.4-2ubuntu1~14.04.3) and amusingly it works. But I can reproduce
>>> the bug with 7.0.1.
>>
>> It's probably because gcc-4.8 didn't support KASAN yet, so the added
>> check had no effect.
>
> I've tested without KASAN with both compilers.
Ah ok. I had not realized that this happened even without KASAN. I only saw this
problem in one out of hundreds of defconfig builds and assumed it was related
since this came from a kasan change.
Arnd
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2017-03-24 08:00 +0100 |
| Subject | Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations |
| Message-ID | <tormy-64z-11@gated-at.bofh.it> |
| In reply to | #1600848 |
* Dmitry Vyukov <dvyukov@google.com> wrote:
> KASAN uses compiler instrumentation to intercept all memory accesses.
> But it does not see memory accesses done in assembly code.
> One notable user of assembly code is atomic operations. Frequently,
> for example, an atomic reference decrement is the last access to an
> object and a good candidate for a racy use-after-free.
>
> Atomic operations are defined in arch files, but KASAN instrumentation
> is required for several archs that support KASAN. Later we will need
> similar hooks for KMSAN (uninit use detector) and KTSAN (data race
> detector).
>
> This change introduces wrappers around atomic operations that can be
> used to add KASAN/KMSAN/KTSAN instrumentation across several archs.
> This patch uses the wrappers only for x86 arch. Arm64 will be switched
> later.
>
> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Will Deacon <will.deacon@arm.com>,
> Cc: Andrew Morton <akpm@linux-foundation.org>,
> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>,
> Cc: Ingo Molnar <mingo@redhat.com>,
> Cc: kasan-dev@googlegroups.com
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Cc: x86@kernel.org
> ---
> arch/x86/include/asm/atomic.h | 100 +++++++-------
> arch/x86/include/asm/atomic64_32.h | 86 ++++++------
> arch/x86/include/asm/atomic64_64.h | 90 ++++++-------
> arch/x86/include/asm/cmpxchg.h | 12 +-
> arch/x86/include/asm/cmpxchg_32.h | 8 +-
> arch/x86/include/asm/cmpxchg_64.h | 4 +-
> include/asm-generic/atomic-instrumented.h | 210 ++++++++++++++++++++++++++++++
> 7 files changed, 367 insertions(+), 143 deletions(-)
Ugh, that's disgusting really...
>
> diff --git a/arch/x86/include/asm/atomic.h b/arch/x86/include/asm/atomic.h
> index 14635c5ea025..95dd167eb3af 100644
> --- a/arch/x86/include/asm/atomic.h
> +++ b/arch/x86/include/asm/atomic.h
> @@ -16,36 +16,46 @@
> #define ATOMIC_INIT(i) { (i) }
>
> /**
> - * atomic_read - read atomic variable
> + * arch_atomic_read - read atomic variable
> * @v: pointer of type atomic_t
> *
> * Atomically reads the value of @v.
> */
> -static __always_inline int atomic_read(const atomic_t *v)
> +static __always_inline int arch_atomic_read(const atomic_t *v)
> {
> - return READ_ONCE((v)->counter);
> + /*
> + * We use READ_ONCE_NOCHECK() because atomic_read() contains KASAN
> + * instrumentation. Double instrumentation is unnecessary.
> + */
> + return READ_ONCE_NOCHECK((v)->counter);
> }
Firstly, the patch is way too large, please split off new the documentation parts
of the patch to reduce the size and to make it easier to read!
Secondly, the next patch should do the rename to arch_atomic_*() pattern - and
nothing else:
>
> /**
> - * atomic_set - set atomic variable
> + * arch_atomic_set - set atomic variable
> * @v: pointer of type atomic_t
> * @i: required value
> *
> * Atomically sets the value of @v to @i.
> */
> -static __always_inline void atomic_set(atomic_t *v, int i)
> +static __always_inline void arch_atomic_set(atomic_t *v, int i)
Third, the prototype CPP complications:
> +#define __INSTR_VOID1(op, sz) \
> +static __always_inline void atomic##sz##_##op(atomic##sz##_t *v) \
> +{ \
> + arch_atomic##sz##_##op(v); \
> +}
> +
> +#define INSTR_VOID1(op) \
> +__INSTR_VOID1(op,); \
> +__INSTR_VOID1(op, 64)
> +
> +INSTR_VOID1(inc);
> +INSTR_VOID1(dec);
> +
> +#undef __INSTR_VOID1
> +#undef INSTR_VOID1
> +
> +#define __INSTR_VOID2(op, sz, type) \
> +static __always_inline void atomic##sz##_##op(type i, atomic##sz##_t *v)\
> +{ \
> + arch_atomic##sz##_##op(i, v); \
> +}
> +
> +#define INSTR_VOID2(op) \
> +__INSTR_VOID2(op, , int); \
> +__INSTR_VOID2(op, 64, long long)
> +
> +INSTR_VOID2(add);
> +INSTR_VOID2(sub);
> +INSTR_VOID2(and);
> +INSTR_VOID2(or);
> +INSTR_VOID2(xor);
> +
> +#undef __INSTR_VOID2
> +#undef INSTR_VOID2
> +
> +#define __INSTR_RET1(op, sz, type, rtype) \
> +static __always_inline rtype atomic##sz##_##op(atomic##sz##_t *v) \
> +{ \
> + return arch_atomic##sz##_##op(v); \
> +}
> +
> +#define INSTR_RET1(op) \
> +__INSTR_RET1(op, , int, int); \
> +__INSTR_RET1(op, 64, long long, long long)
> +
> +INSTR_RET1(inc_return);
> +INSTR_RET1(dec_return);
> +__INSTR_RET1(inc_not_zero, 64, long long, long long);
> +__INSTR_RET1(dec_if_positive, 64, long long, long long);
> +
> +#define INSTR_RET_BOOL1(op) \
> +__INSTR_RET1(op, , int, bool); \
> +__INSTR_RET1(op, 64, long long, bool)
> +
> +INSTR_RET_BOOL1(dec_and_test);
> +INSTR_RET_BOOL1(inc_and_test);
> +
> +#undef __INSTR_RET1
> +#undef INSTR_RET1
> +#undef INSTR_RET_BOOL1
> +
> +#define __INSTR_RET2(op, sz, type, rtype) \
> +static __always_inline rtype atomic##sz##_##op(type i, atomic##sz##_t *v) \
> +{ \
> + return arch_atomic##sz##_##op(i, v); \
> +}
> +
> +#define INSTR_RET2(op) \
> +__INSTR_RET2(op, , int, int); \
> +__INSTR_RET2(op, 64, long long, long long)
> +
> +INSTR_RET2(add_return);
> +INSTR_RET2(sub_return);
> +INSTR_RET2(fetch_add);
> +INSTR_RET2(fetch_sub);
> +INSTR_RET2(fetch_and);
> +INSTR_RET2(fetch_or);
> +INSTR_RET2(fetch_xor);
> +
> +#define INSTR_RET_BOOL2(op) \
> +__INSTR_RET2(op, , int, bool); \
> +__INSTR_RET2(op, 64, long long, bool)
> +
> +INSTR_RET_BOOL2(sub_and_test);
> +INSTR_RET_BOOL2(add_negative);
> +
> +#undef __INSTR_RET2
> +#undef INSTR_RET2
> +#undef INSTR_RET_BOOL2
Are just utterly disgusting that turn perfectly readable code into an unreadable,
unmaintainable mess.
You need to find some better, cleaner solution please, or convince me that no such
solution is possible. NAK for the time being.
Thanks,
Ingo
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| Date | 2017-03-24 08:20 +0100 |
| Subject | Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations |
| Message-ID | <torFU-6sZ-7@gated-at.bofh.it> |
| In reply to | #1608165 |
On Fri, Mar 24, 2017 at 7:52 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Dmitry Vyukov <dvyukov@google.com> wrote:
>
>> KASAN uses compiler instrumentation to intercept all memory accesses.
>> But it does not see memory accesses done in assembly code.
>> One notable user of assembly code is atomic operations. Frequently,
>> for example, an atomic reference decrement is the last access to an
>> object and a good candidate for a racy use-after-free.
>>
>> Atomic operations are defined in arch files, but KASAN instrumentation
>> is required for several archs that support KASAN. Later we will need
>> similar hooks for KMSAN (uninit use detector) and KTSAN (data race
>> detector).
>>
>> This change introduces wrappers around atomic operations that can be
>> used to add KASAN/KMSAN/KTSAN instrumentation across several archs.
>> This patch uses the wrappers only for x86 arch. Arm64 will be switched
>> later.
>>
>> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Will Deacon <will.deacon@arm.com>,
>> Cc: Andrew Morton <akpm@linux-foundation.org>,
>> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>,
>> Cc: Ingo Molnar <mingo@redhat.com>,
>> Cc: kasan-dev@googlegroups.com
>> Cc: linux-mm@kvack.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: x86@kernel.org
>> ---
>> arch/x86/include/asm/atomic.h | 100 +++++++-------
>> arch/x86/include/asm/atomic64_32.h | 86 ++++++------
>> arch/x86/include/asm/atomic64_64.h | 90 ++++++-------
>> arch/x86/include/asm/cmpxchg.h | 12 +-
>> arch/x86/include/asm/cmpxchg_32.h | 8 +-
>> arch/x86/include/asm/cmpxchg_64.h | 4 +-
>> include/asm-generic/atomic-instrumented.h | 210 ++++++++++++++++++++++++++++++
>> 7 files changed, 367 insertions(+), 143 deletions(-)
>
> Ugh, that's disgusting really...
>
>>
>> diff --git a/arch/x86/include/asm/atomic.h b/arch/x86/include/asm/atomic.h
>> index 14635c5ea025..95dd167eb3af 100644
>> --- a/arch/x86/include/asm/atomic.h
>> +++ b/arch/x86/include/asm/atomic.h
>> @@ -16,36 +16,46 @@
>> #define ATOMIC_INIT(i) { (i) }
>>
>> /**
>> - * atomic_read - read atomic variable
>> + * arch_atomic_read - read atomic variable
>> * @v: pointer of type atomic_t
>> *
>> * Atomically reads the value of @v.
>> */
>> -static __always_inline int atomic_read(const atomic_t *v)
>> +static __always_inline int arch_atomic_read(const atomic_t *v)
>> {
>> - return READ_ONCE((v)->counter);
>> + /*
>> + * We use READ_ONCE_NOCHECK() because atomic_read() contains KASAN
>> + * instrumentation. Double instrumentation is unnecessary.
>> + */
>> + return READ_ONCE_NOCHECK((v)->counter);
>> }
Hello Ingo,
> Firstly, the patch is way too large, please split off new the documentation parts
> of the patch to reduce the size and to make it easier to read!
>
> Secondly, the next patch should do the rename to arch_atomic_*() pattern - and
> nothing else:
Next after what? Please provide full list of patches as you see them.
How do we avoid build breakage if we do only the rename in a separate patch?
>> /**
>> - * atomic_set - set atomic variable
>> + * arch_atomic_set - set atomic variable
>> * @v: pointer of type atomic_t
>> * @i: required value
>> *
>> * Atomically sets the value of @v to @i.
>> */
>> -static __always_inline void atomic_set(atomic_t *v, int i)
>> +static __always_inline void arch_atomic_set(atomic_t *v, int i)
>
>
> Third, the prototype CPP complications:
>
>> +#define __INSTR_VOID1(op, sz) \
>> +static __always_inline void atomic##sz##_##op(atomic##sz##_t *v) \
>> +{ \
>> + arch_atomic##sz##_##op(v); \
>> +}
>> +
>> +#define INSTR_VOID1(op) \
>> +__INSTR_VOID1(op,); \
>> +__INSTR_VOID1(op, 64)
>> +
>> +INSTR_VOID1(inc);
>> +INSTR_VOID1(dec);
>> +
>> +#undef __INSTR_VOID1
>> +#undef INSTR_VOID1
>> +
>> +#define __INSTR_VOID2(op, sz, type) \
>> +static __always_inline void atomic##sz##_##op(type i, atomic##sz##_t *v)\
>> +{ \
>> + arch_atomic##sz##_##op(i, v); \
>> +}
>> +
>> +#define INSTR_VOID2(op) \
>> +__INSTR_VOID2(op, , int); \
>> +__INSTR_VOID2(op, 64, long long)
>> +
>> +INSTR_VOID2(add);
>> +INSTR_VOID2(sub);
>> +INSTR_VOID2(and);
>> +INSTR_VOID2(or);
>> +INSTR_VOID2(xor);
>> +
>> +#undef __INSTR_VOID2
>> +#undef INSTR_VOID2
>> +
>> +#define __INSTR_RET1(op, sz, type, rtype) \
>> +static __always_inline rtype atomic##sz##_##op(atomic##sz##_t *v) \
>> +{ \
>> + return arch_atomic##sz##_##op(v); \
>> +}
>> +
>> +#define INSTR_RET1(op) \
>> +__INSTR_RET1(op, , int, int); \
>> +__INSTR_RET1(op, 64, long long, long long)
>> +
>> +INSTR_RET1(inc_return);
>> +INSTR_RET1(dec_return);
>> +__INSTR_RET1(inc_not_zero, 64, long long, long long);
>> +__INSTR_RET1(dec_if_positive, 64, long long, long long);
>> +
>> +#define INSTR_RET_BOOL1(op) \
>> +__INSTR_RET1(op, , int, bool); \
>> +__INSTR_RET1(op, 64, long long, bool)
>> +
>> +INSTR_RET_BOOL1(dec_and_test);
>> +INSTR_RET_BOOL1(inc_and_test);
>> +
>> +#undef __INSTR_RET1
>> +#undef INSTR_RET1
>> +#undef INSTR_RET_BOOL1
>> +
>> +#define __INSTR_RET2(op, sz, type, rtype) \
>> +static __always_inline rtype atomic##sz##_##op(type i, atomic##sz##_t *v) \
>> +{ \
>> + return arch_atomic##sz##_##op(i, v); \
>> +}
>> +
>> +#define INSTR_RET2(op) \
>> +__INSTR_RET2(op, , int, int); \
>> +__INSTR_RET2(op, 64, long long, long long)
>> +
>> +INSTR_RET2(add_return);
>> +INSTR_RET2(sub_return);
>> +INSTR_RET2(fetch_add);
>> +INSTR_RET2(fetch_sub);
>> +INSTR_RET2(fetch_and);
>> +INSTR_RET2(fetch_or);
>> +INSTR_RET2(fetch_xor);
>> +
>> +#define INSTR_RET_BOOL2(op) \
>> +__INSTR_RET2(op, , int, bool); \
>> +__INSTR_RET2(op, 64, long long, bool)
>> +
>> +INSTR_RET_BOOL2(sub_and_test);
>> +INSTR_RET_BOOL2(add_negative);
>> +
>> +#undef __INSTR_RET2
>> +#undef INSTR_RET2
>> +#undef INSTR_RET_BOOL2
>
> Are just utterly disgusting that turn perfectly readable code into an unreadable,
> unmaintainable mess.
>
> You need to find some better, cleaner solution please, or convince me that no such
> solution is possible. NAK for the time being.
Well, I can just write all functions as is. Does it better confirm to
kernel style? I've just looked at the x86 atomic.h and it uses macros
for similar purpose (ATOMIC_OP/ATOMIC_FETCH_OP), so I thought that
must be idiomatic kernel style...
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| Date | 2017-03-24 09:50 +0100 |
| Subject | Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations |
| Message-ID | <tot51-7ku-35@gated-at.bofh.it> |
| In reply to | #1608172 |
On Fri, Mar 24, 2017 at 8:14 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Fri, Mar 24, 2017 at 7:52 AM, Ingo Molnar <mingo@kernel.org> wrote:
>>
>> * Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>>> KASAN uses compiler instrumentation to intercept all memory accesses.
>>> But it does not see memory accesses done in assembly code.
>>> One notable user of assembly code is atomic operations. Frequently,
>>> for example, an atomic reference decrement is the last access to an
>>> object and a good candidate for a racy use-after-free.
>>>
>>> Atomic operations are defined in arch files, but KASAN instrumentation
>>> is required for several archs that support KASAN. Later we will need
>>> similar hooks for KMSAN (uninit use detector) and KTSAN (data race
>>> detector).
>>>
>>> This change introduces wrappers around atomic operations that can be
>>> used to add KASAN/KMSAN/KTSAN instrumentation across several archs.
>>> This patch uses the wrappers only for x86 arch. Arm64 will be switched
>>> later.
>>>
>>> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>> Cc: Will Deacon <will.deacon@arm.com>,
>>> Cc: Andrew Morton <akpm@linux-foundation.org>,
>>> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>,
>>> Cc: Ingo Molnar <mingo@redhat.com>,
>>> Cc: kasan-dev@googlegroups.com
>>> Cc: linux-mm@kvack.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Cc: x86@kernel.org
>>> ---
>>> arch/x86/include/asm/atomic.h | 100 +++++++-------
>>> arch/x86/include/asm/atomic64_32.h | 86 ++++++------
>>> arch/x86/include/asm/atomic64_64.h | 90 ++++++-------
>>> arch/x86/include/asm/cmpxchg.h | 12 +-
>>> arch/x86/include/asm/cmpxchg_32.h | 8 +-
>>> arch/x86/include/asm/cmpxchg_64.h | 4 +-
>>> include/asm-generic/atomic-instrumented.h | 210 ++++++++++++++++++++++++++++++
>>> 7 files changed, 367 insertions(+), 143 deletions(-)
>>
>> Ugh, that's disgusting really...
>>
>>>
>>> diff --git a/arch/x86/include/asm/atomic.h b/arch/x86/include/asm/atomic.h
>>> index 14635c5ea025..95dd167eb3af 100644
>>> --- a/arch/x86/include/asm/atomic.h
>>> +++ b/arch/x86/include/asm/atomic.h
>>> @@ -16,36 +16,46 @@
>>> #define ATOMIC_INIT(i) { (i) }
>>>
>>> /**
>>> - * atomic_read - read atomic variable
>>> + * arch_atomic_read - read atomic variable
>>> * @v: pointer of type atomic_t
>>> *
>>> * Atomically reads the value of @v.
>>> */
>>> -static __always_inline int atomic_read(const atomic_t *v)
>>> +static __always_inline int arch_atomic_read(const atomic_t *v)
>>> {
>>> - return READ_ONCE((v)->counter);
>>> + /*
>>> + * We use READ_ONCE_NOCHECK() because atomic_read() contains KASAN
>>> + * instrumentation. Double instrumentation is unnecessary.
>>> + */
>>> + return READ_ONCE_NOCHECK((v)->counter);
>>> }
>
> Hello Ingo,
>
>> Firstly, the patch is way too large, please split off new the documentation parts
>> of the patch to reduce the size and to make it easier to read!
>>
>> Secondly, the next patch should do the rename to arch_atomic_*() pattern - and
>> nothing else:
>
> Next after what? Please provide full list of patches as you see them.
> How do we avoid build breakage if we do only the rename in a separate patch?
>
>
>
>>> /**
>>> - * atomic_set - set atomic variable
>>> + * arch_atomic_set - set atomic variable
>>> * @v: pointer of type atomic_t
>>> * @i: required value
>>> *
>>> * Atomically sets the value of @v to @i.
>>> */
>>> -static __always_inline void atomic_set(atomic_t *v, int i)
>>> +static __always_inline void arch_atomic_set(atomic_t *v, int i)
>>
>>
>> Third, the prototype CPP complications:
>>
>>> +#define __INSTR_VOID1(op, sz) \
>>> +static __always_inline void atomic##sz##_##op(atomic##sz##_t *v) \
>>> +{ \
>>> + arch_atomic##sz##_##op(v); \
>>> +}
>>> +
>>> +#define INSTR_VOID1(op) \
>>> +__INSTR_VOID1(op,); \
>>> +__INSTR_VOID1(op, 64)
>>> +
>>> +INSTR_VOID1(inc);
>>> +INSTR_VOID1(dec);
>>> +
>>> +#undef __INSTR_VOID1
>>> +#undef INSTR_VOID1
>>> +
>>> +#define __INSTR_VOID2(op, sz, type) \
>>> +static __always_inline void atomic##sz##_##op(type i, atomic##sz##_t *v)\
>>> +{ \
>>> + arch_atomic##sz##_##op(i, v); \
>>> +}
>>> +
>>> +#define INSTR_VOID2(op) \
>>> +__INSTR_VOID2(op, , int); \
>>> +__INSTR_VOID2(op, 64, long long)
>>> +
>>> +INSTR_VOID2(add);
>>> +INSTR_VOID2(sub);
>>> +INSTR_VOID2(and);
>>> +INSTR_VOID2(or);
>>> +INSTR_VOID2(xor);
>>> +
>>> +#undef __INSTR_VOID2
>>> +#undef INSTR_VOID2
>>> +
>>> +#define __INSTR_RET1(op, sz, type, rtype) \
>>> +static __always_inline rtype atomic##sz##_##op(atomic##sz##_t *v) \
>>> +{ \
>>> + return arch_atomic##sz##_##op(v); \
>>> +}
>>> +
>>> +#define INSTR_RET1(op) \
>>> +__INSTR_RET1(op, , int, int); \
>>> +__INSTR_RET1(op, 64, long long, long long)
>>> +
>>> +INSTR_RET1(inc_return);
>>> +INSTR_RET1(dec_return);
>>> +__INSTR_RET1(inc_not_zero, 64, long long, long long);
>>> +__INSTR_RET1(dec_if_positive, 64, long long, long long);
>>> +
>>> +#define INSTR_RET_BOOL1(op) \
>>> +__INSTR_RET1(op, , int, bool); \
>>> +__INSTR_RET1(op, 64, long long, bool)
>>> +
>>> +INSTR_RET_BOOL1(dec_and_test);
>>> +INSTR_RET_BOOL1(inc_and_test);
>>> +
>>> +#undef __INSTR_RET1
>>> +#undef INSTR_RET1
>>> +#undef INSTR_RET_BOOL1
>>> +
>>> +#define __INSTR_RET2(op, sz, type, rtype) \
>>> +static __always_inline rtype atomic##sz##_##op(type i, atomic##sz##_t *v) \
>>> +{ \
>>> + return arch_atomic##sz##_##op(i, v); \
>>> +}
>>> +
>>> +#define INSTR_RET2(op) \
>>> +__INSTR_RET2(op, , int, int); \
>>> +__INSTR_RET2(op, 64, long long, long long)
>>> +
>>> +INSTR_RET2(add_return);
>>> +INSTR_RET2(sub_return);
>>> +INSTR_RET2(fetch_add);
>>> +INSTR_RET2(fetch_sub);
>>> +INSTR_RET2(fetch_and);
>>> +INSTR_RET2(fetch_or);
>>> +INSTR_RET2(fetch_xor);
>>> +
>>> +#define INSTR_RET_BOOL2(op) \
>>> +__INSTR_RET2(op, , int, bool); \
>>> +__INSTR_RET2(op, 64, long long, bool)
>>> +
>>> +INSTR_RET_BOOL2(sub_and_test);
>>> +INSTR_RET_BOOL2(add_negative);
>>> +
>>> +#undef __INSTR_RET2
>>> +#undef INSTR_RET2
>>> +#undef INSTR_RET_BOOL2
>>
>> Are just utterly disgusting that turn perfectly readable code into an unreadable,
>> unmaintainable mess.
>>
>> You need to find some better, cleaner solution please, or convince me that no such
>> solution is possible. NAK for the time being.
>
> Well, I can just write all functions as is. Does it better confirm to
> kernel style? I've just looked at the x86 atomic.h and it uses macros
> for similar purpose (ATOMIC_OP/ATOMIC_FETCH_OP), so I thought that
> must be idiomatic kernel style...
Stephen Rothwell reported that this patch conflicts with:
a9ebf306f52c ("locking/atomic: Introduce atomic_try_cmpxchg()")
e6790e4b5d5e ("locking/atomic/x86: Use atomic_try_cmpxchg()")
does it make sense to base my patch on the tree where these patches
were added and then submit to that tree?
I've also sent 2 fixes for this patch, if I resent this I also squash
these fixes, right?
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2017-03-24 12:00 +0100 |
| Subject | Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations |
| Message-ID | <tov6P-la-29@gated-at.bofh.it> |
| In reply to | #1608172 |
* Dmitry Vyukov <dvyukov@google.com> wrote: > > Are just utterly disgusting that turn perfectly readable code into an > > unreadable, unmaintainable mess. > > > > You need to find some better, cleaner solution please, or convince me that no > > such solution is possible. NAK for the time being. > > Well, I can just write all functions as is. Does it better confirm to kernel > style? I think writing the prototypes out as-is, properly organized, beats any of these macro based solutions. > [...] I've just looked at the x86 atomic.h and it uses macros for similar > purpose (ATOMIC_OP/ATOMIC_FETCH_OP), so I thought that must be idiomatic kernel > style... Mind fixing those too while at it? And please squash any bug fixes and re-send a clean series against latest upstream or so. Thanks, Ingo
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| Date | 2017-03-24 13:50 +0100 |
| Subject | Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations |
| Message-ID | <towPg-1DH-11@gated-at.bofh.it> |
| In reply to | #1608314 |
On Fri, Mar 24, 2017 at 11:57 AM, Ingo Molnar <mingo@kernel.org> wrote: > > * Dmitry Vyukov <dvyukov@google.com> wrote: > >> > Are just utterly disgusting that turn perfectly readable code into an >> > unreadable, unmaintainable mess. >> > >> > You need to find some better, cleaner solution please, or convince me that no >> > such solution is possible. NAK for the time being. >> >> Well, I can just write all functions as is. Does it better confirm to kernel >> style? > > I think writing the prototypes out as-is, properly organized, beats any of these > macro based solutions. You mean write out the prototypes, but use what for definitions? Macros again? >> [...] I've just looked at the x86 atomic.h and it uses macros for similar >> purpose (ATOMIC_OP/ATOMIC_FETCH_OP), so I thought that must be idiomatic kernel >> style... > > Mind fixing those too while at it? I don't mind once I understand how exactly you want it to look. > And please squash any bug fixes and re-send a clean series against latest upstream > or so. > > Thanks, > > Ingo
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web