| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 1 May 2008 14:46:28 -0400
From: Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
To: Ingo Molnar <mingo@...e.hu>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Luca Tettamanti <kronos.it@...il.com>
Cc: linux-kernel@...r.kernel.org
Subject: [PATCH] Fix SMP alternatives : use mutex instead of spinlock,
text_poke is sleepable
* Luca Tettamanti (kronos.it@...il.com) wrote:
> On Thu, May 1, 2008 at 5:02 PM, Luca Tettamanti <kronos.it@...il.com> wrote:
> > On Thu, May 1, 2008 at 3:04 PM, Mathieu Desnoyers
> > <mathieu.desnoyers@...ymtl.ca> wrote:
> > > * Luca Tettamanti (kronos.it@...il.com) wrote:
> > > > Hello,
> > > > with git current I'm seeing these warnings when bringing CPUs down:
> > > >
> > >
> > >
> > > Hi Luca,
> > >
> > > Does your tree include the patch posted here ?
> > >
> > > http://lkml.org/lkml/2008/4/19/139
> >
> > Nope, I was using vanilla kernel (smp_alt is still a spinlock). Will
> > test the patch ASAP.
>
> Tested the patch: works fine, and the system is stable after multiple
> up&down, I'm not experiencing a reboot.
>
> Luca
Ingo, Linus,
I guess fast-tracking pulling the following patch into mainline wouldn't
hurt. I just updated it so it applies to mainline.
Thanks,
Mathieu
Fix SMP alternatives : use mutex instead of spinlock, text_poke is sleepable
* Pekka Paalanen (pq@....fi) wrote:
> On Mon, 14 Apr 2008 08:57:13 +0200
> Ingo Molnar <mingo@...e.hu> wrote:
>
> > Pekka Paalanen wrote:
> > > When I tested this patch on Intel Core 2 Duo, enter_uniprocessor()
> > > triggered the following kernel bug:
> > >
> > > Linux version 2.6.25-rc8-sched-devel.git-x86-latest.git (paalanen@...00006)
> > > (gcc version 4.1.2 (Gentoo 4.1.2 p1.0.1)) #2 SMP PREEMPT Sun Apr 13
> > > 22:09:03 EEST 2008
> > > ...
> > > in mmio_trace_init
> > > mmiotrace: Disabling non-boot CPUs...
> > > CPU 1 is now offline
> > > lockdep: fixing up alternatives.
> > > SMP alternatives: switching to UP code
> > > BUG: sleeping function called from invalid context at mm/slab.c:3053
> > > in_atomic():1, irqs_disabled():0
> > > 5 locks held by bash/4423:
> > > #0: (trace_types_lock){--..}, at: [<ffffffff8026442f>] tracing_set_trace_write+0x93/0x11a
> > > #1: (mmiotrace_mutex){--..}, at: [<ffffffff802251e0>] enable_mmiotrace+0x17/0x142
> > > #2: (cpu_add_remove_lock){--..}, at: [<ffffffff802580e5>] cpu_maps_update_begin+0x12/0x14
> > > #3: (&cpu_hotplug.lock){--..}, at: [<ffffffff8025814f>] cpu_hotplug_begin+0x39/0x9f
> > > #4: (smp_alt){--..}, at: [<ffffffff80211bd9>] alternatives_smp_switch+0x66/0x1ab
> > > Pid: 4423, comm: bash Not tainted 2.6.25-rc8-sched-devel.git-x86-latest.git #2
> > >
> > > Call Trace:
> > > [<ffffffff802520c0>] ? __debug_show_held_locks+0x22/0x24
> > > [<ffffffff8022d292>] __might_sleep+0xd9/0xdb
> > > [<ffffffff80287326>] cache_alloc_debugcheck_before+0x23/0x32
> > > [<ffffffff80287a32>] __kmalloc+0x34/0xa5
> > > [<ffffffff80209393>] ? clear_ti_thread_flag+0x10/0x17
> > > [<ffffffff8027d7f6>] kmalloc_node+0x9/0xb
> > > [<ffffffff8027d8e0>] __get_vm_area_node+0xa2/0x1cb
> > > [<ffffffff80209393>] ? clear_ti_thread_flag+0x10/0x17
> > > [<ffffffff8027da41>] __get_vm_area+0x13/0x15
> > > [<ffffffff8027da60>] get_vm_area+0x1d/0x1f
> > > [<ffffffff8027e14c>] vmap+0x2a/0x5c
> > > [<ffffffff8021191b>] text_poke+0xaa/0x136
> > > [<ffffffff804cba2b>] ? _etext+0x0/0x5
> > > [<ffffffff802119f6>] alternatives_smp_unlock+0x4f/0x63
> > > [<ffffffff80211ce1>] alternatives_smp_switch+0x16e/0x1ab
> > > [<ffffffff8021b163>] __cpu_die+0x53/0x7d
> > > [<ffffffff802583e2>] _cpu_down+0x195/0x26c
> > > [<ffffffff802585ca>] cpu_down+0x26/0x36
> > > [<ffffffff80225270>] enable_mmiotrace+0xa7/0x142
> > > [<ffffffff80266b8d>] mmio_trace_init+0x3c/0x40
> > > [<ffffffff8026448e>] tracing_set_trace_write+0xf2/0x11a
> > > [<ffffffff80327fac>] ? security_file_permission+0x11/0x13
> > > [<ffffffff8028b047>] vfs_write+0xa7/0xe1
> > > [<ffffffff8028b13b>] sys_write+0x47/0x6d
> > > [<ffffffff8020b4db>] system_call_after_swapgs+0x7b/0x80
> > >
> > > mmiotrace: CPU1 is down.
> > > mmiotrace: enabled.
> > >
> > > Is this my fault, or is there a bug somewhere else? The kernel tree is
> > > sched-devel/latest git from 12th April, IIRC.
> >
> > there's no known bug of sched-devel/latest of this kind (or any known
> > bug for that matter).
> >
> > i suspect the bug is that you bring the CPU down from an atomic
> > (spinlocked or irq disabled) context.
>
> I have been eyeballing the code in current sched-devel/latest and there's
> something I think I found.
>
> This is the beginning of my enter_uniprocessor() which is called from
> enable_mmiotrace() seen in the backtrace.
>
> static void enter_uniprocessor(void)
> {
> int cpu;
> int err;
>
> get_online_cpus();
> downed_cpus = cpu_online_map;
> cpu_clear(first_cpu(cpu_online_map), downed_cpus);
> if (num_online_cpus() > 1)
> pr_notice(NAME "Disabling non-boot CPUs...\n");
> put_online_cpus();
>
> for_each_cpu_mask(cpu, downed_cpus) {
> err = cpu_down(cpu);
>
> The function get_online_cpus() calls might_sleep(), so at that
> point everything is fine.
>
> Following the backtrace, we come to alternatives_smp_switch(),
> which does
> spin_lock(&smp_alt);
> and then continues eventually into this function:
>
> void *__kprobes text_poke(void *addr, const void *opcode, size_t len)
> {
> unsigned long flags;
> char *vaddr;
> int nr_pages = 2;
>
> BUG_ON(len > sizeof(long));
> BUG_ON((((long)addr + len - 1) & ~(sizeof(long) - 1))
> - ((long)addr & ~(sizeof(long) - 1)));
> if (kernel_text_address((unsigned long)addr)) {
> struct page *pages[2] = { virt_to_page(addr),
> virt_to_page(addr + PAGE_SIZE) };
> if (!pages[1])
> nr_pages = 1;
> vaddr = vmap(pages, nr_pages, VM_MAP, PAGE_KERNEL);
>
> After which we find ourselves in
>
> struct vm_struct *__get_vm_area(unsigned long size, unsigned long flags,
> unsigned long start, unsigned long end)
> {
> return __get_vm_area_node(size, flags, start, end, -1, GFP_KERNEL);
>
> and in __get_vm_area_node() there is
> area = kmalloc_node(sizeof(*area), gfp_mask & GFP_RECLAIM_MASK, node);
>
> where gfp_mask is now GFP_KERNEL. As far as I can tell, using SLAB,
> kmalloc_node() here is actually kmalloc(). Anyway, looks like we end up
> in __kmalloc with such flags that it may sleep.
>
> I have followed the code so that the path up to kmalloc_node() is
> possible, unless there are some "should never happen" conditions
> along the way, which I just cannot know of.
>
> Is this the bug?
>
Yep. I think using a mutex should fix it. There is no reason to use a
spinlock rather than a mutex here.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
CC: Pekka Paalanen <pq@....fi>
CC: Ingo Molnar <mingo@...e.hu>
CC: Steven Rostedt <rostedt@...dmis.org>
---
arch/x86/kernel/alternative.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
Index: linux-2.6-lttng/arch/x86/kernel/alternative.c
===================================================================
--- linux-2.6-lttng.orig/arch/x86/kernel/alternative.c 2008-05-01 14:41:09.000000000 -0400
+++ linux-2.6-lttng/arch/x86/kernel/alternative.c 2008-05-01 14:44:28.000000000 -0400
@@ -1,6 +1,6 @@
#include <linux/module.h>
#include <linux/sched.h>
-#include <linux/spinlock.h>
+#include <linux/mutex.h>
#include <linux/list.h>
#include <linux/kprobes.h>
#include <linux/mm.h>
@@ -279,7 +279,7 @@ struct smp_alt_module {
struct list_head next;
};
static LIST_HEAD(smp_alt_modules);
-static DEFINE_SPINLOCK(smp_alt);
+static DEFINE_MUTEX(smp_alt);
static int smp_mode = 1; /* protected by smp_alt */
void alternatives_smp_module_add(struct module *mod, char *name,
@@ -312,12 +312,12 @@ void alternatives_smp_module_add(struct
__func__, smp->locks, smp->locks_end,
smp->text, smp->text_end, smp->name);
- spin_lock(&smp_alt);
+ mutex_lock(&smp_alt);
list_add_tail(&smp->next, &smp_alt_modules);
if (boot_cpu_has(X86_FEATURE_UP))
alternatives_smp_unlock(smp->locks, smp->locks_end,
smp->text, smp->text_end);
- spin_unlock(&smp_alt);
+ mutex_unlock(&smp_alt);
}
void alternatives_smp_module_del(struct module *mod)
@@ -327,17 +327,17 @@ void alternatives_smp_module_del(struct
if (smp_alt_once || noreplace_smp)
return;
- spin_lock(&smp_alt);
+ mutex_lock(&smp_alt);
list_for_each_entry(item, &smp_alt_modules, next) {
if (mod != item->mod)
continue;
list_del(&item->next);
- spin_unlock(&smp_alt);
+ mutex_unlock(&smp_alt);
DPRINTK("%s: %s\n", __func__, item->name);
kfree(item);
return;
}
- spin_unlock(&smp_alt);
+ mutex_unlock(&smp_alt);
}
void alternatives_smp_switch(int smp)
@@ -359,7 +359,7 @@ void alternatives_smp_switch(int smp)
return;
BUG_ON(!smp && (num_online_cpus() > 1));
- spin_lock(&smp_alt);
+ mutex_lock(&smp_alt);
/*
* Avoid unnecessary switches because it forces JIT based VMs to
@@ -383,7 +383,7 @@ void alternatives_smp_switch(int smp)
mod->text, mod->text_end);
}
smp_mode = smp;
- spin_unlock(&smp_alt);
+ mutex_unlock(&smp_alt);
}
#endif
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists