* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection [not found] ` <1504149176.23109.9.camel@gmx.de> @ 2017-08-31 4:01 ` Kees Cook 2017-08-31 4:10 ` Kees Cook 0 siblings, 1 reply; 15+ messages in thread From: Kees Cook @ 2017-08-31 4:01 UTC (permalink / raw) To: Mike Galbraith, David S. Miller Cc: LKML, Ingo Molnar, Reshetova, Elena, Network Development On Wed, Aug 30, 2017 at 8:12 PM, Mike Galbraith <efault@gmx.de> wrote: > On Wed, 2017-08-30 at 19:27 -0700, Kees Cook wrote: > >> Interesting! Can you try with 633547973ffc3 ("net: convert >> sk_buff.users from atomic_t to refcount_t") reverted? I'll see if >> running haveged will help me trigger this on my system... > > With that (plus 230cd1279d001 fix to it) reverted, vbox boots. Wonderful! Thank you so much for helping track this down. So, it seems that sk_buff.users will need some more special attention before we can convert it to refcount. x86-refcount will saturate with refcount_dec_and_test() if the result is negative. But that would mean at least starting at 0. FULL should have WARNed in this case, so I remain slightly confused why it was missed by FULL. Ingo, I'm not sure the best path for this. It seems we need to revert 230cd1279d001 and 633547973ffc3 and then we can restore ARCH_HAS_REFCOUNT. -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection 2017-08-31 4:01 ` tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Kees Cook @ 2017-08-31 4:10 ` Kees Cook 2017-08-31 4:38 ` Mike Galbraith 2017-08-31 13:58 ` Mike Galbraith 0 siblings, 2 replies; 15+ messages in thread From: Kees Cook @ 2017-08-31 4:10 UTC (permalink / raw) To: Mike Galbraith, David S. Miller, Peter Zijlstra Cc: LKML, Ingo Molnar, Reshetova, Elena, Network Development On Wed, Aug 30, 2017 at 9:01 PM, Kees Cook <keescook@chromium.org> wrote: > On Wed, Aug 30, 2017 at 8:12 PM, Mike Galbraith <efault@gmx.de> wrote: >> On Wed, 2017-08-30 at 19:27 -0700, Kees Cook wrote: >> >>> Interesting! Can you try with 633547973ffc3 ("net: convert >>> sk_buff.users from atomic_t to refcount_t") reverted? I'll see if >>> running haveged will help me trigger this on my system... >> >> With that (plus 230cd1279d001 fix to it) reverted, vbox boots. > > Wonderful! Thank you so much for helping track this down. > > So, it seems that sk_buff.users will need some more special attention > before we can convert it to refcount. > > x86-refcount will saturate with refcount_dec_and_test() if the result > is negative. But that would mean at least starting at 0. FULL should > have WARNed in this case, so I remain slightly confused why it was > missed by FULL. Actually, if this is a race condition it's possible that FULL is slow enough to miss it... I bet something briefly takes the refcount negative, and with unchecked atomics, it come back up positive again during the race. FULL may miss the race, and x86-refcount will catch it and saturate... -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection 2017-08-31 4:10 ` Kees Cook @ 2017-08-31 4:38 ` Mike Galbraith 2017-08-31 13:58 ` Mike Galbraith 1 sibling, 0 replies; 15+ messages in thread From: Mike Galbraith @ 2017-08-31 4:38 UTC (permalink / raw) To: Kees Cook, David S. Miller, Peter Zijlstra Cc: LKML, Ingo Molnar, Reshetova, Elena, Network Development On Wed, 2017-08-30 at 21:10 -0700, Kees Cook wrote: > On Wed, Aug 30, 2017 at 9:01 PM, Kees Cook <keescook@chromium.org> wrote: > > On Wed, Aug 30, 2017 at 8:12 PM, Mike Galbraith <efault@gmx.de> wrote: > >> On Wed, 2017-08-30 at 19:27 -0700, Kees Cook wrote: > >> > >>> Interesting! Can you try with 633547973ffc3 ("net: convert > >>> sk_buff.users from atomic_t to refcount_t") reverted? I'll see if > >>> running haveged will help me trigger this on my system... > >> > >> With that (plus 230cd1279d001 fix to it) reverted, vbox boots. > > > > Wonderful! Thank you so much for helping track this down. > > > > So, it seems that sk_buff.users will need some more special attention > > before we can convert it to refcount. > > > > x86-refcount will saturate with refcount_dec_and_test() if the result > > is negative. But that would mean at least starting at 0. FULL should > > have WARNed in this case, so I remain slightly confused why it was > > missed by FULL. > > Actually, if this is a race condition it's possible that FULL is slow > enough to miss it... > > I bet something briefly takes the refcount negative, and with > unchecked atomics, it come back up positive again during the race. > FULL may miss the race, and x86-refcount will catch it and saturate... Hm, I'll go have a stare.. not that that's likely to turn anything up, memory ordering stares usually inducing a zombie like state. -Mike ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection 2017-08-31 4:10 ` Kees Cook 2017-08-31 4:38 ` Mike Galbraith @ 2017-08-31 13:58 ` Mike Galbraith 2017-08-31 17:00 ` Kees Cook 2017-08-31 19:28 ` Kees Cook 1 sibling, 2 replies; 15+ messages in thread From: Mike Galbraith @ 2017-08-31 13:58 UTC (permalink / raw) To: Kees Cook, David S. Miller, Peter Zijlstra Cc: LKML, Ingo Molnar, Reshetova, Elena, Network Development On Wed, 2017-08-30 at 21:10 -0700, Kees Cook wrote: > On Wed, Aug 30, 2017 at 9:01 PM, Kees Cook <keescook@chromium.org> wrote: > > On Wed, Aug 30, 2017 at 8:12 PM, Mike Galbraith <efault@gmx.de> wrote: > >> On Wed, 2017-08-30 at 19:27 -0700, Kees Cook wrote: > >> > >>> Interesting! Can you try with 633547973ffc3 ("net: convert > >>> sk_buff.users from atomic_t to refcount_t") reverted? I'll see if > >>> running haveged will help me trigger this on my system... > >> > >> With that (plus 230cd1279d001 fix to it) reverted, vbox boots. > > > > Wonderful! Thank you so much for helping track this down. > > > > So, it seems that sk_buff.users will need some more special attention > > before we can convert it to refcount. > > > > x86-refcount will saturate with refcount_dec_and_test() if the result > > is negative. But that would mean at least starting at 0. FULL should > > have WARNed in this case, so I remain slightly confused why it was > > missed by FULL. > > Actually, if this is a race condition it's possible that FULL is slow > enough to miss it... > > I bet something briefly takes the refcount negative, and with > unchecked atomics, it come back up positive again during the race. > FULL may miss the race, and x86-refcount will catch it and saturate... (gdb) list *in6_dev_get+0x1e 0xffffffff8166d3de is in in6_dev_get (./arch/x86/include/asm/refcount.h:52). 47 : "cc", "cx"); 48 } 49 50 static __always_inline void refcount_inc(refcount_t *r) 51 { 52 asm volatile(LOCK_PREFIX "incl %0\n\t" 53 REFCOUNT_CHECK_LT_ZERO 54 : [counter] "+m" (r->refs.counter) 55 : : "cc", "cx"); 56 gdb) list *in6_dev_get+0x10 0xffffffff8166d3d0 is in in6_dev_get (./include/net/addrconf.h:318). 313 { 314 struct inet6_dev *idev; 315 316 rcu_read_lock(); 317 idev = rcu_dereference(dev->ip6_ptr); 318 if (idev) 319 refcount_inc(&idev->refcnt); 320 rcu_read_unlock(); 321 return idev; 322 That's from kernel with no revert, but your silent saturation patch still applied, AND built with gcc-6.3.1. Kernel traps, but it boots and works, as does kernel built with gcc-7.0.1. Remove your silent saturation patch, kernel doesn't notice a thing, just works. With gcc-4.8.5, trap means you're as good as dead, with the other two, trap means the intended. Compiler, constraints, dark elves.. pick one. Full first splat from bootable gcc-6.3.1 built kernel. [ 1.293962] NET: Registered protocol family 10 [ 1.294635] refcount_t silent saturation at in6_dev_get+0x25/0x104 in swapper/0[1], uid/euid: 0/0 [ 1.295616] ------------[ cut here ]------------ [ 1.296120] WARNING: CPU: 0 PID: 1 at kernel/panic.c:612 refcount_error_report+0x94/0x9e [ 1.296950] Modules linked in: [ 1.297276] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.13.0.g152d54a-tip-default #53 [ 1.299179] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014 [ 1.300743] task: ffff88013ab84040 task.stack: ffffc9000062c000 [ 1.301825] RIP: 0010:refcount_error_report+0x94/0x9e [ 1.302804] RSP: 0018:ffffc9000062fc10 EFLAGS: 00010282 [ 1.303791] RAX: 0000000000000055 RBX: ffffffff81a34274 RCX: ffffffff81c605e8 [ 1.304991] RDX: 0000000000000001 RSI: 0000000000000096 RDI: 0000000000000246 [ 1.306189] RBP: ffffc9000062fd58 R08: 0000000000000000 R09: 0000000000000175 [ 1.307392] R10: 0000000000000000 R11: 0000000000000001 R12: ffff88013ab84040 [ 1.308583] R13: 0000000000000000 R14: 0000000000000004 R15: ffffffff81a256c8 [ 1.309768] FS: 0000000000000000(0000) GS:ffff88013fc00000(0000) knlGS:0000000000000000 [ 1.311052] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 1.312100] CR2: 00007f4631fe8df0 CR3: 0000000137d09003 CR4: 00000000001606f0 [ 1.313301] Call Trace: [ 1.314012] ex_handler_refcount+0x63/0x70 [ 1.314893] fixup_exception+0x32/0x40 [ 1.315737] do_trap+0x8c/0x170 [ 1.316519] do_error_trap+0x70/0xd0 [ 1.317340] ? in6_dev_get+0x23/0x104 [ 1.318172] ? netlink_broadcast_filtered+0x2bd/0x430 [ 1.319156] ? kmem_cache_alloc_trace+0xce/0x5d0 [ 1.320098] ? set_debug_rodata+0x11/0x11 [ 1.320964] invalid_op+0x1e/0x30 [ 1.322520] RIP: 0010:in6_dev_get+0x25/0x104 [ 1.323631] RSP: 0018:ffffc9000062fe00 EFLAGS: 00010202 [ 1.324614] RAX: ffff880137de2400 RBX: ffff880137df4600 RCX: ffff880137de24f0 [ 1.325793] RDX: ffff88013a5e4000 RSI: 00000000fffffe00 RDI: ffff88013a5e4000 [ 1.326964] RBP: 00000000000000d1 R08: 0000000000000000 R09: ffff880137de7600 [ 1.328150] R10: 0000000000000000 R11: ffff8801398a4df8 R12: 0000000000000000 [ 1.329374] R13: ffffffff82137872 R14: 014200ca00000000 R15: 0000000000000000 [ 1.330547] ? set_debug_rodata+0x11/0x11 [ 1.331392] ip6_route_init_special_entries+0x2a/0x89 [ 1.332369] addrconf_init+0x9e/0x203 [ 1.333173] inet6_init+0x1af/0x365 [ 1.333956] ? af_unix_init+0x4e/0x4e [ 1.334753] do_one_initcall+0x4e/0x190 [ 1.335555] ? set_debug_rodata+0x11/0x11 [ 1.336369] kernel_init_freeable+0x189/0x20e [ 1.337230] ? rest_init+0xd0/0xd0 [ 1.337999] kernel_init+0xa/0xf7 [ 1.338744] ret_from_fork+0x25/0x30 [ 1.339500] Code: 48 8b 95 80 00 00 00 41 55 49 8d 8c 24 f0 0a 00 00 45 8b 84 24 10 09 00 00 41 89 c1 48 89 de 48 c7 c7 60 7a a3 81 e8 07 de 05 00 <0f> ff 58 5b 5d 41 5c 41 5d c3 0f 1f 44 00 00 55 48 89 e5 41 56 [ 1.342243] ---[ end trace b5d40c0fccce776c ]--- ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection 2017-08-31 13:58 ` Mike Galbraith @ 2017-08-31 17:00 ` Kees Cook 2017-08-31 17:19 ` Mike Galbraith 2017-08-31 19:28 ` Kees Cook 1 sibling, 1 reply; 15+ messages in thread From: Kees Cook @ 2017-08-31 17:00 UTC (permalink / raw) To: Mike Galbraith Cc: David S. Miller, Peter Zijlstra, LKML, Ingo Molnar, Reshetova, Elena, Network Development On Thu, Aug 31, 2017 at 6:58 AM, Mike Galbraith <efault@gmx.de> wrote: > On Wed, 2017-08-30 at 21:10 -0700, Kees Cook wrote: >> On Wed, Aug 30, 2017 at 9:01 PM, Kees Cook <keescook@chromium.org> wrote: >> > On Wed, Aug 30, 2017 at 8:12 PM, Mike Galbraith <efault@gmx.de> wrote: >> >> On Wed, 2017-08-30 at 19:27 -0700, Kees Cook wrote: >> >> >> >>> Interesting! Can you try with 633547973ffc3 ("net: convert >> >>> sk_buff.users from atomic_t to refcount_t") reverted? I'll see if >> >>> running haveged will help me trigger this on my system... >> >> >> >> With that (plus 230cd1279d001 fix to it) reverted, vbox boots. >> > >> > Wonderful! Thank you so much for helping track this down. >> > >> > So, it seems that sk_buff.users will need some more special attention >> > before we can convert it to refcount. >> > >> > x86-refcount will saturate with refcount_dec_and_test() if the result >> > is negative. But that would mean at least starting at 0. FULL should >> > have WARNed in this case, so I remain slightly confused why it was >> > missed by FULL. >> >> Actually, if this is a race condition it's possible that FULL is slow >> enough to miss it... >> >> I bet something briefly takes the refcount negative, and with >> unchecked atomics, it come back up positive again during the race. >> FULL may miss the race, and x86-refcount will catch it and saturate... > > (gdb) list *in6_dev_get+0x1e > 0xffffffff8166d3de is in in6_dev_get (./arch/x86/include/asm/refcount.h:52). > 47 : "cc", "cx"); > 48 } > 49 > 50 static __always_inline void refcount_inc(refcount_t *r) > 51 { > 52 asm volatile(LOCK_PREFIX "incl %0\n\t" > 53 REFCOUNT_CHECK_LT_ZERO > 54 : [counter] "+m" (r->refs.counter) > 55 : : "cc", "cx"); > 56 > > gdb) list *in6_dev_get+0x10 > 0xffffffff8166d3d0 is in in6_dev_get (./include/net/addrconf.h:318). > 313 { > 314 struct inet6_dev *idev; > 315 > 316 rcu_read_lock(); > 317 idev = rcu_dereference(dev->ip6_ptr); > 318 if (idev) > 319 refcount_inc(&idev->refcnt); > 320 rcu_read_unlock(); > 321 return idev; > 322 > > That's from kernel with no revert, but your silent saturation patch > still applied, AND built with gcc-6.3.1. Kernel traps, but it boots > and works, as does kernel built with gcc-7.0.1. Remove your silent > saturation patch, kernel doesn't notice a thing, just works. > > With gcc-4.8.5, trap means you're as good as dead, with the other two, > trap means the intended. Compiler, constraints, dark elves.. pick one. Oh! So it's gcc-version sensitive? That's alarming. Is this mapping correct: 4.8.5: WARN, eventual kernel hang 6.3.1, 7.0.1: WARN, but continues working > Full first splat from bootable gcc-6.3.1 built kernel. > > [ 1.293962] NET: Registered protocol family 10 > [ 1.294635] refcount_t silent saturation at in6_dev_get+0x25/0x104 in swapper/0[1], uid/euid: 0/0 That's an _increment_ saturation? Which means the result must be negative, so it started from least -2. > [ 1.295616] ------------[ cut here ]------------ > [ 1.296120] WARNING: CPU: 0 PID: 1 at kernel/panic.c:612 refcount_error_report+0x94/0x9e > [ 1.296950] Modules linked in: > [ 1.297276] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.13.0.g152d54a-tip-default #53 > [ 1.299179] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014 > [ 1.300743] task: ffff88013ab84040 task.stack: ffffc9000062c000 > [ 1.301825] RIP: 0010:refcount_error_report+0x94/0x9e > [ 1.302804] RSP: 0018:ffffc9000062fc10 EFLAGS: 00010282 > [ 1.303791] RAX: 0000000000000055 RBX: ffffffff81a34274 RCX: ffffffff81c605e8 > [ 1.304991] RDX: 0000000000000001 RSI: 0000000000000096 RDI: 0000000000000246 > [ 1.306189] RBP: ffffc9000062fd58 R08: 0000000000000000 R09: 0000000000000175 > [ 1.307392] R10: 0000000000000000 R11: 0000000000000001 R12: ffff88013ab84040 > [ 1.308583] R13: 0000000000000000 R14: 0000000000000004 R15: ffffffff81a256c8 > [ 1.309768] FS: 0000000000000000(0000) GS:ffff88013fc00000(0000) knlGS:0000000000000000 > [ 1.311052] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 1.312100] CR2: 00007f4631fe8df0 CR3: 0000000137d09003 CR4: 00000000001606f0 > [ 1.313301] Call Trace: > [ 1.314012] ex_handler_refcount+0x63/0x70 > [ 1.314893] fixup_exception+0x32/0x40 > [ 1.315737] do_trap+0x8c/0x170 > [ 1.316519] do_error_trap+0x70/0xd0 > [ 1.317340] ? in6_dev_get+0x23/0x104 > [ 1.318172] ? netlink_broadcast_filtered+0x2bd/0x430 > [ 1.319156] ? kmem_cache_alloc_trace+0xce/0x5d0 > [ 1.320098] ? set_debug_rodata+0x11/0x11 > [ 1.320964] invalid_op+0x1e/0x30 > [ 1.322520] RIP: 0010:in6_dev_get+0x25/0x104 > [ 1.323631] RSP: 0018:ffffc9000062fe00 EFLAGS: 00010202 > [ 1.324614] RAX: ffff880137de2400 RBX: ffff880137df4600 RCX: ffff880137de24f0 > [ 1.325793] RDX: ffff88013a5e4000 RSI: 00000000fffffe00 RDI: ffff88013a5e4000 > [ 1.326964] RBP: 00000000000000d1 R08: 0000000000000000 R09: ffff880137de7600 > [ 1.328150] R10: 0000000000000000 R11: ffff8801398a4df8 R12: 0000000000000000 > [ 1.329374] R13: ffffffff82137872 R14: 014200ca00000000 R15: 0000000000000000 > [ 1.330547] ? set_debug_rodata+0x11/0x11 > [ 1.331392] ip6_route_init_special_entries+0x2a/0x89 > [ 1.332369] addrconf_init+0x9e/0x203 > [ 1.333173] inet6_init+0x1af/0x365 > [ 1.333956] ? af_unix_init+0x4e/0x4e > [ 1.334753] do_one_initcall+0x4e/0x190 > [ 1.335555] ? set_debug_rodata+0x11/0x11 > [ 1.336369] kernel_init_freeable+0x189/0x20e > [ 1.337230] ? rest_init+0xd0/0xd0 > [ 1.337999] kernel_init+0xa/0xf7 > [ 1.338744] ret_from_fork+0x25/0x30 > [ 1.339500] Code: 48 8b 95 80 00 00 00 41 55 49 8d 8c 24 f0 0a 00 00 45 8b 84 24 10 09 00 00 41 89 c1 48 89 de 48 c7 c7 60 7a a3 81 e8 07 de 05 00 <0f> ff 58 5b 5d 41 5c 41 5d c3 0f 1f 44 00 00 55 48 89 e5 41 56 > [ 1.342243] ---[ end trace b5d40c0fccce776c ]--- -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection 2017-08-31 17:00 ` Kees Cook @ 2017-08-31 17:19 ` Mike Galbraith 2017-08-31 18:45 ` Kees Cook 0 siblings, 1 reply; 15+ messages in thread From: Mike Galbraith @ 2017-08-31 17:19 UTC (permalink / raw) To: Kees Cook Cc: David S. Miller, Peter Zijlstra, LKML, Ingo Molnar, Reshetova, Elena, Network Development On Thu, 2017-08-31 at 10:00 -0700, Kees Cook wrote: > > Oh! So it's gcc-version sensitive? That's alarming. Is this mapping correct: > > 4.8.5: WARN, eventual kernel hang > 6.3.1, 7.0.1: WARN, but continues working Yeah, that's correct. I find that troubling, simply because this gcc version has been through one hell of a lot of kernels with me. Yeah, I know, that doesn't exempt it from having bugs, but color me suspicious. -Mike ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection 2017-08-31 17:19 ` Mike Galbraith @ 2017-08-31 18:45 ` Kees Cook 2017-09-01 6:57 ` Mike Galbraith 0 siblings, 1 reply; 15+ messages in thread From: Kees Cook @ 2017-08-31 18:45 UTC (permalink / raw) To: Mike Galbraith Cc: David S. Miller, Peter Zijlstra, LKML, Ingo Molnar, Reshetova, Elena, Network Development On Thu, Aug 31, 2017 at 10:19 AM, Mike Galbraith <efault@gmx.de> wrote: > On Thu, 2017-08-31 at 10:00 -0700, Kees Cook wrote: >> >> Oh! So it's gcc-version sensitive? That's alarming. Is this mapping correct: >> >> 4.8.5: WARN, eventual kernel hang >> 6.3.1, 7.0.1: WARN, but continues working > > Yeah, that's correct. I find that troubling, simply because this gcc > version has been through one hell of a lot of kernels with me. Yeah, I > know, that doesn't exempt it from having bugs, but color me suspicious. I still can't hit this with a 4.8.5 build. :( With _RATELIMIT removed, this should, in theory, report whatever goes negative first... diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c index c076f710de4c..d4fc6b91c0e6 100644 --- a/arch/x86/mm/extable.c +++ b/arch/x86/mm/extable.c @@ -72,6 +72,10 @@ bool ex_handler_refcount(const struct exception_table_entry *fixup, bool zero = regs->flags & X86_EFLAGS_ZF; refcount_error_report(regs, zero ? "hit zero" : "overflow"); + } else if (regs->flags & X86_EFLAGS_SF) { + refcount_error_report(regs, "result was negative"); + } else { + refcount_error_report(regs, "unknown state"); } return true; diff --git a/kernel/panic.c b/kernel/panic.c index bdd18afa19a4..966ade491543 100644 --- a/kernel/panic.c +++ b/kernel/panic.c @@ -605,7 +605,7 @@ EXPORT_SYMBOL(__stack_chk_fail); #ifdef CONFIG_ARCH_HAS_REFCOUNT void refcount_error_report(struct pt_regs *regs, const char *err) { - WARN_RATELIMIT(1, "refcount_t %s at %pB in %s[%d], uid/euid: %u/%u\n", + WARN(1, "refcount_t %s at %pB in %s[%d], uid/euid: %u/%u\n", err, (void *)instruction_pointer(regs), current->comm, task_pid_nr(current), from_kuid_munged(&init_user_ns, current_uid()), And if it is still a refcount_inc(), and only on gcc 4.8.5, then I think we need to study the resulting assembly... -Kees -- Kees Cook Pixel Security ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection 2017-08-31 18:45 ` Kees Cook @ 2017-09-01 6:57 ` Mike Galbraith 2017-09-01 13:09 ` Mike Galbraith 0 siblings, 1 reply; 15+ messages in thread From: Mike Galbraith @ 2017-09-01 6:57 UTC (permalink / raw) To: Kees Cook Cc: David S. Miller, Peter Zijlstra, LKML, Ingo Molnar, Reshetova, Elena, Network Development On Thu, 2017-08-31 at 11:45 -0700, Kees Cook wrote: > On Thu, Aug 31, 2017 at 10:19 AM, Mike Galbraith <efault@gmx.de> wrote: > > On Thu, 2017-08-31 at 10:00 -0700, Kees Cook wrote: > >> > >> Oh! So it's gcc-version sensitive? That's alarming. Is this mapping correct: > >> > >> 4.8.5: WARN, eventual kernel hang > >> 6.3.1, 7.0.1: WARN, but continues working > > > > Yeah, that's correct. I find that troubling, simply because this gcc > > version has been through one hell of a lot of kernels with me. Yeah, I > > know, that doesn't exempt it from having bugs, but color me suspicious. > > I still can't hit this with a 4.8.5 build. :( > > With _RATELIMIT removed, this should, in theory, report whatever goes > negative first... I applied the other patch you posted, and built with gcc-6.3.1 to remove the gcc-4.8.5 aspect. Look below the resulting splat. [ 1.293962] NET: Registered protocol family 10 [ 1.294635] refcount_t silent saturation at in6_dev_get+0x25/0x104 in swapper/0[1], uid/euid: 0/0 [ 1.295616] ------------[ cut here ]------------ [ 1.296120] WARNING: CPU: 0 PID: 1 at kernel/panic.c:612 refcount_error_report+0x94/0x9e [ 1.296950] Modules linked in: [ 1.297276] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.13.0.g152d54a-tip-default #53 [ 1.299179] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014 [ 1.300743] task: ffff88013ab84040 task.stack: ffffc9000062c000 [ 1.301825] RIP: 0010:refcount_error_report+0x94/0x9e [ 1.302804] RSP: 0018:ffffc9000062fc10 EFLAGS: 00010282 [ 1.303791] RAX: 0000000000000055 RBX: ffffffff81a34274 RCX: ffffffff81c605e8 [ 1.304991] RDX: 0000000000000001 RSI: 0000000000000096 RDI: 0000000000000246 [ 1.306189] RBP: ffffc9000062fd58 R08: 0000000000000000 R09: 0000000000000175 [ 1.307392] R10: 0000000000000000 R11: 0000000000000001 R12: ffff88013ab84040 [ 1.308583] R13: 0000000000000000 R14: 0000000000000004 R15: ffffffff81a256c8 [ 1.309768] FS: 0000000000000000(0000) GS:ffff88013fc00000(0000) knlGS:0000000000000000 [ 1.311052] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 1.312100] CR2: 00007f4631fe8df0 CR3: 0000000137d09003 CR4: 00000000001606f0 [ 1.313301] Call Trace: [ 1.314012] ex_handler_refcount+0x63/0x70 [ 1.314893] fixup_exception+0x32/0x40 [ 1.315737] do_trap+0x8c/0x170 [ 1.316519] do_error_trap+0x70/0xd0 [ 1.317340] ? in6_dev_get+0x23/0x104 [ 1.318172] ? netlink_broadcast_filtered+0x2bd/0x430 [ 1.319156] ? kmem_cache_alloc_trace+0xce/0x5d0 [ 1.320098] ? set_debug_rodata+0x11/0x11 [ 1.320964] invalid_op+0x1e/0x30 [ 1.322520] RIP: 0010:in6_dev_get+0x25/0x104 [ 1.323631] RSP: 0018:ffffc9000062fe00 EFLAGS: 00010202 [ 1.324614] RAX: ffff880137de2400 RBX: ffff880137df4600 RCX: ffff880137de24f0 [ 1.325793] RDX: ffff88013a5e4000 RSI: 00000000fffffe00 RDI: ffff88013a5e4000 [ 1.326964] RBP: 00000000000000d1 R08: 0000000000000000 R09: ffff880137de7600 [ 1.328150] R10: 0000000000000000 R11: ffff8801398a4df8 R12: 0000000000000000 [ 1.329374] R13: ffffffff82137872 R14: 014200ca00000000 R15: 0000000000000000 [ 1.330547] ? set_debug_rodata+0x11/0x11 [ 1.331392] ip6_route_init_special_entries+0x2a/0x89 [ 1.332369] addrconf_init+0x9e/0x203 [ 1.333173] inet6_init+0x1af/0x365 [ 1.333956] ? af_unix_init+0x4e/0x4e [ 1.334753] do_one_initcall+0x4e/0x190 [ 1.335555] ? set_debug_rodata+0x11/0x11 [ 1.336369] kernel_init_freeable+0x189/0x20e [ 1.337230] ? rest_init+0xd0/0xd0 [ 1.337999] kernel_init+0xa/0xf7 [ 1.338744] ret_from_fork+0x25/0x30 [ 1.339500] Code: 48 8b 95 80 00 00 00 41 55 49 8d 8c 24 f0 0a 00 00 45 8b 84 24 10 09 00 00 41 89 c1 48 89 de 48 c7 c7 60 7a a3 81 e8 07 de 05 00 <0f> ff 58 5b 5d 41 5c 41 5d c3 0f 1f 44 00 00 55 48 89 e5 41 56 [ 1.342243] ---[ end trace b5d40c0fccce776c ]--- Back yours out, and... # tracer: nop # # _-----=> irqs-off # / _----=> need-resched # | / _---=> hardirq/softirq # || / _--=> preempt-depth # ||| / delay # TASK-PID CPU# |||| TIMESTAMP FUNCTION # | | | |||| | | swapper/0-1 [000] ...1 1.974114: in6_dev_getx: refs.counter:-1073741824 swapper/0-1 [000] ...1 1.974116: in6_dev_getx: refs.counter:-1073741824 --- arch/x86/include/asm/refcount.h | 9 +++++++++ include/net/addrconf.h | 12 ++++++++++++ net/ipv6/route.c | 4 ++-- 3 files changed, 23 insertions(+), 2 deletions(-) --- a/arch/x86/include/asm/refcount.h +++ b/arch/x86/include/asm/refcount.h @@ -55,6 +55,15 @@ static __always_inline void refcount_inc : : "cc", "cx"); } +static __always_inline void refcount_inc_x(refcount_t *r) +{ + trace_printk("refs.counter:%d\n", r->refs.counter); + asm volatile(LOCK_PREFIX "incl %0\n\t" + REFCOUNT_CHECK_LT_ZERO + : [counter] "+m" (r->refs.counter) + : : "cc", "cx"); +} + static __always_inline void refcount_dec(refcount_t *r) { asm volatile(LOCK_PREFIX "decl %0\n\t" --- a/include/net/addrconf.h +++ b/include/net/addrconf.h @@ -321,6 +321,18 @@ static inline struct inet6_dev *in6_dev_ return idev; } +static inline struct inet6_dev *in6_dev_getx(const struct net_device *dev) +{ + struct inet6_dev *idev; + + rcu_read_lock(); + idev = rcu_dereference(dev->ip6_ptr); + if (idev) + refcount_inc_x(&idev->refcnt); + rcu_read_unlock(); + return idev; +} + static inline struct neigh_parms *__in6_dev_nd_parms_get_rcu(const struct net_device *dev) { struct inet6_dev *idev = __in6_dev_get(dev); --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -4044,9 +4044,9 @@ void __init ip6_route_init_special_entri init_net.ipv6.ip6_null_entry->rt6i_idev = in6_dev_get(init_net.loopback_dev); #ifdef CONFIG_IPV6_MULTIPLE_TABLES init_net.ipv6.ip6_prohibit_entry->dst.dev = init_net.loopback_dev; - init_net.ipv6.ip6_prohibit_entry->rt6i_idev = in6_dev_get(init_net.loopback_dev); + init_net.ipv6.ip6_prohibit_entry->rt6i_idev = in6_dev_getx(init_net.loopback_dev); init_net.ipv6.ip6_blk_hole_entry->dst.dev = init_net.loopback_dev; - init_net.ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_get(init_net.loopback_dev); + init_net.ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_getx(init_net.loopback_dev); #endif } ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection 2017-09-01 6:57 ` Mike Galbraith @ 2017-09-01 13:09 ` Mike Galbraith 2017-09-01 17:12 ` Kees Cook 0 siblings, 1 reply; 15+ messages in thread From: Mike Galbraith @ 2017-09-01 13:09 UTC (permalink / raw) To: Kees Cook Cc: David S. Miller, Peter Zijlstra, LKML, Ingo Molnar, Reshetova, Elena, Network Development On Fri, 2017-09-01 at 08:57 +0200, Mike Galbraith wrote: > On Thu, 2017-08-31 at 11:45 -0700, Kees Cook wrote: > > On Thu, Aug 31, 2017 at 10:19 AM, Mike Galbraith <efault@gmx.de> wrote: > > > On Thu, 2017-08-31 at 10:00 -0700, Kees Cook wrote: > > >> > > >> Oh! So it's gcc-version sensitive? That's alarming. Is this mapping correct: > > >> > > >> 4.8.5: WARN, eventual kernel hang > > >> 6.3.1, 7.0.1: WARN, but continues working > > > > > > Yeah, that's correct. I find that troubling, simply because this gcc > > > version has been through one hell of a lot of kernels with me. Yeah, I > > > know, that doesn't exempt it from having bugs, but color me suspicious. > > > > I still can't hit this with a 4.8.5 build. :( > > > > With _RATELIMIT removed, this should, in theory, report whatever goes > > negative first... > > I applied the other patch you posted, and built with gcc-6.3.1 to > remove the gcc-4.8.5 aspect. Look below the resulting splat. Grr, that one has a in6_dev_getx() line missing for the first increment, where things go pear shaped. With that added, looking at counter both before, and after incl, with a trace_printk() in the exception handler showing it doing its saturate thing, irqs disabled across the whole damn refcount_inc(), and even booting box nr_cpus=1 for extra credit... HTH can that first refcount_inc() get there? # tracer: nop # # _-----=> irqs-off # / _----=> need-resched # | / _---=> hardirq/softirq # || / _--=> preempt-depth # ||| / delay # TASK-PID CPU# |||| TIMESTAMP FUNCTION # | | | |||| | | systemd-1 [000] d..1 1.937284: in6_dev_getx: PRE refs.counter:3 systemd-1 [000] d..1 1.937295: ex_handler_refcount: *(int *)regs->cx = -1073741824 systemd-1 [000] d..1 1.937296: in6_dev_getx: POST refs.counter:-1073741824 systemd-1 [000] d..1 1.937296: in6_dev_getx: PRE refs.counter:-1073741824 systemd-1 [000] d..1 1.937297: ex_handler_refcount: *(int *)regs->cx = -1073741824 systemd-1 [000] d..1 1.937297: in6_dev_getx: POST refs.counter:-1073741824 systemd-1 [000] d..1 1.937297: in6_dev_getx: PRE refs.counter:-1073741824 systemd-1 [000] d..1 1.937298: ex_handler_refcount: *(int *)regs->cx = -1073741824 systemd-1 [000] d..1 1.937299: in6_dev_getx: POST refs.counter:-1073741824 --- arch/x86/include/asm/refcount.h | 14 ++++++++++++++ arch/x86/mm/extable.c | 1 + include/net/addrconf.h | 12 ++++++++++++ net/ipv6/route.c | 6 +++--- 4 files changed, 30 insertions(+), 3 deletions(-) --- a/arch/x86/include/asm/refcount.h +++ b/arch/x86/include/asm/refcount.h @@ -55,6 +55,20 @@ static __always_inline void refcount_inc : : "cc", "cx"); } +static __always_inline void refcount_inc_x(refcount_t *r) +{ + unsigned long flags; + + local_irq_save(flags); + trace_printk("PRE refs.counter:%d\n", r->refs.counter); + asm volatile(LOCK_PREFIX "incl %0\n\t" + REFCOUNT_CHECK_LT_ZERO + : [counter] "+m" (r->refs.counter) + : : "cc", "cx"); + trace_printk("POST refs.counter:%d\n", r->refs.counter); + local_irq_restore(flags); +} + static __always_inline void refcount_dec(refcount_t *r) { asm volatile(LOCK_PREFIX "decl %0\n\t" --- a/arch/x86/mm/extable.c +++ b/arch/x86/mm/extable.c @@ -45,6 +45,7 @@ bool ex_handler_refcount(const struct ex { /* First unconditionally saturate the refcount. */ *(int *)regs->cx = INT_MIN / 2; + trace_printk("*(int *)regs->cx = %d\n", *(int *)regs->cx); /* * Strictly speaking, this reports the fixup destination, not --- a/include/net/addrconf.h +++ b/include/net/addrconf.h @@ -321,6 +321,18 @@ static inline struct inet6_dev *in6_dev_ return idev; } +static inline struct inet6_dev *in6_dev_getx(const struct net_device *dev) +{ + struct inet6_dev *idev; + + rcu_read_lock(); + idev = rcu_dereference(dev->ip6_ptr); + if (idev) + refcount_inc_x(&idev->refcnt); + rcu_read_unlock(); + return idev; +} + static inline struct neigh_parms *__in6_dev_nd_parms_get_rcu(const struct net_device *dev) { struct inet6_dev *idev = __in6_dev_get(dev); --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -4041,12 +4041,12 @@ void __init ip6_route_init_special_entri * the loopback reference in rt6_info will not be taken, do it * manually for init_net */ init_net.ipv6.ip6_null_entry->dst.dev = init_net.loopback_dev; - init_net.ipv6.ip6_null_entry->rt6i_idev = in6_dev_get(init_net.loopback_dev); + init_net.ipv6.ip6_null_entry->rt6i_idev = in6_dev_getx(init_net.loopback_dev); #ifdef CONFIG_IPV6_MULTIPLE_TABLES init_net.ipv6.ip6_prohibit_entry->dst.dev = init_net.loopback_dev; - init_net.ipv6.ip6_prohibit_entry->rt6i_idev = in6_dev_get(init_net.loopback_dev); + init_net.ipv6.ip6_prohibit_entry->rt6i_idev = in6_dev_getx(init_net.loopback_dev); init_net.ipv6.ip6_blk_hole_entry->dst.dev = init_net.loopback_dev; - init_net.ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_get(init_net.loopback_dev); + init_net.ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_getx(init_net.loopback_dev); #endif } ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection 2017-09-01 13:09 ` Mike Galbraith @ 2017-09-01 17:12 ` Kees Cook 2017-09-01 17:52 ` Mike Galbraith 0 siblings, 1 reply; 15+ messages in thread From: Kees Cook @ 2017-09-01 17:12 UTC (permalink / raw) To: Mike Galbraith Cc: David S. Miller, Peter Zijlstra, LKML, Ingo Molnar, Reshetova, Elena, Network Development On Fri, Sep 1, 2017 at 6:09 AM, Mike Galbraith <efault@gmx.de> wrote: > On Fri, 2017-09-01 at 08:57 +0200, Mike Galbraith wrote: >> On Thu, 2017-08-31 at 11:45 -0700, Kees Cook wrote: >> > On Thu, Aug 31, 2017 at 10:19 AM, Mike Galbraith <efault@gmx.de> wrote: >> > > On Thu, 2017-08-31 at 10:00 -0700, Kees Cook wrote: >> > >> >> > >> Oh! So it's gcc-version sensitive? That's alarming. Is this mapping correct: >> > >> >> > >> 4.8.5: WARN, eventual kernel hang >> > >> 6.3.1, 7.0.1: WARN, but continues working >> > > >> > > Yeah, that's correct. I find that troubling, simply because this gcc >> > > version has been through one hell of a lot of kernels with me. Yeah, I >> > > know, that doesn't exempt it from having bugs, but color me suspicious. >> > >> > I still can't hit this with a 4.8.5 build. :( >> > >> > With _RATELIMIT removed, this should, in theory, report whatever goes >> > negative first... >> >> I applied the other patch you posted, and built with gcc-6.3.1 to >> remove the gcc-4.8.5 aspect. Look below the resulting splat. > > Grr, that one has a in6_dev_getx() line missing for the first > increment, where things go pear shaped. > > With that added, looking at counter both before, and after incl, with a > trace_printk() in the exception handler showing it doing its saturate > thing, irqs disabled across the whole damn refcount_inc(), and even > booting box nr_cpus=1 for extra credit... > > HTH can that first refcount_inc() get there? > > # tracer: nop > # > # _-----=> irqs-off > # / _----=> need-resched > # | / _---=> hardirq/softirq > # || / _--=> preempt-depth > # ||| / delay > # TASK-PID CPU# |||| TIMESTAMP FUNCTION > # | | | |||| | | > systemd-1 [000] d..1 1.937284: in6_dev_getx: PRE refs.counter:3 > systemd-1 [000] d..1 1.937295: ex_handler_refcount: *(int *)regs->cx = -1073741824 > systemd-1 [000] d..1 1.937296: in6_dev_getx: POST refs.counter:-1073741824 O_o Can you paste the disassembly of in6_dev_getx? I can't understand how we're landing in the exception handler. > systemd-1 [000] d..1 1.937296: in6_dev_getx: PRE refs.counter:-1073741824 > systemd-1 [000] d..1 1.937297: ex_handler_refcount: *(int *)regs->cx = -1073741824 > systemd-1 [000] d..1 1.937297: in6_dev_getx: POST refs.counter:-1073741824 > systemd-1 [000] d..1 1.937297: in6_dev_getx: PRE refs.counter:-1073741824 > systemd-1 [000] d..1 1.937298: ex_handler_refcount: *(int *)regs->cx = -1073741824 > systemd-1 [000] d..1 1.937299: in6_dev_getx: POST refs.counter:-1073741824 > > --- > arch/x86/include/asm/refcount.h | 14 ++++++++++++++ > arch/x86/mm/extable.c | 1 + > include/net/addrconf.h | 12 ++++++++++++ > net/ipv6/route.c | 6 +++--- > 4 files changed, 30 insertions(+), 3 deletions(-) > > --- a/arch/x86/include/asm/refcount.h > +++ b/arch/x86/include/asm/refcount.h > @@ -55,6 +55,20 @@ static __always_inline void refcount_inc > : : "cc", "cx"); > } > > +static __always_inline void refcount_inc_x(refcount_t *r) > +{ > + unsigned long flags; > + > + local_irq_save(flags); > + trace_printk("PRE refs.counter:%d\n", r->refs.counter); > + asm volatile(LOCK_PREFIX "incl %0\n\t" > + REFCOUNT_CHECK_LT_ZERO > + : [counter] "+m" (r->refs.counter) > + : : "cc", "cx"); Does this need an explicit "memory" added to the clobbers line here? This isn't present in the atomic_inc() implementation, but maybe something confuses gcc in this case into ignoring the "+m" marking? > + trace_printk("POST refs.counter:%d\n", r->refs.counter); > + local_irq_restore(flags); > +} > + > static __always_inline void refcount_dec(refcount_t *r) > { > asm volatile(LOCK_PREFIX "decl %0\n\t" > --- a/arch/x86/mm/extable.c > +++ b/arch/x86/mm/extable.c > @@ -45,6 +45,7 @@ bool ex_handler_refcount(const struct ex > { > /* First unconditionally saturate the refcount. */ > *(int *)regs->cx = INT_MIN / 2; > + trace_printk("*(int *)regs->cx = %d\n", *(int *)regs->cx); Just for fun, can you print out *(int *)regs->cx before the assignment too? > > /* > * Strictly speaking, this reports the fixup destination, not > --- a/include/net/addrconf.h > +++ b/include/net/addrconf.h > @@ -321,6 +321,18 @@ static inline struct inet6_dev *in6_dev_ > return idev; > } > > +static inline struct inet6_dev *in6_dev_getx(const struct net_device *dev) > +{ > + struct inet6_dev *idev; > + > + rcu_read_lock(); > + idev = rcu_dereference(dev->ip6_ptr); > + if (idev) > + refcount_inc_x(&idev->refcnt); > + rcu_read_unlock(); > + return idev; > +} > + > static inline struct neigh_parms *__in6_dev_nd_parms_get_rcu(const struct net_device *dev) > { > struct inet6_dev *idev = __in6_dev_get(dev); > --- a/net/ipv6/route.c > +++ b/net/ipv6/route.c > @@ -4041,12 +4041,12 @@ void __init ip6_route_init_special_entri > * the loopback reference in rt6_info will not be taken, do it > * manually for init_net */ > init_net.ipv6.ip6_null_entry->dst.dev = init_net.loopback_dev; > - init_net.ipv6.ip6_null_entry->rt6i_idev = in6_dev_get(init_net.loopback_dev); > + init_net.ipv6.ip6_null_entry->rt6i_idev = in6_dev_getx(init_net.loopback_dev); > #ifdef CONFIG_IPV6_MULTIPLE_TABLES > init_net.ipv6.ip6_prohibit_entry->dst.dev = init_net.loopback_dev; > - init_net.ipv6.ip6_prohibit_entry->rt6i_idev = in6_dev_get(init_net.loopback_dev); > + init_net.ipv6.ip6_prohibit_entry->rt6i_idev = in6_dev_getx(init_net.loopback_dev); > init_net.ipv6.ip6_blk_hole_entry->dst.dev = init_net.loopback_dev; > - init_net.ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_get(init_net.loopback_dev); > + init_net.ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_getx(init_net.loopback_dev); > #endif > } > -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection 2017-09-01 17:12 ` Kees Cook @ 2017-09-01 17:52 ` Mike Galbraith 2017-09-01 18:58 ` Kees Cook 0 siblings, 1 reply; 15+ messages in thread From: Mike Galbraith @ 2017-09-01 17:52 UTC (permalink / raw) To: Kees Cook Cc: David S. Miller, Peter Zijlstra, LKML, Ingo Molnar, Reshetova, Elena, Network Development On Fri, 2017-09-01 at 10:12 -0700, Kees Cook wrote: > On Fri, Sep 1, 2017 at 6:09 AM, Mike Galbraith <efault@gmx.de> wrote: > > On Fri, 2017-09-01 at 08:57 +0200, Mike Galbraith wrote: > >> On Thu, 2017-08-31 at 11:45 -0700, Kees Cook wrote: > >> > On Thu, Aug 31, 2017 at 10:19 AM, Mike Galbraith <efault@gmx.de> wrote: > >> > > On Thu, 2017-08-31 at 10:00 -0700, Kees Cook wrote: > >> > >> > >> > >> Oh! So it's gcc-version sensitive? That's alarming. Is this mapping correct: > >> > >> > >> > >> 4.8.5: WARN, eventual kernel hang > >> > >> 6.3.1, 7.0.1: WARN, but continues working > >> > > > >> > > Yeah, that's correct. I find that troubling, simply because this gcc > >> > > version has been through one hell of a lot of kernels with me. Yeah, I > >> > > know, that doesn't exempt it from having bugs, but color me suspicious. > >> > > >> > I still can't hit this with a 4.8.5 build. :( > >> > > >> > With _RATELIMIT removed, this should, in theory, report whatever goes > >> > negative first... > >> > >> I applied the other patch you posted, and built with gcc-6.3.1 to > >> remove the gcc-4.8.5 aspect. Look below the resulting splat. > > > > Grr, that one has a in6_dev_getx() line missing for the first > > increment, where things go pear shaped. > > > > With that added, looking at counter both before, and after incl, with a > > trace_printk() in the exception handler showing it doing its saturate > > thing, irqs disabled across the whole damn refcount_inc(), and even > > booting box nr_cpus=1 for extra credit... > > > > HTH can that first refcount_inc() get there? > > > > # tracer: nop > > # > > # _-----=> irqs-off > > # / _----=> need-resched > > # | / _---=> hardirq/softirq > > # || / _--=> preempt-depth > > # ||| / delay > > # TASK-PID CPU# |||| TIMESTAMP FUNCTION > > # | | | |||| | | > > systemd-1 [000] d..1 1.937284: in6_dev_getx: PRE refs.counter:3 > > systemd-1 [000] d..1 1.937295: ex_handler_refcount: *(int *)regs->cx = -1073741824 > > systemd-1 [000] d..1 1.937296: in6_dev_getx: POST refs.counter:-1073741824 > > O_o > > Can you paste the disassembly of in6_dev_getx? I can't understand how > we're landing in the exception handler. I was hoping you'd say that. 0xffffffff816b2f72 <+0>: push %rbp 0xffffffff816b2f73 <+1>: mov %rsp,%rbp 0xffffffff816b2f76 <+4>: push %r12 0xffffffff816b2f78 <+6>: push %rbx 0xffffffff816b2f79 <+7>: incl %gs:0x7e95a2d0(%rip) # 0xd250 <__preempt_count> 0xffffffff816b2f80 <+14>: mov 0x308(%rdi),%rbx 0xffffffff816b2f87 <+21>: test %rbx,%rbx 0xffffffff816b2f8a <+24>: je 0xffffffff816b2feb <in6_dev_getx+121> 0xffffffff816b2f8c <+26>: callq *0xffffffff81c35a00 0xffffffff816b2f93 <+33>: mov %rax,%r12 0xffffffff816b2f96 <+36>: callq *0xffffffff81c35a10 0xffffffff816b2f9d <+43>: mov 0x769ad4(%rip),%rsi # 0xffffffff81e1ca78 <trace_printk_fmt.21733> 0xffffffff816b2fa4 <+50>: mov 0xf0(%rbx),%edx 0xffffffff816b2faa <+56>: mov $0xffffffff816b2f8c,%rdi 0xffffffff816b2fb1 <+63>: callq 0xffffffff81171fc0 <__trace_bprintk> 0xffffffff816b2fb6 <+68>: lock incl 0xf0(%rbx) 0xffffffff816b2fbd <+75>: js 0xffffffff816b2fbf <in6_dev_getx+77> 0xffffffff816b2fbf <+77>: lea 0xf0(%rbx),%rcx 0xffffffff816b2fc6 <+84>: (bad) 0xffffffff816b2fc8 <+86>: mov 0x769a99(%rip),%rsi # 0xffffffff81e1ca68 <trace_printk_fmt.21744> 0xffffffff816b2fcf <+93>: mov 0xf0(%rbx),%edx 0xffffffff816b2fd5 <+99>: mov $0xffffffff816b2f8c,%rdi 0xffffffff816b2fdc <+106>: callq 0xffffffff81171fc0 <__trace_bprintk> 0xffffffff816b2fe1 <+111>: mov %r12,%rdi 0xffffffff816b2fe4 <+114>: callq *0xffffffff81c35a08 0xffffffff816b2feb <+121>: decl %gs:0x7e95a25e(%rip) # 0xd250 <__preempt_count> 0xffffffff816b2ff2 <+128>: mov %rbx,%rax 0xffffffff816b2ff5 <+131>: pop %rbx 0xffffffff816b2ff6 <+132>: pop %r12 0xffffffff816b2ff8 <+134>: pop %rbp 0xffffffff816b2ff9 <+135>: retq I don't get the section business at all, +75 looks to me like we're gonna trap no matter what.. as we appear to be doing. > > --- a/arch/x86/include/asm/refcount.h > > +++ b/arch/x86/include/asm/refcount.h > > @@ -55,6 +55,20 @@ static __always_inline void refcount_inc > > : : "cc", "cx"); > > } > > > > +static __always_inline void refcount_inc_x(refcount_t *r) > > +{ > > + unsigned long flags; > > + > > + local_irq_save(flags); > > + trace_printk("PRE refs.counter:%d\n", r->refs.counter); > > + asm volatile(LOCK_PREFIX "incl %0\n\t" > > + REFCOUNT_CHECK_LT_ZERO > > + : [counter] "+m" (r->refs.counter) > > + : : "cc", "cx"); > > Does this need an explicit "memory" added to the clobbers line here? > This isn't present in the atomic_inc() implementation, but maybe > something confuses gcc in this case into ignoring the "+m" marking? I thought about adding that (hail mary), but resisted. > > + trace_printk("POST refs.counter:%d\n", r->refs.counter); > > + local_irq_restore(flags); > > +} > > + > > static __always_inline void refcount_dec(refcount_t *r) > > { > > asm volatile(LOCK_PREFIX "decl %0\n\t" > > --- a/arch/x86/mm/extable.c > > +++ b/arch/x86/mm/extable.c > > @@ -45,6 +45,7 @@ bool ex_handler_refcount(const struct ex > > { > > /* First unconditionally saturate the refcount. */ > > *(int *)regs->cx = INT_MIN / 2; > > + trace_printk("*(int *)regs->cx = %d\n", *(int *)regs->cx); > > Just for fun, can you print out *(int *)regs->cx before the assignment too? Sure, tomorrow. -Mike ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection 2017-09-01 17:52 ` Mike Galbraith @ 2017-09-01 18:58 ` Kees Cook 2017-09-01 19:24 ` Mike Galbraith 2017-09-01 19:40 ` Kees Cook 0 siblings, 2 replies; 15+ messages in thread From: Kees Cook @ 2017-09-01 18:58 UTC (permalink / raw) To: Mike Galbraith Cc: David S. Miller, Peter Zijlstra, LKML, Ingo Molnar, Reshetova, Elena, Network Development On Fri, Sep 1, 2017 at 10:52 AM, Mike Galbraith <efault@gmx.de> wrote: > On Fri, 2017-09-01 at 10:12 -0700, Kees Cook wrote: >> On Fri, Sep 1, 2017 at 6:09 AM, Mike Galbraith <efault@gmx.de> wrote: >> > On Fri, 2017-09-01 at 08:57 +0200, Mike Galbraith wrote: >> >> On Thu, 2017-08-31 at 11:45 -0700, Kees Cook wrote: >> >> > On Thu, Aug 31, 2017 at 10:19 AM, Mike Galbraith <efault@gmx.de> wrote: >> >> > > On Thu, 2017-08-31 at 10:00 -0700, Kees Cook wrote: >> >> > >> >> >> > >> Oh! So it's gcc-version sensitive? That's alarming. Is this mapping correct: >> >> > >> >> >> > >> 4.8.5: WARN, eventual kernel hang >> >> > >> 6.3.1, 7.0.1: WARN, but continues working >> >> > > >> >> > > Yeah, that's correct. I find that troubling, simply because this gcc >> >> > > version has been through one hell of a lot of kernels with me. Yeah, I >> >> > > know, that doesn't exempt it from having bugs, but color me suspicious. >> >> > >> >> > I still can't hit this with a 4.8.5 build. :( >> >> > >> >> > With _RATELIMIT removed, this should, in theory, report whatever goes >> >> > negative first... >> >> >> >> I applied the other patch you posted, and built with gcc-6.3.1 to >> >> remove the gcc-4.8.5 aspect. Look below the resulting splat. >> > >> > Grr, that one has a in6_dev_getx() line missing for the first >> > increment, where things go pear shaped. >> > >> > With that added, looking at counter both before, and after incl, with a >> > trace_printk() in the exception handler showing it doing its saturate >> > thing, irqs disabled across the whole damn refcount_inc(), and even >> > booting box nr_cpus=1 for extra credit... >> > >> > HTH can that first refcount_inc() get there? >> > >> > # tracer: nop >> > # >> > # _-----=> irqs-off >> > # / _----=> need-resched >> > # | / _---=> hardirq/softirq >> > # || / _--=> preempt-depth >> > # ||| / delay >> > # TASK-PID CPU# |||| TIMESTAMP FUNCTION >> > # | | | |||| | | >> > systemd-1 [000] d..1 1.937284: in6_dev_getx: PRE refs.counter:3 >> > systemd-1 [000] d..1 1.937295: ex_handler_refcount: *(int *)regs->cx = -1073741824 >> > systemd-1 [000] d..1 1.937296: in6_dev_getx: POST refs.counter:-1073741824 >> >> O_o >> >> Can you paste the disassembly of in6_dev_getx? I can't understand how >> we're landing in the exception handler. > > I was hoping you'd say that. > > 0xffffffff816b2f72 <+0>: push %rbp > 0xffffffff816b2f73 <+1>: mov %rsp,%rbp > 0xffffffff816b2f76 <+4>: push %r12 > 0xffffffff816b2f78 <+6>: push %rbx > 0xffffffff816b2f79 <+7>: incl %gs:0x7e95a2d0(%rip) # 0xd250 <__preempt_count> > 0xffffffff816b2f80 <+14>: mov 0x308(%rdi),%rbx > 0xffffffff816b2f87 <+21>: test %rbx,%rbx > 0xffffffff816b2f8a <+24>: je 0xffffffff816b2feb <in6_dev_getx+121> > 0xffffffff816b2f8c <+26>: callq *0xffffffff81c35a00 > 0xffffffff816b2f93 <+33>: mov %rax,%r12 > 0xffffffff816b2f96 <+36>: callq *0xffffffff81c35a10 > 0xffffffff816b2f9d <+43>: mov 0x769ad4(%rip),%rsi # 0xffffffff81e1ca78 <trace_printk_fmt.21733> > 0xffffffff816b2fa4 <+50>: mov 0xf0(%rbx),%edx > 0xffffffff816b2faa <+56>: mov $0xffffffff816b2f8c,%rdi > 0xffffffff816b2fb1 <+63>: callq 0xffffffff81171fc0 <__trace_bprintk> > 0xffffffff816b2fb6 <+68>: lock incl 0xf0(%rbx) > 0xffffffff816b2fbd <+75>: js 0xffffffff816b2fbf <in6_dev_getx+77> > 0xffffffff816b2fbf <+77>: lea 0xf0(%rbx),%rcx > 0xffffffff816b2fc6 <+84>: (bad) > 0xffffffff816b2fc8 <+86>: mov 0x769a99(%rip),%rsi # 0xffffffff81e1ca68 <trace_printk_fmt.21744> > 0xffffffff816b2fcf <+93>: mov 0xf0(%rbx),%edx > 0xffffffff816b2fd5 <+99>: mov $0xffffffff816b2f8c,%rdi > 0xffffffff816b2fdc <+106>: callq 0xffffffff81171fc0 <__trace_bprintk> > 0xffffffff816b2fe1 <+111>: mov %r12,%rdi > 0xffffffff816b2fe4 <+114>: callq *0xffffffff81c35a08 > 0xffffffff816b2feb <+121>: decl %gs:0x7e95a25e(%rip) # 0xd250 <__preempt_count> > 0xffffffff816b2ff2 <+128>: mov %rbx,%rax > 0xffffffff816b2ff5 <+131>: pop %rbx > 0xffffffff816b2ff6 <+132>: pop %r12 > 0xffffffff816b2ff8 <+134>: pop %rbp > 0xffffffff816b2ff9 <+135>: retq > > I don't get the section business at all, +75 looks to me like we're > gonna trap no matter what.. as we appear to be doing. The section stuff is supposed to be a trick to push the error case off into the .text.unlikely area to avoid needing a jmp over the handler and with possibly some redundancy removal done by the compiler (though this appears to be rather limited) if it notices a bunch of error paths are the same. However, in your disassembly, it's inline (!!) in the code, as if "pushsection" and "popsection" were entirely ignored. And when I make my own in6_dev_getx(), I see the same disassembly: 0xffffffff818a757b <+181>: lock incl 0x1e0(%rbx) 0xffffffff818a7582 <+188>: js 0xffffffff818a7584 <in6_dev_getx+190> 0xffffffff818a7584 <+190>: lea 0x1e0(%rbx),%rcx 0xffffffff818a758b <+197>: (bad) Which is VERY different from how it looks in other places! e.g. from lkdtm_REFCOUNT_INC_SATURATED: 0xffffffff815657df <+47>: lock incl -0xc(%rbp) 0xffffffff815657e3 <+51>: js 0xffffffff81565cac ... 0xffffffff81565cac: lea -0xc(%rbp),%rcx 0xffffffff81565cb0: (bad) So, at least I can reproduce this in the build now. I must not be exercising these paths. FWIW, this is with Ubuntu's 6.3.0 gcc. I'll try to figure out what's going on here... -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection 2017-09-01 18:58 ` Kees Cook @ 2017-09-01 19:24 ` Mike Galbraith 2017-09-01 19:40 ` Kees Cook 1 sibling, 0 replies; 15+ messages in thread From: Mike Galbraith @ 2017-09-01 19:24 UTC (permalink / raw) To: Kees Cook Cc: David S. Miller, Peter Zijlstra, LKML, Ingo Molnar, Reshetova, Elena, Network Development On Fri, 2017-09-01 at 11:58 -0700, Kees Cook wrote: > > The section stuff is supposed to be a trick to push the error case off > into the .text.unlikely area to avoid needing a jmp over the handler > and with possibly some redundancy removal done by the compiler (though > this appears to be rather limited) if it notices a bunch of error > paths are the same. However, in your disassembly, it's inline (!!) in > the code, as if "pushsection" and "popsection" were entirely ignored. > > And when I make my own in6_dev_getx(), I see the same disassembly: > > 0xffffffff818a757b <+181>: lock incl 0x1e0(%rbx) > 0xffffffff818a7582 <+188>: js 0xffffffff818a7584 <in6_dev_getx+190> > 0xffffffff818a7584 <+190>: lea 0x1e0(%rbx),%rcx > 0xffffffff818a758b <+197>: (bad) > > Which is VERY different from how it looks in other places! > > e.g. from lkdtm_REFCOUNT_INC_SATURATED: > > 0xffffffff815657df <+47>: lock incl -0xc(%rbp) > 0xffffffff815657e3 <+51>: js 0xffffffff81565cac > ... > 0xffffffff81565cac: lea -0xc(%rbp),%rcx > 0xffffffff81565cb0: (bad) > > So, at least I can reproduce this in the build now. I must not be > exercising these paths. FWIW, this is with Ubuntu's 6.3.0 gcc. > > I'll try to figure out what's going on here... Heh, make in6_dev_getx() __always_inline. swapper/0-1 [000] d..1 1.438587: ip6_route_init_special_entries: PRE refs.counter:3 swapper/0-1 [000] d..1 1.438590: ip6_route_init_special_entries: POST refs.counter:4 swapper/0-1 [000] d..1 1.438591: ip6_route_init_special_entries: PRE refs.counter:4 swapper/0-1 [000] d..1 1.438592: ip6_route_init_special_entries: POST refs.counter:5 swapper/0-1 [000] d..1 1.438592: ip6_route_init_special_entries: PRE refs.counter:5 swapper/0-1 [000] d..1 1.438593: ip6_route_init_special_entries: POST refs.counter:6 -Mike ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection 2017-09-01 18:58 ` Kees Cook 2017-09-01 19:24 ` Mike Galbraith @ 2017-09-01 19:40 ` Kees Cook 1 sibling, 0 replies; 15+ messages in thread From: Kees Cook @ 2017-09-01 19:40 UTC (permalink / raw) To: Mike Galbraith Cc: David S. Miller, Peter Zijlstra, LKML, Ingo Molnar, Reshetova, Elena, Network Development On Fri, Sep 1, 2017 at 11:58 AM, Kees Cook <keescook@chromium.org> wrote: > On Fri, Sep 1, 2017 at 10:52 AM, Mike Galbraith <efault@gmx.de> wrote: >> On Fri, 2017-09-01 at 10:12 -0700, Kees Cook wrote: >>> On Fri, Sep 1, 2017 at 6:09 AM, Mike Galbraith <efault@gmx.de> wrote: >>> > On Fri, 2017-09-01 at 08:57 +0200, Mike Galbraith wrote: >>> >> On Thu, 2017-08-31 at 11:45 -0700, Kees Cook wrote: >>> >> > On Thu, Aug 31, 2017 at 10:19 AM, Mike Galbraith <efault@gmx.de> wrote: >>> >> > > On Thu, 2017-08-31 at 10:00 -0700, Kees Cook wrote: >>> >> > >> >>> >> > >> Oh! So it's gcc-version sensitive? That's alarming. Is this mapping correct: >>> >> > >> >>> >> > >> 4.8.5: WARN, eventual kernel hang >>> >> > >> 6.3.1, 7.0.1: WARN, but continues working >>> >> > > >>> >> > > Yeah, that's correct. I find that troubling, simply because this gcc >>> >> > > version has been through one hell of a lot of kernels with me. Yeah, I >>> >> > > know, that doesn't exempt it from having bugs, but color me suspicious. >>> >> > >>> >> > I still can't hit this with a 4.8.5 build. :( >>> >> > >>> >> > With _RATELIMIT removed, this should, in theory, report whatever goes >>> >> > negative first... >>> >> >>> >> I applied the other patch you posted, and built with gcc-6.3.1 to >>> >> remove the gcc-4.8.5 aspect. Look below the resulting splat. >>> > >>> > Grr, that one has a in6_dev_getx() line missing for the first >>> > increment, where things go pear shaped. >>> > >>> > With that added, looking at counter both before, and after incl, with a >>> > trace_printk() in the exception handler showing it doing its saturate >>> > thing, irqs disabled across the whole damn refcount_inc(), and even >>> > booting box nr_cpus=1 for extra credit... >>> > >>> > HTH can that first refcount_inc() get there? >>> > >>> > # tracer: nop >>> > # >>> > # _-----=> irqs-off >>> > # / _----=> need-resched >>> > # | / _---=> hardirq/softirq >>> > # || / _--=> preempt-depth >>> > # ||| / delay >>> > # TASK-PID CPU# |||| TIMESTAMP FUNCTION >>> > # | | | |||| | | >>> > systemd-1 [000] d..1 1.937284: in6_dev_getx: PRE refs.counter:3 >>> > systemd-1 [000] d..1 1.937295: ex_handler_refcount: *(int *)regs->cx = -1073741824 >>> > systemd-1 [000] d..1 1.937296: in6_dev_getx: POST refs.counter:-1073741824 >>> >>> O_o >>> >>> Can you paste the disassembly of in6_dev_getx? I can't understand how >>> we're landing in the exception handler. >> >> I was hoping you'd say that. >> >> 0xffffffff816b2f72 <+0>: push %rbp >> 0xffffffff816b2f73 <+1>: mov %rsp,%rbp >> 0xffffffff816b2f76 <+4>: push %r12 >> 0xffffffff816b2f78 <+6>: push %rbx >> 0xffffffff816b2f79 <+7>: incl %gs:0x7e95a2d0(%rip) # 0xd250 <__preempt_count> >> 0xffffffff816b2f80 <+14>: mov 0x308(%rdi),%rbx >> 0xffffffff816b2f87 <+21>: test %rbx,%rbx >> 0xffffffff816b2f8a <+24>: je 0xffffffff816b2feb <in6_dev_getx+121> >> 0xffffffff816b2f8c <+26>: callq *0xffffffff81c35a00 >> 0xffffffff816b2f93 <+33>: mov %rax,%r12 >> 0xffffffff816b2f96 <+36>: callq *0xffffffff81c35a10 >> 0xffffffff816b2f9d <+43>: mov 0x769ad4(%rip),%rsi # 0xffffffff81e1ca78 <trace_printk_fmt.21733> >> 0xffffffff816b2fa4 <+50>: mov 0xf0(%rbx),%edx >> 0xffffffff816b2faa <+56>: mov $0xffffffff816b2f8c,%rdi >> 0xffffffff816b2fb1 <+63>: callq 0xffffffff81171fc0 <__trace_bprintk> >> 0xffffffff816b2fb6 <+68>: lock incl 0xf0(%rbx) >> 0xffffffff816b2fbd <+75>: js 0xffffffff816b2fbf <in6_dev_getx+77> >> 0xffffffff816b2fbf <+77>: lea 0xf0(%rbx),%rcx >> 0xffffffff816b2fc6 <+84>: (bad) >> 0xffffffff816b2fc8 <+86>: mov 0x769a99(%rip),%rsi # 0xffffffff81e1ca68 <trace_printk_fmt.21744> >> 0xffffffff816b2fcf <+93>: mov 0xf0(%rbx),%edx >> 0xffffffff816b2fd5 <+99>: mov $0xffffffff816b2f8c,%rdi >> 0xffffffff816b2fdc <+106>: callq 0xffffffff81171fc0 <__trace_bprintk> >> 0xffffffff816b2fe1 <+111>: mov %r12,%rdi >> 0xffffffff816b2fe4 <+114>: callq *0xffffffff81c35a08 >> 0xffffffff816b2feb <+121>: decl %gs:0x7e95a25e(%rip) # 0xd250 <__preempt_count> >> 0xffffffff816b2ff2 <+128>: mov %rbx,%rax >> 0xffffffff816b2ff5 <+131>: pop %rbx >> 0xffffffff816b2ff6 <+132>: pop %r12 >> 0xffffffff816b2ff8 <+134>: pop %rbp >> 0xffffffff816b2ff9 <+135>: retq >> >> I don't get the section business at all, +75 looks to me like we're >> gonna trap no matter what.. as we appear to be doing. > > The section stuff is supposed to be a trick to push the error case off > into the .text.unlikely area to avoid needing a jmp over the handler > and with possibly some redundancy removal done by the compiler (though > this appears to be rather limited) if it notices a bunch of error > paths are the same. However, in your disassembly, it's inline (!!) in > the code, as if "pushsection" and "popsection" were entirely ignored. > > And when I make my own in6_dev_getx(), I see the same disassembly: > > 0xffffffff818a757b <+181>: lock incl 0x1e0(%rbx) > 0xffffffff818a7582 <+188>: js 0xffffffff818a7584 <in6_dev_getx+190> > 0xffffffff818a7584 <+190>: lea 0x1e0(%rbx),%rcx > 0xffffffff818a758b <+197>: (bad) > > Which is VERY different from how it looks in other places! Found it. If the compiler already pushed the entire function into .text.unlikely, x86-refcount's .pushsection doesn't do any good (obviously). Durrr. .section .text.unlikely,"ax",@progbits .type in6_dev_getx, @function in6_dev_getx: .LFB4673: .loc 2 4128 0 .cfi_startproc ... lock; incl 480(%rbx) js 111f .pushsection .text.unlikely 111: lea 480(%rbx), %rcx 112: .byte 0x0f, 0xff .popsection 113: I will get this fixed. Thank you again for helping track this down! -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection 2017-08-31 13:58 ` Mike Galbraith 2017-08-31 17:00 ` Kees Cook @ 2017-08-31 19:28 ` Kees Cook 1 sibling, 0 replies; 15+ messages in thread From: Kees Cook @ 2017-08-31 19:28 UTC (permalink / raw) To: Mike Galbraith Cc: David S. Miller, Peter Zijlstra, LKML, Ingo Molnar, Reshetova, Elena, Network Development On Thu, Aug 31, 2017 at 6:58 AM, Mike Galbraith <efault@gmx.de> wrote: > gdb) list *in6_dev_get+0x10 > 0xffffffff8166d3d0 is in in6_dev_get (./include/net/addrconf.h:318). > 313 { > 314 struct inet6_dev *idev; > 315 > 316 rcu_read_lock(); > 317 idev = rcu_dereference(dev->ip6_ptr); > 318 if (idev) > 319 refcount_inc(&idev->refcnt); > 320 rcu_read_unlock(); > 321 return idev; > 322 And this is a completely different refcount from the other that tripped. This one is quite simple, too, though I see it uses refcount_dec(), which is a path to saturation. I've sent a patch to try to clarify this further... -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-09-01 19:40 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1503996623.8323.20.camel@gmx.de> [not found] ` <CAGXu5jLYHy6Damtg_Pc4=G76OUiMV3fcY9+Ru1-Lmiq6VWEgsw@mail.gmail.com> [not found] ` <1504025721.6024.25.camel@gmx.de> [not found] ` <1504030207.6560.0.camel@gmx.de> [not found] ` <CAGXu5jKSWiZ8NkgUqO6G8fs5R06bMtdFBmZz9MMX-HAk8nXXNw@mail.gmail.com> [not found] ` <1504069332.8352.3.camel@gmx.de> [not found] ` <CAGXu5jJim=r8b+y1qQXtuzm9fPyv1tp9ANebxbCc9ic2c83_Mg@mail.gmail.com> [not found] ` <1504113212.5852.6.camel@gmx.de> [not found] ` <CAGXu5jJd80-0PJvB51x9ZDETPSoAtRVQ7dQ6HWwd0DYpKBgPFA@mail.gmail.com> [not found] ` <1504115735.5852.11.camel@gmx.de> [not found] ` <CAGXu5j+BLj6_LPfKs-vts-mbyJHu8D--WrmWbiT8Zbt-iD65KA@mail.gmail.com> [not found] ` <CAGXu5j+neSYAqb8=r42=jQ0LsJTGjCSdpgc9Hv+ipyAFURbCdw@mail.gmail.com> [not found] ` <1504145389.23109.4.camel@gmx.de> [not found] ` <CAGXu5jLKXEDdSpxxVM5oHHUWKbSTozx4FZRn70bQm4tQAhMqUQ@mail.gmail.com> [not found] ` <1504149176.23109.9.camel@gmx.de> 2017-08-31 4:01 ` tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Kees Cook 2017-08-31 4:10 ` Kees Cook 2017-08-31 4:38 ` Mike Galbraith 2017-08-31 13:58 ` Mike Galbraith 2017-08-31 17:00 ` Kees Cook 2017-08-31 17:19 ` Mike Galbraith 2017-08-31 18:45 ` Kees Cook 2017-09-01 6:57 ` Mike Galbraith 2017-09-01 13:09 ` Mike Galbraith 2017-09-01 17:12 ` Kees Cook 2017-09-01 17:52 ` Mike Galbraith 2017-09-01 18:58 ` Kees Cook 2017-09-01 19:24 ` Mike Galbraith 2017-09-01 19:40 ` Kees Cook 2017-08-31 19:28 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).