Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1722244 > unrolled thread
| Started by | Mike Galbraith <efault@gmx.de> |
|---|---|
| First post | 2017-08-29 11:00 +0200 |
| Last post | 2017-08-31 21:30 +0200 |
| Articles | 9 on this page of 29 — 2 participants |
Back to article view | Back to linux.kernel
tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Mike Galbraith <efault@gmx.de> - 2017-08-29 11:00 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Kees Cook <keescook@chromium.org> - 2017-08-29 18:00 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Mike Galbraith <efault@gmx.de> - 2017-08-29 20:20 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Kees Cook <keescook@chromium.org> - 2017-08-29 20:50 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Mike Galbraith <efault@gmx.de> - 2017-08-30 07:10 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Kees Cook <keescook@chromium.org> - 2017-08-30 18:40 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Mike Galbraith <efault@gmx.de> - 2017-08-30 19:20 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Kees Cook <keescook@chromium.org> - 2017-08-30 19:40 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Mike Galbraith <efault@gmx.de> - 2017-08-30 20:00 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Kees Cook <keescook@chromium.org> - 2017-08-30 21:20 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Kees Cook <keescook@chromium.org> - 2017-08-30 21:50 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Mike Galbraith <efault@gmx.de> - 2017-08-31 04:20 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Kees Cook <keescook@chromium.org> - 2017-08-31 04:30 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Mike Galbraith <efault@gmx.de> - 2017-08-31 05:20 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Kees Cook <keescook@chromium.org> - 2017-08-31 06:10 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Kees Cook <keescook@chromium.org> - 2017-08-31 06:20 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Mike Galbraith <efault@gmx.de> - 2017-08-31 06:40 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Mike Galbraith <efault@gmx.de> - 2017-08-31 16:00 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Kees Cook <keescook@chromium.org> - 2017-08-31 19:10 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Mike Galbraith <efault@gmx.de> - 2017-08-31 19:30 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Kees Cook <keescook@chromium.org> - 2017-08-31 20:50 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Mike Galbraith <efault@gmx.de> - 2017-09-01 09:00 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Mike Galbraith <efault@gmx.de> - 2017-09-01 15:20 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Kees Cook <keescook@chromium.org> - 2017-09-01 19:20 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Mike Galbraith <efault@gmx.de> - 2017-09-01 20:00 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Kees Cook <keescook@chromium.org> - 2017-09-01 21:00 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Mike Galbraith <efault@gmx.de> - 2017-09-01 21:30 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Kees Cook <keescook@chromium.org> - 2017-09-01 21:50 +0200
Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection Kees Cook <keescook@chromium.org> - 2017-08-31 21:30 +0200
Page 2 of 2 — ← Prev page 1 [2]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-08-31 20:50 +0200 |
| Message-ID | <ukCHp-Qu-29@gated-at.bofh.it> |
| In reply to | #1724444 |
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
[toc] | [prev] | [next] | [standalone]
| From | Mike Galbraith <efault@gmx.de> |
|---|---|
| Date | 2017-09-01 09:00 +0200 |
| Subject | Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection |
| Message-ID | <ukO5P-dZ-3@gated-at.bofh.it> |
| In reply to | #1724491 |
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
}
[toc] | [prev] | [next] | [standalone]
| From | Mike Galbraith <efault@gmx.de> |
|---|---|
| Date | 2017-09-01 15:20 +0200 |
| Subject | Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection |
| Message-ID | <ukU1A-5iT-9@gated-at.bofh.it> |
| In reply to | #1724811 |
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
}
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-09-01 19:20 +0200 |
| Message-ID | <ukXLP-86R-9@gated-at.bofh.it> |
| In reply to | #1725042 |
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
[toc] | [prev] | [next] | [standalone]
| From | Mike Galbraith <efault@gmx.de> |
|---|---|
| Date | 2017-09-01 20:00 +0200 |
| Subject | Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection |
| Message-ID | <ukYoy-8lL-9@gated-at.bofh.it> |
| In reply to | #1725245 |
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
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-09-01 21:00 +0200 |
| Message-ID | <ukZkD-vv-55@gated-at.bofh.it> |
| In reply to | #1725269 |
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
[toc] | [prev] | [next] | [standalone]
| From | Mike Galbraith <efault@gmx.de> |
|---|---|
| Date | 2017-09-01 21:30 +0200 |
| Subject | Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection |
| Message-ID | <ukZND-VC-17@gated-at.bofh.it> |
| In reply to | #1725282 |
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
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-09-01 21:50 +0200 |
| Message-ID | <ul070-15q-31@gated-at.bofh.it> |
| In reply to | #1725282 |
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
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-08-31 21:30 +0200 |
| Message-ID | <ukDk5-1mJ-3@gated-at.bofh.it> |
| In reply to | #1724263 |
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
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web