[PATCH 0/2 x86] fix some page faults in nmi if kmemcheck is enabled

[PATCH 0/2 x86] fix some page faults in nmi if kmemcheck is enabled

[Li Zhong]

If CONFIG_KMEMCHECK is enabled, there might be page faults in nmi if the
pages are marked as not present by kmemcheck, like following:

[    4.535803] WARNING: at arch/x86/mm/kmemcheck/kmemcheck.c:634 kmemcheck_fault+0xb9/0xd0()
[    4.633429] Hardware name: System x3650 M3 -[7945AC1]-
[    4.694710] Modules linked in:
[    4.731105] Pid: 1, comm: swapper/0 Not tainted 3.3.0-rc3 #15
[    4.799654] Call Trace:
[    4.828751]  <NMI>  [<ffffffff81042eca>] warn_slowpath_common+0x7a/0xb0
[    4.907713]  [<ffffffff81042f15>] warn_slowpath_null+0x15/0x20
[    4.977301]  [<ffffffff8103ce89>] kmemcheck_fault+0xb9/0xd0
[    5.043778]  [<ffffffff81551ba6>] do_page_fault+0x406/0x550
[    5.110252]  [<ffffffff8154e235>] page_fault+0x25/0x30
[    5.171535]  [<ffffffff8154f005>] ? nmi_handle.clone.1+0x75/0xc0
[    5.243202]  [<ffffffff8154efcf>] ? nmi_handle.clone.1+0x3f/0xc0
[    5.314867]  [<ffffffff8154ef90>] ? __die+0xf0/0xf0
[    5.373038]  [<ffffffff8154f15f>] do_nmi+0x10f/0x360
[    5.432243]  [<ffffffff8154e5cd>] restart_nmi+0x1a/0x1e
[    5.494565]  [<ffffffff8154e210>] ? general_protection+0x30/0x30
[    5.566234]  [<ffffffff8154e210>] ? general_protection+0x30/0x30
[    5.637898]  [<ffffffff8154e210>] ? general_protection+0x30/0x30
[    5.709566]  <<EOE>>  [<ffffffff8126d814>] ? rb_insert_color+0xa4/0x150
[    5.788526]  [<ffffffff8119d17b>] sysfs_link_sibling+0x8b/0x110
[    5.859155]  [<ffffffff8119dff1>] __sysfs_add_one+0xc1/0x100
[    5.926666]  [<ffffffff8119e056>] sysfs_add_one+0x26/0xd0
[    5.991065]  [<ffffffff8119cdf4>] sysfs_add_file_mode+0xc4/0x100
[    6.062731]  [<ffffffff8119fc41>] internal_create_group+0xc1/0x1a0
[    6.136473]  [<ffffffff8119fd4e>] sysfs_create_group+0xe/0x10
[    6.205026]  [<ffffffff81351c1a>] dpm_sysfs_add+0x2a/0xd0
[    6.269425]  [<ffffffff81349bf5>] device_add+0x5e5/0x730
[    6.332783]  [<ffffffff81349d59>] device_register+0x19/0x20
[    6.399260]  [<ffffffff8135b6b8>] add_memory_section+0x158/0x1e0
[    6.470927]  [<ffffffff81ca757e>] memory_dev_init+0x75/0x108
[    6.538439]  [<ffffffff81ca73a9>] driver_init+0x31/0x33
[    6.600762]  [<ffffffff81c72c68>] kernel_init+0xcc/0x169
[    6.664121]  [<ffffffff81555e64>] kernel_thread_helper+0x4/0x10
[    6.734749]  [<ffffffff81c72b9c>] ? start_kernel+0x3ab/0x3ab
[    6.802261]  [<ffffffff81555e60>] ? gs_change+0x13/0x13
[    6.864585] ---[ end trace a7919e7f17c0a725 ]---

These two patches tries to fix some of the problems by avoiding using the
non-present pages.

[PATCH 1/2 x86] fix page faults by nmi handler in nmi if kmemcheck is enabled

[Li Zhong]

This patch tries to
  change the nmi handler name from a pointer to an array. 
  use __get_free_pages instead of kzalloc if CONFIG_KMEMCHECK is
enabled.

Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
 arch/x86/kernel/nmi.c |   23 +++++++++++++++--------
 1 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 47acaf3..84aa03a 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -36,7 +36,7 @@ struct nmiaction {
 	struct list_head list;
 	nmi_handler_t handler;
 	unsigned int flags;
-	char *name;
+	char name[NMI_MAX_NAMELEN];
 };
 
 struct nmi_desc {
@@ -169,16 +169,18 @@ int register_nmi_handler(unsigned int type,
nmi_handler_t handler,
 	if (!handler)
 		return -EINVAL;
 
+#ifdef CONFIG_KMEMCHECK
+	action = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
+				get_order(sizeof(*action)));
+#else
 	action = kzalloc(sizeof(struct nmiaction), GFP_KERNEL);
+#endif
 	if (!action)
 		goto fail_action;
 
 	action->handler = handler;
 	action->flags = nmiflags;
-	action->name = kstrndup(devname, NMI_MAX_NAMELEN, GFP_KERNEL);
-	if (!action->name)
-		goto fail_action_name;
-
+	strncpy(action->name, devname, sizeof(action->name));
 	retval = __setup_nmi(type, action);
 
 	if (retval)
@@ -187,9 +189,11 @@ int register_nmi_handler(unsigned int type,
nmi_handler_t handler,
 	return retval;
 
 fail_setup_nmi:
-	kfree(action->name);
-fail_action_name:
+#ifdef CONFIG_KMEMCHECK
+	free_pages((unsigned long)action, get_order(sizeof(*action)));
+#else
 	kfree(action);
+#endif
 fail_action:	
 
 	return retval;
@@ -202,8 +206,11 @@ void unregister_nmi_handler(unsigned int type,
const char *name)
 
 	a = __free_nmi(type, name);
 	if (a) {
-		kfree(a->name);
+#ifdef CONFIG_KMEMCHECK
+		free_pages((unsigned long)a, get_order(sizeof(*a)));
+#else
 		kfree(a);
+#endif
 	}
 }
 
-- 
1.7.5.4

[PATCH 2/2 x86] fix page faults by perf events in nmi if kmemcheck is enabled

[Li Zhong]

This patch tries to use __get_free_pages for the perf_event and it's
callchain buffers if CONFIG_KMEMCHECK is enabled. 

Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
 kernel/events/callchain.c |   29 +++++++++++++++++++++++++++++
 kernel/events/core.c      |   13 +++++++++++++
 2 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c
index 6581a04..57f5eaa 100644
--- a/kernel/events/callchain.c
+++ b/kernel/events/callchain.c
@@ -38,13 +38,25 @@ static void release_callchain_buffers_rcu(struct
rcu_head *head)
 {
 	struct callchain_cpus_entries *entries;
 	int cpu;
+#ifdef CONFIG_KMEMCHECK
+	int size;
+
+	size = sizeof(struct perf_callchain_entry) * PERF_NR_CONTEXTS;
+#endif
 
 	entries = container_of(head, struct callchain_cpus_entries, rcu_head);
 
 	for_each_possible_cpu(cpu)
+#ifdef CONFIG_KMEMCHECK
+		free_pages((unsigned long)entries->cpu_entries[cpu],
+			get_order(size));
+	size = offsetof(struct callchain_cpus_entries,
cpu_entries[nr_cpu_ids]);
+	free_pages((unsigned long)entries, get_order(size));
+#else
 		kfree(entries->cpu_entries[cpu]);
 
 	kfree(entries);
+#endif
 }
 
 static void release_callchain_buffers(void)
@@ -69,15 +81,25 @@ static int alloc_callchain_buffers(void)
 	 */
 	size = offsetof(struct callchain_cpus_entries,
cpu_entries[nr_cpu_ids]);
 
+#ifdef CONFIG_KMEMCHECK
+	entries = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
+					get_order(size));
+#else
 	entries = kzalloc(size, GFP_KERNEL);
+#endif
 	if (!entries)
 		return -ENOMEM;
 
 	size = sizeof(struct perf_callchain_entry) * PERF_NR_CONTEXTS;
 
 	for_each_possible_cpu(cpu) {
+#ifdef CONFIG_KMEMCHECK
+		entries->cpu_entries[cpu] = (void *)__get_free_pages(
+				GFP_KERNEL, get_order(size));
+#else
 		entries->cpu_entries[cpu] = kmalloc_node(size, GFP_KERNEL,
 							 cpu_to_node(cpu));
+#endif
 		if (!entries->cpu_entries[cpu])
 			goto fail;
 	}
@@ -88,8 +110,15 @@ static int alloc_callchain_buffers(void)
 
 fail:
 	for_each_possible_cpu(cpu)
+#ifdef CONFIG_KMEMCHECK
+		free_pages((unsigned long)entries->cpu_entries[cpu],
+				get_order(size));
+	size = offsetof(struct callchain_cpus_entries,
cpu_entries[nr_cpu_ids]);
+	free_pages((unsigned long)entries, get_order(size));
+#else
 		kfree(entries->cpu_entries[cpu]);
 	kfree(entries);
+#endif
 
 	return -ENOMEM;
 }
diff --git a/kernel/events/core.c b/kernel/events/core.c
index ba36013..841c2ab 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2758,7 +2758,11 @@ static void free_event_rcu(struct rcu_head *head)
 	if (event->ns)
 		put_pid_ns(event->ns);
 	perf_event_free_filter(event);
+#ifdef CONFIG_KMEMCHECK
+	free_pages((unsigned long)event, get_order(sizeof(*event)));
+#else
 	kfree(event);
+#endif
 }
 
 static void ring_buffer_put(struct ring_buffer *rb);
@@ -5723,7 +5727,12 @@ perf_event_alloc(struct perf_event_attr *attr,
int cpu,
 			return ERR_PTR(-EINVAL);
 	}
 
+#ifdef CONFIG_KMEMCHECK
+	event = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
+				get_order(sizeof(*event)));
+#else
 	event = kzalloc(sizeof(*event), GFP_KERNEL);
+#endif
 	if (!event)
 		return ERR_PTR(-ENOMEM);
 
@@ -5810,7 +5819,11 @@ done:
 	if (err) {
 		if (event->ns)
 			put_pid_ns(event->ns);
+#ifdef CONFIG_KMEMCHECK
+		free_pages((unsigned long)event, get_order(sizeof(*event)));
+#else
 		kfree(event);
+#endif
 		return ERR_PTR(err);
 	}
 
-- 
1.7.5.4

Re: [PATCH 0/2 x86] fix some page faults in nmi if kmemcheck is enabled

[Peter Zijlstra]

On Mon, 2012-02-20 at 14:01 +0800, Li Zhong wrote:
> If CONFIG_KMEMCHECK is enabled, there might be page faults in nmi if the
> pages are marked as not present by kmemcheck, like following:
> 
> [    4.535803] WARNING: at arch/x86/mm/kmemcheck/kmemcheck.c:634 kmemcheck_fault+0xb9/0xd0()
> [    4.633429] Hardware name: System x3650 M3 -[7945AC1]-
> [    4.694710] Modules linked in:
> [    4.731105] Pid: 1, comm: swapper/0 Not tainted 3.3.0-rc3 #15
> [    4.799654] Call Trace:
> [    4.828751]  <NMI>  [<ffffffff81042eca>] warn_slowpath_common+0x7a/0xb0
> [    4.907713]  [<ffffffff81042f15>] warn_slowpath_null+0x15/0x20
> [    4.977301]  [<ffffffff8103ce89>] kmemcheck_fault+0xb9/0xd0
> [    5.043778]  [<ffffffff81551ba6>] do_page_fault+0x406/0x550
> [    5.110252]  [<ffffffff8154e235>] page_fault+0x25/0x30
> [    5.171535]  [<ffffffff8154f005>] ? nmi_handle.clone.1+0x75/0xc0
> [    5.243202]  [<ffffffff8154efcf>] ? nmi_handle.clone.1+0x3f/0xc0
> [    5.314867]  [<ffffffff8154ef90>] ? __die+0xf0/0xf0
> [    5.373038]  [<ffffffff8154f15f>] do_nmi+0x10f/0x360
> [    5.432243]  [<ffffffff8154e5cd>] restart_nmi+0x1a/0x1e
> [    5.494565]  [<ffffffff8154e210>] ? general_protection+0x30/0x30
> [    5.566234]  [<ffffffff8154e210>] ? general_protection+0x30/0x30
> [    5.637898]  [<ffffffff8154e210>] ? general_protection+0x30/0x30
> [    5.709566]  <<EOE>>  [<ffffffff8126d814>] ? rb_insert_color+0xa4/0x150
> [    5.788526]  [<ffffffff8119d17b>] sysfs_link_sibling+0x8b/0x110
> [    5.859155]  [<ffffffff8119dff1>] __sysfs_add_one+0xc1/0x100
> [    5.926666]  [<ffffffff8119e056>] sysfs_add_one+0x26/0xd0
> [    5.991065]  [<ffffffff8119cdf4>] sysfs_add_file_mode+0xc4/0x100
> [    6.062731]  [<ffffffff8119fc41>] internal_create_group+0xc1/0x1a0
> [    6.136473]  [<ffffffff8119fd4e>] sysfs_create_group+0xe/0x10
> [    6.205026]  [<ffffffff81351c1a>] dpm_sysfs_add+0x2a/0xd0
> [    6.269425]  [<ffffffff81349bf5>] device_add+0x5e5/0x730
> [    6.332783]  [<ffffffff81349d59>] device_register+0x19/0x20
> [    6.399260]  [<ffffffff8135b6b8>] add_memory_section+0x158/0x1e0
> [    6.470927]  [<ffffffff81ca757e>] memory_dev_init+0x75/0x108
> [    6.538439]  [<ffffffff81ca73a9>] driver_init+0x31/0x33
> [    6.600762]  [<ffffffff81c72c68>] kernel_init+0xcc/0x169
> [    6.664121]  [<ffffffff81555e64>] kernel_thread_helper+0x4/0x10
> [    6.734749]  [<ffffffff81c72b9c>] ? start_kernel+0x3ab/0x3ab
> [    6.802261]  [<ffffffff81555e60>] ? gs_change+0x13/0x13
> [    6.864585] ---[ end trace a7919e7f17c0a725 ]---
> 
> These two patches tries to fix some of the problems by avoiding using the
> non-present pages.


Hell no, these are some of the ugliest patches I've seen in a while. Not
to mention that their changelogs are utter crap since they don't even
explain why they're doing what they're doing.

Re: [PATCH 0/2 x86] fix some page faults in nmi if kmemcheck is enabled

[Li Zhong]

On Mon, 2012-02-20 at 12:00 +0100, Peter Zijlstra wrote:
> On Mon, 2012-02-20 at 14:01 +0800, Li Zhong wrote:
> > If CONFIG_KMEMCHECK is enabled, there might be page faults in nmi if the
> > pages are marked as not present by kmemcheck, like following:
> > 
> > [    4.535803] WARNING: at arch/x86/mm/kmemcheck/kmemcheck.c:634 kmemcheck_fault+0xb9/0xd0()
> > [    4.633429] Hardware name: System x3650 M3 -[7945AC1]-
> > [    4.694710] Modules linked in:
> > [    4.731105] Pid: 1, comm: swapper/0 Not tainted 3.3.0-rc3 #15
> > [    4.799654] Call Trace:
> > [    4.828751]  <NMI>  [<ffffffff81042eca>] warn_slowpath_common+0x7a/0xb0
> > [    4.907713]  [<ffffffff81042f15>] warn_slowpath_null+0x15/0x20
> > [    4.977301]  [<ffffffff8103ce89>] kmemcheck_fault+0xb9/0xd0
> > [    5.043778]  [<ffffffff81551ba6>] do_page_fault+0x406/0x550
> > [    5.110252]  [<ffffffff8154e235>] page_fault+0x25/0x30
> > [    5.171535]  [<ffffffff8154f005>] ? nmi_handle.clone.1+0x75/0xc0
> > [    5.243202]  [<ffffffff8154efcf>] ? nmi_handle.clone.1+0x3f/0xc0
> > [    5.314867]  [<ffffffff8154ef90>] ? __die+0xf0/0xf0
> > [    5.373038]  [<ffffffff8154f15f>] do_nmi+0x10f/0x360
> > [    5.432243]  [<ffffffff8154e5cd>] restart_nmi+0x1a/0x1e
> > [    5.494565]  [<ffffffff8154e210>] ? general_protection+0x30/0x30
> > [    5.566234]  [<ffffffff8154e210>] ? general_protection+0x30/0x30
> > [    5.637898]  [<ffffffff8154e210>] ? general_protection+0x30/0x30
> > [    5.709566]  <<EOE>>  [<ffffffff8126d814>] ? rb_insert_color+0xa4/0x150
> > [    5.788526]  [<ffffffff8119d17b>] sysfs_link_sibling+0x8b/0x110
> > [    5.859155]  [<ffffffff8119dff1>] __sysfs_add_one+0xc1/0x100
> > [    5.926666]  [<ffffffff8119e056>] sysfs_add_one+0x26/0xd0
> > [    5.991065]  [<ffffffff8119cdf4>] sysfs_add_file_mode+0xc4/0x100
> > [    6.062731]  [<ffffffff8119fc41>] internal_create_group+0xc1/0x1a0
> > [    6.136473]  [<ffffffff8119fd4e>] sysfs_create_group+0xe/0x10
> > [    6.205026]  [<ffffffff81351c1a>] dpm_sysfs_add+0x2a/0xd0
> > [    6.269425]  [<ffffffff81349bf5>] device_add+0x5e5/0x730
> > [    6.332783]  [<ffffffff81349d59>] device_register+0x19/0x20
> > [    6.399260]  [<ffffffff8135b6b8>] add_memory_section+0x158/0x1e0
> > [    6.470927]  [<ffffffff81ca757e>] memory_dev_init+0x75/0x108
> > [    6.538439]  [<ffffffff81ca73a9>] driver_init+0x31/0x33
> > [    6.600762]  [<ffffffff81c72c68>] kernel_init+0xcc/0x169
> > [    6.664121]  [<ffffffff81555e64>] kernel_thread_helper+0x4/0x10
> > [    6.734749]  [<ffffffff81c72b9c>] ? start_kernel+0x3ab/0x3ab
> > [    6.802261]  [<ffffffff81555e60>] ? gs_change+0x13/0x13
> > [    6.864585] ---[ end trace a7919e7f17c0a725 ]---
> > 
> > These two patches tries to fix some of the problems by avoiding using the
> > non-present pages.
> 
> 
> Hell no, these are some of the ugliest patches I've seen in a while. Not
> to mention that their changelogs are utter crap since they don't even
> explain why they're doing what they're doing.
> 
Hi Peter, 

I agree that the fix is ugly. I'm willing to change if there are some
better ways. 

The problem here is: 
1. It seems x86 doesn't allow page faults in nmi, and there are checks
in the code, like WARN_ON_ONCE(in_nmi()).
 
2. If CONFIG_KMEMCHECK is enabled, the pages allocated through slab will
be marked as non-present, to capture uninitialized memory access. More
information in Documentation/kmemcheck.txt .

3. From the log, there are some memories accessed in nmi, which are in
pages marked as non-present by kmemcheck, as they are allocated by
something like kmalloc(). 

Thanks,
Zhong

Re: [PATCH 0/2 x86] fix some page faults in nmi if kmemcheck is enabled

[Peter Zijlstra]

On Tue, 2012-02-21 at 09:42 +0800, Li Zhong wrote:

> > Hell no, these are some of the ugliest patches I've seen in a while. Not
> > to mention that their changelogs are utter crap since they don't even
> > explain why they're doing what they're doing.
> > 
> Hi Peter, 
> 
> I agree that the fix is ugly. I'm willing to change if there are some
> better ways. 

There are always better ways..

> The problem here is: 
> 1. It seems x86 doesn't allow page faults in nmi, and there are checks
> in the code, like WARN_ON_ONCE(in_nmi()).

I bet that's not x86 only..

> 2. If CONFIG_KMEMCHECK is enabled, the pages allocated through slab will
> be marked as non-present, to capture uninitialized memory access. More
> information in Documentation/kmemcheck.txt .

So then kmemcheck is buggy, since the nmiaction structure is initialized
in register_nmi_handler(), so it should most definitely not be marked
non-present.

> 3. From the log, there are some memories accessed in nmi, which are in
> pages marked as non-present by kmemcheck, as they are allocated by
> something like kmalloc(). 

So figure out why and fix that instead of writing ugly ass patches that
seemingly work around the problem without actually thinking about it.

Re: [PATCH 0/2 x86] fix some page faults in nmi if kmemcheck is enabled

[Li Zhong]

On Tue, 2012-02-21 at 11:17 +0100, Peter Zijlstra wrote:
> On Tue, 2012-02-21 at 09:42 +0800, Li Zhong wrote:
> 
> > > Hell no, these are some of the ugliest patches I've seen in a while. Not
> > > to mention that their changelogs are utter crap since they don't even
> > > explain why they're doing what they're doing.
> > > 
> > Hi Peter, 
> > 
> > I agree that the fix is ugly. I'm willing to change if there are some
> > better ways. 
> 
> There are always better ways..

Hi Peter,

I will think further about it, and would appreciate it if you could give
some good ideas. 

> 
> > The problem here is: 
> > 1. It seems x86 doesn't allow page faults in nmi, and there are checks
> > in the code, like WARN_ON_ONCE(in_nmi()).
> 
> I bet that's not x86 only..

Maybe, I found this problem on x86, didn't check other archs. 

However, from Documentation/kmemcheck.txt, seems kmemcheck only supports
x86. 

> 
> > 2. If CONFIG_KMEMCHECK is enabled, the pages allocated through slab will
> > be marked as non-present, to capture uninitialized memory access. More
> > information in Documentation/kmemcheck.txt .
> 
> So then kmemcheck is buggy, since the nmiaction structure is initialized
> in register_nmi_handler(), so it should most definitely not be marked
> non-present.
> 

I'm not sure whether I understand it correctly. Do you mean that
nmiaction is initialized in register_nmi_handler(), which indicates it
will be used in nmi, so it shouldn't be marked non-present?

But for kmemcheck, why need it know the information that page fault is
not allowed in nmi? 

Or maybe I misunderstand your point here? 

> > 3. From the log, there are some memories accessed in nmi, which are in
> > pages marked as non-present by kmemcheck, as they are allocated by
> > something like kmalloc(). 
> 
> So figure out why and fix that instead of writing ugly ass patches that
> seemingly work around the problem without actually thinking about it.
> 

I think the reason is that kmalloc ( or kzalloc ... ) uses malloc_sizes
slab caches to allocate memory. The malloc_sizes slab caches is set up
without SLAB_NOTRACK flag, then kmemcheck marks the pages non-present to
do its check in page fault handling code. I think we shouldn't disable
kmemechek for the general malloc_sizes caches. 

Thanks,
Zhong

Re: [PATCH 0/2 x86] fix some page faults in nmi if kmemcheck is enabled

[Peter Zijlstra]

On Thu, 2012-02-23 at 17:53 +0800, Li Zhong wrote:

> I will think further about it, and would appreciate it if you could give
> some good ideas. 

*sigh*.. or you could do your own damn work..

> > > 2. If CONFIG_KMEMCHECK is enabled, the pages allocated through slab will
> > > be marked as non-present, to capture uninitialized memory access. More
> > > information in Documentation/kmemcheck.txt .
> > 
> > So then kmemcheck is buggy, since the nmiaction structure is initialized
> > in register_nmi_handler(), so it should most definitely not be marked
> > non-present.
> > 
> 
> I'm not sure whether I understand it correctly. Do you mean that
> nmiaction is initialized in register_nmi_handler(), which indicates it
> will be used in nmi, so it shouldn't be marked non-present?

No, you said that it marks memory non-present to detect uninitialized
stuff, but since it is initialized, it shouldn't then be non-present,
right?

> But for kmemcheck, why need it know the information that page fault is
> not allowed in nmi? 

Uh, what?

> > > 3. From the log, there are some memories accessed in nmi, which are in
> > > pages marked as non-present by kmemcheck, as they are allocated by
> > > something like kmalloc(). 
> > 
> > So figure out why and fix that instead of writing ugly ass patches that
> > seemingly work around the problem without actually thinking about it.
> > 
> 
> I think the reason is that kmalloc ( or kzalloc ... ) uses malloc_sizes
> slab caches to allocate memory. The malloc_sizes slab caches is set up
> without SLAB_NOTRACK flag, then kmemcheck marks the pages non-present to
> do its check in page fault handling code. I think we shouldn't disable
> kmemechek for the general malloc_sizes caches. 

Nobody said you should.. there's plenty of solutions that aren't ass
backward stupid nor as ugly.

First you need to figure out why the page is marked non-present since
the data structure is initialized (I've got a fair idea why), then look
if you can tell kmemcheck not to be silly like that.

Alternatively you can change the nmi stuff to use static storage like
other notifiers (see notifier_block).

What you don't ever do is write alternative code paths that are never
ever used except for debugging, that is just asking for problems.

Re: [PATCH 0/2 x86] fix some page faults in nmi if kmemcheck is enabled

[Li Zhong]

On Mon, 2012-02-27 at 11:58 +0100, Peter Zijlstra wrote:
> On Thu, 2012-02-23 at 17:53 +0800, Li Zhong wrote:
> 
> > I will think further about it, and would appreciate it if you could give
> > some good ideas. 
> 
> *sigh*.. or you could do your own damn work..

I'm still doing the work ...

> 
> > > > 2. If CONFIG_KMEMCHECK is enabled, the pages allocated through slab will
> > > > be marked as non-present, to capture uninitialized memory access. More
> > > > information in Documentation/kmemcheck.txt .
> > > 
> > > So then kmemcheck is buggy, since the nmiaction structure is initialized
> > > in register_nmi_handler(), so it should most definitely not be marked
> > > non-present.
> > > 
> > 
> > I'm not sure whether I understand it correctly. Do you mean that
> > nmiaction is initialized in register_nmi_handler(), which indicates it
> > will be used in nmi, so it shouldn't be marked non-present?
> 
> No, you said that it marks memory non-present to detect uninitialized
> stuff, but since it is initialized, it shouldn't then be non-present,
> right?

>From my understanding of kmemcheck, the checking is based on the
non-present page. So while handling page fault, if the memory hasn't
been written before read, kmemcheck knows that it is uninitialized. 

I think it is used to find code errors, so it need mark all non-present,
to check if there are any access to uninitialized memory. 

> 
> > But for kmemcheck, why need it know the information that page fault is
> > not allowed in nmi? 
> 
> Uh, what?

Please ignore it, as I misunderstood your point previously. 

> > > > 3. From the log, there are some memories accessed in nmi, which are in
> > > > pages marked as non-present by kmemcheck, as they are allocated by
> > > > something like kmalloc(). 
> > > 
> > > So figure out why and fix that instead of writing ugly ass patches that
> > > seemingly work around the problem without actually thinking about it.
> > > 
> > 
> > I think the reason is that kmalloc ( or kzalloc ... ) uses malloc_sizes
> > slab caches to allocate memory. The malloc_sizes slab caches is set up
> > without SLAB_NOTRACK flag, then kmemcheck marks the pages non-present to
> > do its check in page fault handling code. I think we shouldn't disable
> > kmemechek for the general malloc_sizes caches. 
> 
> Nobody said you should.. there's plenty of solutions that aren't ass
> backward stupid nor as ugly.
> 
> First you need to figure out why the page is marked non-present since
> the data structure is initialized (I've got a fair idea why), then look
> if you can tell kmemcheck not to be silly like that.
> 
> Alternatively you can change the nmi stuff to use static storage like
> other notifiers (see notifier_block).

OK, I will try to update the nmi one using this way.

But I think it couldn't be used to the perf stuff. 
For perf, maybe it's good for kmemcheck to have some flag like
__GFP_NO_PAGE_FAULT? Currently it seems only has flags like
__GFP_NOTRACK, which will still mark page non-present. 

> 
> What you don't ever do is write alternative code paths that are never
> ever used except for debugging, that is just asking for problems.
> 

Got it, thanks for the reminder! Previously, I thought the biggest
problem was wasting memory ...

Thanks,
Zhong

Re: [PATCH 0/2 x86] fix some page faults in nmi if kmemcheck is enabled

[Don Zickus]

On Tue, Feb 28, 2012 at 10:45:41AM +0800, Li Zhong wrote:
> > > I'm not sure whether I understand it correctly. Do you mean that
> > > nmiaction is initialized in register_nmi_handler(), which indicates it
> > > will be used in nmi, so it shouldn't be marked non-present?
> > 
> > No, you said that it marks memory non-present to detect uninitialized
> > stuff, but since it is initialized, it shouldn't then be non-present,
> > right?
> 
> From my understanding of kmemcheck, the checking is based on the
> non-present page. So while handling page fault, if the memory hasn't
> been written before read, kmemcheck knows that it is uninitialized. 
> 
> I think it is used to find code errors, so it need mark all non-present,
> to check if there are any access to uninitialized memory. 

I am not sure if this is what your tool is catching, but someone pointed
out to me privately that when panic'ing in an NMI, either the shutdown
path I modified or the kdump path will register an NMI handler in an NMI
context.  Thus they will try to allocate memory in the NMI context.

So I will have to fix that.  Wonder if that popular lockless memory
allocator can help me there... otherwise I have to go back to pass in
structs like the notifier blocks do.

Cheers,
Don

Re: [PATCH 0/2 x86] fix some page faults in nmi if kmemcheck is enabled

[Li Zhong]

On Fri, 2012-03-02 at 14:44 -0500, Don Zickus wrote:
> On Tue, Feb 28, 2012 at 10:45:41AM +0800, Li Zhong wrote:
> > > > I'm not sure whether I understand it correctly. Do you mean that
> > > > nmiaction is initialized in register_nmi_handler(), which indicates it
> > > > will be used in nmi, so it shouldn't be marked non-present?
> > > 
> > > No, you said that it marks memory non-present to detect uninitialized
> > > stuff, but since it is initialized, it shouldn't then be non-present,
> > > right?
> > 
> > From my understanding of kmemcheck, the checking is based on the
> > non-present page. So while handling page fault, if the memory hasn't
> > been written before read, kmemcheck knows that it is uninitialized. 
> > 
> > I think it is used to find code errors, so it need mark all non-present,
> > to check if there are any access to uninitialized memory. 
> 
> I am not sure if this is what your tool is catching, but someone pointed
> out to me privately that when panic'ing in an NMI, either the shutdown
> path I modified or the kdump path will register an NMI handler in an NMI
> context.  Thus they will try to allocate memory in the NMI context.
> 

Maybe I didn't describe it clearly. 
This is not what kmemcheck is catching, and normally I think it is fine
to allocate memory that would be accessed in nmi. 

What is not allowed is page fault ( maybe also other exceptions) in nmi.
And if CONFIG_KMEMCHECK is enabled, for example, if kmemcheck needs
check whether any fields in the allocated nmiaction is read before
written (reading uninitialized memory), it marks the pages the nmiaction
belongs to as non-present, so the check could be done in the page fault
handling. 
In this case, as all the fields in nmiaction are assigned values in
register_nmi_handler, so no errors would be reported by kmemcheck. The
problem is there would be page fault when the nmi code calling the nmi
handlers, as it accesses the memory of nmiaction. 

> So I will have to fix that.  Wonder if that popular lockless memory
> allocator can help me there... otherwise I have to go back to pass in
> structs like the notifier blocks do.

I need check what lockless memory allocator is, but guessing from its
name "lockless", seems it couldn't help this problem.

By the way, I have done a draft fix to make the nmiaction using static
storage, will send out for review soon. 

Thanks,
Zhong
 
> 
> Cheers,
> Don
> 

[PATCH v2 x86 1/2] fix page faults by nmiaction in nmi if kmemcheck is enabled

[Li Zhong]

This patch tries to fix the problem of page fault exception caused by
accessing nmiaction structure in nmi if kmemcheck is enabled. 

If kmemcheck is enabled, the memory allocated through slab are in pages
that are marked non-present, so that some checks could be done in the
page fault handling code ( e.g. whether the memory is read before
written to ). 
As nmiaction is allocated in this way, so it resides in a non-present
page. Then there is a page fault while the nmi code accessing the
nmiaction structure, which would then cause a warning by
WARN_ON_ONCE(in_nmi()) in kmemcheck_fault(), called by do_page_fault().

v2: as Peter suggested, changed the nmiaction to use static storage.

Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
 arch/x86/include/asm/nmi.h              |   10 +++++-
 arch/x86/kernel/apic/hw_nmi.c           |    8 ++++-
 arch/x86/kernel/apic/x2apic_uv_x.c      |    7 ++++-
 arch/x86/kernel/cpu/mcheck/mce-inject.c |    8 ++++-
 arch/x86/kernel/cpu/perf_event.c        |    7 ++++-
 arch/x86/kernel/kgdb.c                  |   11 ++++--
 arch/x86/kernel/nmi.c                   |   49 ++----------------------------
 arch/x86/kernel/nmi_selftest.c          |   16 ++++++++--
 arch/x86/kernel/reboot.c                |    9 ++++-
 arch/x86/kernel/smp.c                   |    9 ++++-
 arch/x86/oprofile/nmi_int.c             |    8 ++++-
 drivers/acpi/apei/ghes.c                |    8 ++++-
 drivers/char/ipmi/ipmi_watchdog.c       |   10 +++++-
 drivers/watchdog/hpwdt.c                |   21 +++++++++++--
 14 files changed, 108 insertions(+), 73 deletions(-)

diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
index fd3f9f1..08d464f 100644
--- a/arch/x86/include/asm/nmi.h
+++ b/arch/x86/include/asm/nmi.h
@@ -35,8 +35,14 @@ enum {
 
 typedef int (*nmi_handler_t)(unsigned int, struct pt_regs *);
 
-int register_nmi_handler(unsigned int, nmi_handler_t, unsigned long,
-			 const char *);
+struct nmiaction {
+	struct list_head list;
+	nmi_handler_t handler;
+	unsigned int flags;
+	const char *name;
+};
+
+int register_nmi_handler(unsigned int, struct nmiaction *);
 
 void unregister_nmi_handler(unsigned int, const char *);
 
diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
index 31cb9ae..9baea77 100644
--- a/arch/x86/kernel/apic/hw_nmi.c
+++ b/arch/x86/kernel/apic/hw_nmi.c
@@ -80,10 +80,14 @@ arch_trigger_all_cpu_backtrace_handler(unsigned int cmd, struct pt_regs *regs)
 	return NMI_DONE;
 }
 
+static struct nmiaction arch_trigger_all_cpu_bt_nmiaction = {
+	.handler	= arch_trigger_all_cpu_backtrace_handler,
+	.name		= "arch_bt",
+};
+
 static int __init register_trigger_all_cpu_backtrace(void)
 {
-	register_nmi_handler(NMI_LOCAL, arch_trigger_all_cpu_backtrace_handler,
-				0, "arch_bt");
+	register_nmi_handler(NMI_LOCAL, &arch_trigger_all_cpu_bt_nmiaction);
 	return 0;
 }
 early_initcall(register_trigger_all_cpu_backtrace);
diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index 79b05b8..5739042 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -715,9 +715,14 @@ int uv_handle_nmi(unsigned int reason, struct pt_regs *regs)
 	return NMI_HANDLED;
 }
 
+static struct nmiaction uv_nmiaction = {
+	.handler	= uv_handle_nmi,
+	.name		= "uv",
+};
+
 void uv_register_nmi_notifier(void)
 {
-	if (register_nmi_handler(NMI_UNKNOWN, uv_handle_nmi, 0, "uv"))
+	if (register_nmi_handler(NMI_UNKNOWN, &uv_nmiaction))
 		printk(KERN_WARNING "UV NMI handler failed to register\n");
 }
 
diff --git a/arch/x86/kernel/cpu/mcheck/mce-inject.c b/arch/x86/kernel/cpu/mcheck/mce-inject.c
index fc4beb3..f9ab41c 100644
--- a/arch/x86/kernel/cpu/mcheck/mce-inject.c
+++ b/arch/x86/kernel/cpu/mcheck/mce-inject.c
@@ -229,14 +229,18 @@ static ssize_t mce_write(struct file *filp, const char __user *ubuf,
 	return usize;
 }
 
+static struct nmiaction mce_nmiaction = {
+	.handler	= mce_raise_notify,
+	.name		= "mce_notify",
+};
+
 static int inject_init(void)
 {
 	if (!alloc_cpumask_var(&mce_inject_cpumask, GFP_KERNEL))
 		return -ENOMEM;
 	printk(KERN_INFO "Machine check injector initialized\n");
 	register_mce_write_callback(mce_write);
-	register_nmi_handler(NMI_LOCAL, mce_raise_notify, 0,
-				"mce_notify");
+	register_nmi_handler(NMI_LOCAL, &mce_nmiaction);
 	return 0;
 }
 
diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 5adce10..8bdff1b 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1246,6 +1246,11 @@ static void __init pmu_check_apic(void)
 	pr_info("no hardware sampling interrupt available.\n");
 }
 
+static struct nmiaction perf_event_nmiaction = {
+	.handler	= perf_event_nmi_handler,
+	.name		= "PMI",
+};
+
 static int __init init_hw_perf_events(void)
 {
 	struct x86_pmu_quirk *quirk;
@@ -1297,7 +1302,7 @@ static int __init init_hw_perf_events(void)
 		((1LL << x86_pmu.num_counters_fixed)-1) << X86_PMC_IDX_FIXED;
 
 	perf_events_lapic_init();
-	register_nmi_handler(NMI_LOCAL, perf_event_nmi_handler, 0, "PMI");
+	register_nmi_handler(NMI_LOCAL, &perf_event_nmiaction);
 
 	unconstrained = (struct event_constraint)
 		__EVENT_CONSTRAINT(0, (1ULL << x86_pmu.num_counters) - 1,
diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index faba577..d259d2a 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -601,6 +601,11 @@ static struct notifier_block kgdb_notifier = {
 	.notifier_call	= kgdb_notify,
 };
 
+static struct nmiaction kgdb_nmiaction = {
+	.handler	= kgdb_nmi_handler,
+	.name		= "kgdb",
+};
+
 /**
  *	kgdb_arch_init - Perform any architecture specific initalization.
  *
@@ -615,13 +620,11 @@ int kgdb_arch_init(void)
 	if (retval)
 		goto out;
 
-	retval = register_nmi_handler(NMI_LOCAL, kgdb_nmi_handler,
-					0, "kgdb");
+	retval = register_nmi_handler(NMI_LOCAL, &kgdb_nmiaction);
 	if (retval)
 		goto out1;
 
-	retval = register_nmi_handler(NMI_UNKNOWN, kgdb_nmi_handler,
-					0, "kgdb");
+	retval = register_nmi_handler(NMI_UNKNOWN, &kgdb_nmiaction);
 
 	if (retval)
 		goto out2;
diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 47acaf3..d844acc 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -31,14 +31,6 @@
 #include <asm/nmi.h>
 #include <asm/x86_init.h>
 
-#define NMI_MAX_NAMELEN	16
-struct nmiaction {
-	struct list_head list;
-	nmi_handler_t handler;
-	unsigned int flags;
-	char *name;
-};
-
 struct nmi_desc {
 	spinlock_t lock;
 	struct list_head head;
@@ -160,51 +152,18 @@ static struct nmiaction *__free_nmi(unsigned int type, const char *name)
 	return (n);
 }
 
-int register_nmi_handler(unsigned int type, nmi_handler_t handler,
-			unsigned long nmiflags, const char *devname)
+int register_nmi_handler(unsigned int type, struct nmiaction *na)
 {
-	struct nmiaction *action;
-	int retval = -ENOMEM;
-
-	if (!handler)
+	if (!na->handler)
 		return -EINVAL;
 
-	action = kzalloc(sizeof(struct nmiaction), GFP_KERNEL);
-	if (!action)
-		goto fail_action;
-
-	action->handler = handler;
-	action->flags = nmiflags;
-	action->name = kstrndup(devname, NMI_MAX_NAMELEN, GFP_KERNEL);
-	if (!action->name)
-		goto fail_action_name;
-
-	retval = __setup_nmi(type, action);
-
-	if (retval)
-		goto fail_setup_nmi;
-
-	return retval;
-
-fail_setup_nmi:
-	kfree(action->name);
-fail_action_name:
-	kfree(action);
-fail_action:	
-
-	return retval;
+	return __setup_nmi(type, na);
 }
 EXPORT_SYMBOL_GPL(register_nmi_handler);
 
 void unregister_nmi_handler(unsigned int type, const char *name)
 {
-	struct nmiaction *a;
-
-	a = __free_nmi(type, name);
-	if (a) {
-		kfree(a->name);
-		kfree(a);
-	}
+	__free_nmi(type, name);
 }
 
 EXPORT_SYMBOL_GPL(unregister_nmi_handler);
diff --git a/arch/x86/kernel/nmi_selftest.c b/arch/x86/kernel/nmi_selftest.c
index 0d01a8e..40fd637 100644
--- a/arch/x86/kernel/nmi_selftest.c
+++ b/arch/x86/kernel/nmi_selftest.c
@@ -37,10 +37,15 @@ static int nmi_unk_cb(unsigned int val, struct pt_regs *regs)
 	return NMI_HANDLED;
 }
 
+static struct nmiaction selftest_unk_nmiaction = {
+	.handler	= nmi_unk_cb,
+	.name		= "nmi_selftest_unk",
+};
+
 static void init_nmi_testsuite(void)
 {
 	/* trap all the unknown NMIs we may generate */
-	register_nmi_handler(NMI_UNKNOWN, nmi_unk_cb, 0, "nmi_selftest_unk");
+	register_nmi_handler(NMI_UNKNOWN, &selftest_unk_nmiaction);
 }
 
 static void cleanup_nmi_testsuite(void)
@@ -58,12 +63,17 @@ static int test_nmi_ipi_callback(unsigned int val, struct pt_regs *regs)
         return NMI_DONE;
 }
 
+static struct nmiaction selftest_nmiaction = {
+	.handler	= test_nmi_ipi_callback,
+	.flags		= NMI_FLAG_FIRST,
+	.name		= "nmi_selftest",
+};
+
 static void test_nmi_ipi(struct cpumask *mask)
 {
 	unsigned long timeout;
 
-	if (register_nmi_handler(NMI_LOCAL, test_nmi_ipi_callback,
-				 NMI_FLAG_FIRST, "nmi_selftest")) {
+	if (register_nmi_handler(NMI_LOCAL, &selftest_nmiaction)) {
 		nmi_fail = FAILURE;
 		return;
 	}
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index d840e69..a0f8c15 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -799,6 +799,12 @@ static void smp_send_nmi_allbutself(void)
 	apic->send_IPI_allbutself(NMI_VECTOR);
 }
 
+static struct nmiaction crash_nmiaction = {
+	.handler	= crash_nmi_callback,
+	.flags		= NMI_FLAG_FIRST,
+	.name		= "crash",
+};
+
 /* Halt all other CPUs, calling the specified function on each of them
  *
  * This function can be used to halt all other CPUs on crash
@@ -817,8 +823,7 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback)
 
 	atomic_set(&waiting_for_crash_ipi, num_online_cpus() - 1);
 	/* Would it be better to replace the trap vector here? */
-	if (register_nmi_handler(NMI_LOCAL, crash_nmi_callback,
-				 NMI_FLAG_FIRST, "crash"))
+	if (register_nmi_handler(NMI_LOCAL, &crash_nmiaction))
 		return;		/* return what? */
 	/* Ensure the new callback function is set before sending
 	 * out the NMI
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index 66c74f4..135d0b2 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -162,6 +162,12 @@ static int smp_stop_nmi_callback(unsigned int val, struct pt_regs *regs)
 	return NMI_HANDLED;
 }
 
+static struct nmiaction smp_stop_nmiaction = {
+	.handler	= smp_stop_nmi_callback,
+	.flags		= NMI_FLAG_FIRST,
+	.name		= "smp_stop",
+};
+
 static void native_nmi_stop_other_cpus(int wait)
 {
 	unsigned long flags;
@@ -179,8 +185,7 @@ static void native_nmi_stop_other_cpus(int wait)
 		if (atomic_cmpxchg(&stopping_cpu, -1, safe_smp_processor_id()) != -1)
 			return;
 
-		if (register_nmi_handler(NMI_LOCAL, smp_stop_nmi_callback,
-					 NMI_FLAG_FIRST, "smp_stop"))
+		if (register_nmi_handler(NMI_LOCAL, &smp_stop_nmiaction))
 			/* Note: we ignore failures here */
 			return;
 
diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c
index 26b8a85..c3408f6 100644
--- a/arch/x86/oprofile/nmi_int.c
+++ b/arch/x86/oprofile/nmi_int.c
@@ -453,6 +453,11 @@ static struct notifier_block oprofile_cpu_nb = {
 	.notifier_call = oprofile_cpu_notifier
 };
 
+static struct nmiaction oprofile_nmiaction = {
+	.handler	= profile_exceptions_notify,
+	.name		= "oprofile",
+};
+
 static int nmi_setup(void)
 {
 	int err = 0;
@@ -489,8 +494,7 @@ static int nmi_setup(void)
 	ctr_running = 0;
 	/* make variables visible to the nmi handler: */
 	smp_mb();
-	err = register_nmi_handler(NMI_LOCAL, profile_exceptions_notify,
-					0, "oprofile");
+	err = register_nmi_handler(NMI_LOCAL, &oprofile_nmiaction);
 	if (err)
 		goto fail;
 
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 9b3cac0..1d38f92 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -901,6 +901,11 @@ static unsigned long ghes_esource_prealloc_size(
 	return prealloc_size;
 }
 
+static struct nmiaction ghes_nmiaction = {
+	.handler	= ghes_notify_nmi,
+	.name		= "ghes",
+};
+
 static int __devinit ghes_probe(struct platform_device *ghes_dev)
 {
 	struct acpi_hest_generic *generic;
@@ -975,8 +980,7 @@ static int __devinit ghes_probe(struct platform_device *ghes_dev)
 		ghes_estatus_pool_expand(len);
 		mutex_lock(&ghes_list_mutex);
 		if (list_empty(&ghes_nmi))
-			register_nmi_handler(NMI_LOCAL, ghes_notify_nmi, 0,
-						"ghes");
+			register_nmi_handler(NMI_LOCAL, &ghes_nmiaction);
 		list_add_rcu(&ghes->list, &ghes_nmi);
 		mutex_unlock(&ghes_list_mutex);
 		break;
diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/char/ipmi/ipmi_watchdog.c
index 34767a6..29a6e0a 100644
--- a/drivers/char/ipmi/ipmi_watchdog.c
+++ b/drivers/char/ipmi/ipmi_watchdog.c
@@ -1296,6 +1296,13 @@ static int preop_op(const char *inval, char *outval)
 	return 0;
 }
 
+#ifdef HAVE_DIE_NMI
+static struct nmiaction ipmi_nmiaction = {
+	.handler	= ipmi_nmi,
+	.name		= "ipmi",
+};
+#endif
+
 static void check_parms(void)
 {
 #ifdef HAVE_DIE_NMI
@@ -1313,8 +1320,7 @@ static void check_parms(void)
 		}
 	}
 	if (do_nmi && !nmi_handler_registered) {
-		rv = register_nmi_handler(NMI_UNKNOWN, ipmi_nmi, 0,
-						"ipmi");
+		rv = register_nmi_handler(NMI_UNKNOWN, &ipmi_nmiaction);
 		if (rv) {
 			printk(KERN_WARNING PFX
 			       "Can't register nmi handler\n");
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 8464ea1..e575e63 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -689,9 +689,22 @@ static void __devinit dmi_find_icru(const struct dmi_header *dm, void *dummy)
 	}
 }
 
+static struct nmiaction hpwdt_nmiaction[] = {
+	{
+		.handler	= hpwdt_pretimeout,
+		.name		= "hpwdt",
+	},
+	{
+		.handler	= hpwdt_pretimeout,
+		.flags		= NMI_FLAG_FIRST,
+		.name		= "hpwdt",
+	},
+};
+
 static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev)
 {
 	int retval;
+	struct nmiaction *na = hpwdt_nmiaction;
 
 	/*
 	 * On typical CRU-based systems we need to map that service in
@@ -733,9 +746,11 @@ static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev)
 	 * die notify list to handle a critical NMI. The default is to
 	 * be last so other users of the NMI signal can function.
 	 */
-	retval = register_nmi_handler(NMI_UNKNOWN, hpwdt_pretimeout,
-					(priority) ? NMI_FLAG_FIRST : 0,
-					"hpwdt");
+
+	if (priority)
+		na = &hpwdt_nmiaction[1];
+
+	retval = register_nmi_handler(NMI_UNKNOWN, na);
 	if (retval != 0) {
 		dev_warn(&dev->dev,
 			"Unable to register a die notifier (err=%d).\n",
-- 
1.7.1

Re: [PATCH v2 x86 1/2] fix page faults by nmiaction in nmi if kmemcheck is enabled

[Wim Van Sebroeck]

Hi Li,

> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index 8464ea1..e575e63 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -689,9 +689,22 @@ static void __devinit dmi_find_icru(const struct dmi_header *dm, void *dummy)
>  	}
>  }
>  
> +static struct nmiaction hpwdt_nmiaction[] = {
> +	{
> +		.handler	= hpwdt_pretimeout,
> +		.name		= "hpwdt",
> +	},
> +	{
> +		.handler	= hpwdt_pretimeout,
> +		.flags		= NMI_FLAG_FIRST,
> +		.name		= "hpwdt",
> +	},
> +};
> +
>  static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev)
>  {
>  	int retval;
> +	struct nmiaction *na = hpwdt_nmiaction;
>  
>  	/*
>  	 * On typical CRU-based systems we need to map that service in
> @@ -733,9 +746,11 @@ static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev)
>  	 * die notify list to handle a critical NMI. The default is to
>  	 * be last so other users of the NMI signal can function.
>  	 */
> -	retval = register_nmi_handler(NMI_UNKNOWN, hpwdt_pretimeout,
> -					(priority) ? NMI_FLAG_FIRST : 0,
> -					"hpwdt");
> +
> +	if (priority)
> +		na = &hpwdt_nmiaction[1];
> +
> +	retval = register_nmi_handler(NMI_UNKNOWN, na);
>  	if (retval != 0) {
>  		dev_warn(&dev->dev,
>  			"Unable to register a die notifier (err=%d).\n",

Why not do something like;

static struct nmiaction hpwdt_nmiaction = {
	.handler	= hpwdt_pretimeout,
	.name		= "hpwdt",
};

...
	if (priority)
		hpwdt_nmiaction.flags = NMI_FLAG_FIRST;
...

Kind regards,
Wim.

Re: [PATCH v2 x86 1/2] fix page faults by nmiaction in nmi if kmemcheck is enabled

[Don Zickus]

On Mon, Mar 05, 2012 at 06:05:17PM +0800, Li Zhong wrote:
> This patch tries to fix the problem of page fault exception caused by
> accessing nmiaction structure in nmi if kmemcheck is enabled. 
> 
> If kmemcheck is enabled, the memory allocated through slab are in pages
> that are marked non-present, so that some checks could be done in the
> page fault handling code ( e.g. whether the memory is read before
> written to ). 
> As nmiaction is allocated in this way, so it resides in a non-present
> page. Then there is a page fault while the nmi code accessing the
> nmiaction structure, which would then cause a warning by
> WARN_ON_ONCE(in_nmi()) in kmemcheck_fault(), called by do_page_fault().

This is one way of doing this.  I was trying to avoid this when I rewrote the
nmi handlers, because everyone kept screwing up the structs.  I thought it
would be safer to have callers pass in data based on an api instead.

So I am not necessarily a big fan of this patch (nor is it entirely
correct because you throwaway all the checks in register_nmi_handler()).

Then again I am not a memory expert and may be misunderstanding something
simple why it is safer to do static storage.

Cheers,
Don

> 
> v2: as Peter suggested, changed the nmiaction to use static storage.
> 
> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> ---
>  arch/x86/include/asm/nmi.h              |   10 +++++-
>  arch/x86/kernel/apic/hw_nmi.c           |    8 ++++-
>  arch/x86/kernel/apic/x2apic_uv_x.c      |    7 ++++-
>  arch/x86/kernel/cpu/mcheck/mce-inject.c |    8 ++++-
>  arch/x86/kernel/cpu/perf_event.c        |    7 ++++-
>  arch/x86/kernel/kgdb.c                  |   11 ++++--
>  arch/x86/kernel/nmi.c                   |   49 ++----------------------------
>  arch/x86/kernel/nmi_selftest.c          |   16 ++++++++--
>  arch/x86/kernel/reboot.c                |    9 ++++-
>  arch/x86/kernel/smp.c                   |    9 ++++-
>  arch/x86/oprofile/nmi_int.c             |    8 ++++-
>  drivers/acpi/apei/ghes.c                |    8 ++++-
>  drivers/char/ipmi/ipmi_watchdog.c       |   10 +++++-
>  drivers/watchdog/hpwdt.c                |   21 +++++++++++--
>  14 files changed, 108 insertions(+), 73 deletions(-)
> 
> diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
> index fd3f9f1..08d464f 100644
> --- a/arch/x86/include/asm/nmi.h
> +++ b/arch/x86/include/asm/nmi.h
> @@ -35,8 +35,14 @@ enum {
>  
>  typedef int (*nmi_handler_t)(unsigned int, struct pt_regs *);
>  
> -int register_nmi_handler(unsigned int, nmi_handler_t, unsigned long,
> -			 const char *);
> +struct nmiaction {
> +	struct list_head list;
> +	nmi_handler_t handler;
> +	unsigned int flags;
> +	const char *name;
> +};
> +
> +int register_nmi_handler(unsigned int, struct nmiaction *);
>  
>  void unregister_nmi_handler(unsigned int, const char *);
>  
> diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
> index 31cb9ae..9baea77 100644
> --- a/arch/x86/kernel/apic/hw_nmi.c
> +++ b/arch/x86/kernel/apic/hw_nmi.c
> @@ -80,10 +80,14 @@ arch_trigger_all_cpu_backtrace_handler(unsigned int cmd, struct pt_regs *regs)
>  	return NMI_DONE;
>  }
>  
> +static struct nmiaction arch_trigger_all_cpu_bt_nmiaction = {
> +	.handler	= arch_trigger_all_cpu_backtrace_handler,
> +	.name		= "arch_bt",
> +};
> +
>  static int __init register_trigger_all_cpu_backtrace(void)
>  {
> -	register_nmi_handler(NMI_LOCAL, arch_trigger_all_cpu_backtrace_handler,
> -				0, "arch_bt");
> +	register_nmi_handler(NMI_LOCAL, &arch_trigger_all_cpu_bt_nmiaction);
>  	return 0;
>  }
>  early_initcall(register_trigger_all_cpu_backtrace);
> diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
> index 79b05b8..5739042 100644
> --- a/arch/x86/kernel/apic/x2apic_uv_x.c
> +++ b/arch/x86/kernel/apic/x2apic_uv_x.c
> @@ -715,9 +715,14 @@ int uv_handle_nmi(unsigned int reason, struct pt_regs *regs)
>  	return NMI_HANDLED;
>  }
>  
> +static struct nmiaction uv_nmiaction = {
> +	.handler	= uv_handle_nmi,
> +	.name		= "uv",
> +};
> +
>  void uv_register_nmi_notifier(void)
>  {
> -	if (register_nmi_handler(NMI_UNKNOWN, uv_handle_nmi, 0, "uv"))
> +	if (register_nmi_handler(NMI_UNKNOWN, &uv_nmiaction))
>  		printk(KERN_WARNING "UV NMI handler failed to register\n");
>  }
>  
> diff --git a/arch/x86/kernel/cpu/mcheck/mce-inject.c b/arch/x86/kernel/cpu/mcheck/mce-inject.c
> index fc4beb3..f9ab41c 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce-inject.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce-inject.c
> @@ -229,14 +229,18 @@ static ssize_t mce_write(struct file *filp, const char __user *ubuf,
>  	return usize;
>  }
>  
> +static struct nmiaction mce_nmiaction = {
> +	.handler	= mce_raise_notify,
> +	.name		= "mce_notify",
> +};
> +
>  static int inject_init(void)
>  {
>  	if (!alloc_cpumask_var(&mce_inject_cpumask, GFP_KERNEL))
>  		return -ENOMEM;
>  	printk(KERN_INFO "Machine check injector initialized\n");
>  	register_mce_write_callback(mce_write);
> -	register_nmi_handler(NMI_LOCAL, mce_raise_notify, 0,
> -				"mce_notify");
> +	register_nmi_handler(NMI_LOCAL, &mce_nmiaction);
>  	return 0;
>  }
>  
> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index 5adce10..8bdff1b 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -1246,6 +1246,11 @@ static void __init pmu_check_apic(void)
>  	pr_info("no hardware sampling interrupt available.\n");
>  }
>  
> +static struct nmiaction perf_event_nmiaction = {
> +	.handler	= perf_event_nmi_handler,
> +	.name		= "PMI",
> +};
> +
>  static int __init init_hw_perf_events(void)
>  {
>  	struct x86_pmu_quirk *quirk;
> @@ -1297,7 +1302,7 @@ static int __init init_hw_perf_events(void)
>  		((1LL << x86_pmu.num_counters_fixed)-1) << X86_PMC_IDX_FIXED;
>  
>  	perf_events_lapic_init();
> -	register_nmi_handler(NMI_LOCAL, perf_event_nmi_handler, 0, "PMI");
> +	register_nmi_handler(NMI_LOCAL, &perf_event_nmiaction);
>  
>  	unconstrained = (struct event_constraint)
>  		__EVENT_CONSTRAINT(0, (1ULL << x86_pmu.num_counters) - 1,
> diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
> index faba577..d259d2a 100644
> --- a/arch/x86/kernel/kgdb.c
> +++ b/arch/x86/kernel/kgdb.c
> @@ -601,6 +601,11 @@ static struct notifier_block kgdb_notifier = {
>  	.notifier_call	= kgdb_notify,
>  };
>  
> +static struct nmiaction kgdb_nmiaction = {
> +	.handler	= kgdb_nmi_handler,
> +	.name		= "kgdb",
> +};
> +
>  /**
>   *	kgdb_arch_init - Perform any architecture specific initalization.
>   *
> @@ -615,13 +620,11 @@ int kgdb_arch_init(void)
>  	if (retval)
>  		goto out;
>  
> -	retval = register_nmi_handler(NMI_LOCAL, kgdb_nmi_handler,
> -					0, "kgdb");
> +	retval = register_nmi_handler(NMI_LOCAL, &kgdb_nmiaction);
>  	if (retval)
>  		goto out1;
>  
> -	retval = register_nmi_handler(NMI_UNKNOWN, kgdb_nmi_handler,
> -					0, "kgdb");
> +	retval = register_nmi_handler(NMI_UNKNOWN, &kgdb_nmiaction);
>  
>  	if (retval)
>  		goto out2;
> diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
> index 47acaf3..d844acc 100644
> --- a/arch/x86/kernel/nmi.c
> +++ b/arch/x86/kernel/nmi.c
> @@ -31,14 +31,6 @@
>  #include <asm/nmi.h>
>  #include <asm/x86_init.h>
>  
> -#define NMI_MAX_NAMELEN	16
> -struct nmiaction {
> -	struct list_head list;
> -	nmi_handler_t handler;
> -	unsigned int flags;
> -	char *name;
> -};
> -
>  struct nmi_desc {
>  	spinlock_t lock;
>  	struct list_head head;
> @@ -160,51 +152,18 @@ static struct nmiaction *__free_nmi(unsigned int type, const char *name)
>  	return (n);
>  }
>  
> -int register_nmi_handler(unsigned int type, nmi_handler_t handler,
> -			unsigned long nmiflags, const char *devname)
> +int register_nmi_handler(unsigned int type, struct nmiaction *na)
>  {
> -	struct nmiaction *action;
> -	int retval = -ENOMEM;
> -
> -	if (!handler)
> +	if (!na->handler)
>  		return -EINVAL;
>  
> -	action = kzalloc(sizeof(struct nmiaction), GFP_KERNEL);
> -	if (!action)
> -		goto fail_action;
> -
> -	action->handler = handler;
> -	action->flags = nmiflags;
> -	action->name = kstrndup(devname, NMI_MAX_NAMELEN, GFP_KERNEL);
> -	if (!action->name)
> -		goto fail_action_name;
> -
> -	retval = __setup_nmi(type, action);
> -
> -	if (retval)
> -		goto fail_setup_nmi;
> -
> -	return retval;
> -
> -fail_setup_nmi:
> -	kfree(action->name);
> -fail_action_name:
> -	kfree(action);
> -fail_action:	
> -
> -	return retval;
> +	return __setup_nmi(type, na);
>  }
>  EXPORT_SYMBOL_GPL(register_nmi_handler);
>  
>  void unregister_nmi_handler(unsigned int type, const char *name)
>  {
> -	struct nmiaction *a;
> -
> -	a = __free_nmi(type, name);
> -	if (a) {
> -		kfree(a->name);
> -		kfree(a);
> -	}
> +	__free_nmi(type, name);
>  }
>  
>  EXPORT_SYMBOL_GPL(unregister_nmi_handler);
> diff --git a/arch/x86/kernel/nmi_selftest.c b/arch/x86/kernel/nmi_selftest.c
> index 0d01a8e..40fd637 100644
> --- a/arch/x86/kernel/nmi_selftest.c
> +++ b/arch/x86/kernel/nmi_selftest.c
> @@ -37,10 +37,15 @@ static int nmi_unk_cb(unsigned int val, struct pt_regs *regs)
>  	return NMI_HANDLED;
>  }
>  
> +static struct nmiaction selftest_unk_nmiaction = {
> +	.handler	= nmi_unk_cb,
> +	.name		= "nmi_selftest_unk",
> +};
> +
>  static void init_nmi_testsuite(void)
>  {
>  	/* trap all the unknown NMIs we may generate */
> -	register_nmi_handler(NMI_UNKNOWN, nmi_unk_cb, 0, "nmi_selftest_unk");
> +	register_nmi_handler(NMI_UNKNOWN, &selftest_unk_nmiaction);
>  }
>  
>  static void cleanup_nmi_testsuite(void)
> @@ -58,12 +63,17 @@ static int test_nmi_ipi_callback(unsigned int val, struct pt_regs *regs)
>          return NMI_DONE;
>  }
>  
> +static struct nmiaction selftest_nmiaction = {
> +	.handler	= test_nmi_ipi_callback,
> +	.flags		= NMI_FLAG_FIRST,
> +	.name		= "nmi_selftest",
> +};
> +
>  static void test_nmi_ipi(struct cpumask *mask)
>  {
>  	unsigned long timeout;
>  
> -	if (register_nmi_handler(NMI_LOCAL, test_nmi_ipi_callback,
> -				 NMI_FLAG_FIRST, "nmi_selftest")) {
> +	if (register_nmi_handler(NMI_LOCAL, &selftest_nmiaction)) {
>  		nmi_fail = FAILURE;
>  		return;
>  	}
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index d840e69..a0f8c15 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -799,6 +799,12 @@ static void smp_send_nmi_allbutself(void)
>  	apic->send_IPI_allbutself(NMI_VECTOR);
>  }
>  
> +static struct nmiaction crash_nmiaction = {
> +	.handler	= crash_nmi_callback,
> +	.flags		= NMI_FLAG_FIRST,
> +	.name		= "crash",
> +};
> +
>  /* Halt all other CPUs, calling the specified function on each of them
>   *
>   * This function can be used to halt all other CPUs on crash
> @@ -817,8 +823,7 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback)
>  
>  	atomic_set(&waiting_for_crash_ipi, num_online_cpus() - 1);
>  	/* Would it be better to replace the trap vector here? */
> -	if (register_nmi_handler(NMI_LOCAL, crash_nmi_callback,
> -				 NMI_FLAG_FIRST, "crash"))
> +	if (register_nmi_handler(NMI_LOCAL, &crash_nmiaction))
>  		return;		/* return what? */
>  	/* Ensure the new callback function is set before sending
>  	 * out the NMI
> diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
> index 66c74f4..135d0b2 100644
> --- a/arch/x86/kernel/smp.c
> +++ b/arch/x86/kernel/smp.c
> @@ -162,6 +162,12 @@ static int smp_stop_nmi_callback(unsigned int val, struct pt_regs *regs)
>  	return NMI_HANDLED;
>  }
>  
> +static struct nmiaction smp_stop_nmiaction = {
> +	.handler	= smp_stop_nmi_callback,
> +	.flags		= NMI_FLAG_FIRST,
> +	.name		= "smp_stop",
> +};
> +
>  static void native_nmi_stop_other_cpus(int wait)
>  {
>  	unsigned long flags;
> @@ -179,8 +185,7 @@ static void native_nmi_stop_other_cpus(int wait)
>  		if (atomic_cmpxchg(&stopping_cpu, -1, safe_smp_processor_id()) != -1)
>  			return;
>  
> -		if (register_nmi_handler(NMI_LOCAL, smp_stop_nmi_callback,
> -					 NMI_FLAG_FIRST, "smp_stop"))
> +		if (register_nmi_handler(NMI_LOCAL, &smp_stop_nmiaction))
>  			/* Note: we ignore failures here */
>  			return;
>  
> diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c
> index 26b8a85..c3408f6 100644
> --- a/arch/x86/oprofile/nmi_int.c
> +++ b/arch/x86/oprofile/nmi_int.c
> @@ -453,6 +453,11 @@ static struct notifier_block oprofile_cpu_nb = {
>  	.notifier_call = oprofile_cpu_notifier
>  };
>  
> +static struct nmiaction oprofile_nmiaction = {
> +	.handler	= profile_exceptions_notify,
> +	.name		= "oprofile",
> +};
> +
>  static int nmi_setup(void)
>  {
>  	int err = 0;
> @@ -489,8 +494,7 @@ static int nmi_setup(void)
>  	ctr_running = 0;
>  	/* make variables visible to the nmi handler: */
>  	smp_mb();
> -	err = register_nmi_handler(NMI_LOCAL, profile_exceptions_notify,
> -					0, "oprofile");
> +	err = register_nmi_handler(NMI_LOCAL, &oprofile_nmiaction);
>  	if (err)
>  		goto fail;
>  
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 9b3cac0..1d38f92 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -901,6 +901,11 @@ static unsigned long ghes_esource_prealloc_size(
>  	return prealloc_size;
>  }
>  
> +static struct nmiaction ghes_nmiaction = {
> +	.handler	= ghes_notify_nmi,
> +	.name		= "ghes",
> +};
> +
>  static int __devinit ghes_probe(struct platform_device *ghes_dev)
>  {
>  	struct acpi_hest_generic *generic;
> @@ -975,8 +980,7 @@ static int __devinit ghes_probe(struct platform_device *ghes_dev)
>  		ghes_estatus_pool_expand(len);
>  		mutex_lock(&ghes_list_mutex);
>  		if (list_empty(&ghes_nmi))
> -			register_nmi_handler(NMI_LOCAL, ghes_notify_nmi, 0,
> -						"ghes");
> +			register_nmi_handler(NMI_LOCAL, &ghes_nmiaction);
>  		list_add_rcu(&ghes->list, &ghes_nmi);
>  		mutex_unlock(&ghes_list_mutex);
>  		break;
> diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/char/ipmi/ipmi_watchdog.c
> index 34767a6..29a6e0a 100644
> --- a/drivers/char/ipmi/ipmi_watchdog.c
> +++ b/drivers/char/ipmi/ipmi_watchdog.c
> @@ -1296,6 +1296,13 @@ static int preop_op(const char *inval, char *outval)
>  	return 0;
>  }
>  
> +#ifdef HAVE_DIE_NMI
> +static struct nmiaction ipmi_nmiaction = {
> +	.handler	= ipmi_nmi,
> +	.name		= "ipmi",
> +};
> +#endif
> +
>  static void check_parms(void)
>  {
>  #ifdef HAVE_DIE_NMI
> @@ -1313,8 +1320,7 @@ static void check_parms(void)
>  		}
>  	}
>  	if (do_nmi && !nmi_handler_registered) {
> -		rv = register_nmi_handler(NMI_UNKNOWN, ipmi_nmi, 0,
> -						"ipmi");
> +		rv = register_nmi_handler(NMI_UNKNOWN, &ipmi_nmiaction);
>  		if (rv) {
>  			printk(KERN_WARNING PFX
>  			       "Can't register nmi handler\n");
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index 8464ea1..e575e63 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -689,9 +689,22 @@ static void __devinit dmi_find_icru(const struct dmi_header *dm, void *dummy)
>  	}
>  }
>  
> +static struct nmiaction hpwdt_nmiaction[] = {
> +	{
> +		.handler	= hpwdt_pretimeout,
> +		.name		= "hpwdt",
> +	},
> +	{
> +		.handler	= hpwdt_pretimeout,
> +		.flags		= NMI_FLAG_FIRST,
> +		.name		= "hpwdt",
> +	},
> +};
> +
>  static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev)
>  {
>  	int retval;
> +	struct nmiaction *na = hpwdt_nmiaction;
>  
>  	/*
>  	 * On typical CRU-based systems we need to map that service in
> @@ -733,9 +746,11 @@ static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev)
>  	 * die notify list to handle a critical NMI. The default is to
>  	 * be last so other users of the NMI signal can function.
>  	 */
> -	retval = register_nmi_handler(NMI_UNKNOWN, hpwdt_pretimeout,
> -					(priority) ? NMI_FLAG_FIRST : 0,
> -					"hpwdt");
> +
> +	if (priority)
> +		na = &hpwdt_nmiaction[1];
> +
> +	retval = register_nmi_handler(NMI_UNKNOWN, na);
>  	if (retval != 0) {
>  		dev_warn(&dev->dev,
>  			"Unable to register a die notifier (err=%d).\n",
> -- 
> 1.7.1
> 
> 
> 
> 

Re: [PATCH v2 x86 1/2] fix page faults by nmiaction in nmi if kmemcheck is enabled

[Peter Zijlstra]

On Mon, 2012-03-05 at 18:05 +0800, Li Zhong wrote:
> +static struct nmiaction arch_trigger_all_cpu_bt_nmiaction = {
> +       .handler        = arch_trigger_all_cpu_backtrace_handler,
> +       .name           = "arch_bt",
> +};
> +
>  static int __init register_trigger_all_cpu_backtrace(void)
>  {
> -       register_nmi_handler(NMI_LOCAL, arch_trigger_all_cpu_backtrace_handler,
> -                               0, "arch_bt");
> +       register_nmi_handler(NMI_LOCAL, &arch_trigger_all_cpu_bt_nmiaction);
>         return 0;
>  } 

If you look at things like cpu_notifier() you can shorten this still:

#define nmi_notifier(t, fn, n)				\
do {							\
	static struct nmiaction fn##_na = {		\
		.handler = (fn),			\
		.name = (n),				\
	};						\
	register_nmi_handler((t), &fn##_na);		\
} while (0)

The whole thing then becomes:

  nmi_notifier(NMI_LOCAL, arch_trigger_all_cpu_backtrace_handler, "arch_bt");

Re: [PATCH v2 x86 1/2] fix page faults by nmiaction in nmi if kmemcheck is enabled

[Peter Zijlstra]

On Mon, 2012-03-05 at 10:54 -0500, Don Zickus wrote:
> This is one way of doing this.  I was trying to avoid this when I rewrote the
> nmi handlers, because everyone kept screwing up the structs.  I thought it
> would be safer to have callers pass in data based on an api instead.

Apparently kmemcheck marks pages as non-present and does magic in the
fault handler. Having the action thing allocated meant kmemcheck also
marks that thing as non-present in the page-tables, the list iteration
from NMI context would then fault and things would go funny.

There's two ways out, help kmemcheck with a new annotation (which of
course starts with checking if there isn't already such a thing).

Or this one, avoid the action things from being allocated, this
side-steps kmemcheck and avoids the problem thusly.

Sadly this patch doesn't at all mention the first possibility and why
that isn't a feasible approach. A well...

Re: [PATCH v2 x86 1/2] fix page faults by nmiaction in nmi if kmemcheck is enabled

[Don Zickus]

On Mon, Mar 05, 2012 at 06:49:07PM +0100, Peter Zijlstra wrote:
> On Mon, 2012-03-05 at 18:05 +0800, Li Zhong wrote:
> > +static struct nmiaction arch_trigger_all_cpu_bt_nmiaction = {
> > +       .handler        = arch_trigger_all_cpu_backtrace_handler,
> > +       .name           = "arch_bt",
> > +};
> > +
> >  static int __init register_trigger_all_cpu_backtrace(void)
> >  {
> > -       register_nmi_handler(NMI_LOCAL, arch_trigger_all_cpu_backtrace_handler,
> > -                               0, "arch_bt");
> > +       register_nmi_handler(NMI_LOCAL, &arch_trigger_all_cpu_bt_nmiaction);
> >         return 0;
> >  } 
> 
> If you look at things like cpu_notifier() you can shorten this still:
> 
> #define nmi_notifier(t, fn, n)				\
> do {							\
> 	static struct nmiaction fn##_na = {		\
> 		.handler = (fn),			\
> 		.name = (n),				\
> 	};						\
> 	register_nmi_handler((t), &fn##_na);		\
> } while (0)
> 
> The whole thing then becomes:
> 
>   nmi_notifier(NMI_LOCAL, arch_trigger_all_cpu_backtrace_handler, "arch_bt");

Hi Li,

I would be open to this suggestion.  If you want to modify the patch for
this approach.

Cheers,
Don

Re: [PATCH v2 x86 1/2] fix page faults by nmiaction in nmi if kmemcheck is enabled

[Li Zhong]

On Mon, 2012-03-05 at 11:29 +0100, Wim Van Sebroeck wrote:
> Hi Li,
> 
> > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> > index 8464ea1..e575e63 100644
> > --- a/drivers/watchdog/hpwdt.c
> > +++ b/drivers/watchdog/hpwdt.c
> > @@ -689,9 +689,22 @@ static void __devinit dmi_find_icru(const struct dmi_header *dm, void *dummy)
> >  	}
> >  }
> >  
> > +static struct nmiaction hpwdt_nmiaction[] = {
> > +	{
> > +		.handler	= hpwdt_pretimeout,
> > +		.name		= "hpwdt",
> > +	},
> > +	{
> > +		.handler	= hpwdt_pretimeout,
> > +		.flags		= NMI_FLAG_FIRST,
> > +		.name		= "hpwdt",
> > +	},
> > +};
> > +
> >  static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev)
> >  {
> >  	int retval;
> > +	struct nmiaction *na = hpwdt_nmiaction;
> >  
> >  	/*
> >  	 * On typical CRU-based systems we need to map that service in
> > @@ -733,9 +746,11 @@ static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev)
> >  	 * die notify list to handle a critical NMI. The default is to
> >  	 * be last so other users of the NMI signal can function.
> >  	 */
> > -	retval = register_nmi_handler(NMI_UNKNOWN, hpwdt_pretimeout,
> > -					(priority) ? NMI_FLAG_FIRST : 0,
> > -					"hpwdt");
> > +
> > +	if (priority)
> > +		na = &hpwdt_nmiaction[1];
> > +
> > +	retval = register_nmi_handler(NMI_UNKNOWN, na);
> >  	if (retval != 0) {
> >  		dev_warn(&dev->dev,
> >  			"Unable to register a die notifier (err=%d).\n",
> 
> Why not do something like;
> 
> static struct nmiaction hpwdt_nmiaction = {
> 	.handler	= hpwdt_pretimeout,
> 	.name		= "hpwdt",
> };
> 
> ...
> 	if (priority)
> 		hpwdt_nmiaction.flags = NMI_FLAG_FIRST;
> ...
> 

Thank you, Wim. I'll update it. 

Thanks,
Zhong

> Kind regards,
> Wim.
> 

Re: [PATCH v3 x86 1/2] fix page faults by nmiaction in nmi if kmemcheck is enabled

[Li Zhong]

This patch tries to fix the problem of page fault exception caused by
accessing nmiaction structure in nmi if kmemcheck is enabled. 

If kmemcheck is enabled, the memory allocated through slab are in pages
that are marked non-present, so that some checks could be done in the
page fault handling code ( e.g. whether the memory is read before
written to ). 
As nmiaction is allocated in this way, so it resides in a non-present
page. Then there is a page fault while the nmi code accessing the
nmiaction structure, which would then cause a warning by
WARN_ON_ONCE(in_nmi()) in kmemcheck_fault(), called by do_page_fault().

v2: as Peter suggested, changed the nmiaction to use static storage.

v3: as Peter suggested, use macro to shorten the codes. Also keep the
original usage of register_nmi_handler, so users of this call doesn't
need change. 

Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
 arch/x86/include/asm/nmi.h |   19 ++++++++++++++-
 arch/x86/kernel/nmi.c      |   52
++++++--------------------------------------
 2 files changed, 24 insertions(+), 47 deletions(-)

diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
index fd3f9f1..5a2b2c6 100644
--- a/arch/x86/include/asm/nmi.h
+++ b/arch/x86/include/asm/nmi.h
@@ -35,8 +35,23 @@ enum {
 
 typedef int (*nmi_handler_t)(unsigned int, struct pt_regs *);
 
-int register_nmi_handler(unsigned int, nmi_handler_t, unsigned long,
-			 const char *);
+struct nmiaction {
+	struct list_head list;
+	nmi_handler_t handler;
+	unsigned int flags;
+	const char *name;
+};
+
+#define register_nmi_handler(t, fn, fg, n)		\
+({							\
+	static struct nmiaction fn##_na = {		\
+		.handler = (fn),			\
+		.name = (n),				\
+	};						\
+	__register_nmi_handler((t), (fg), &fn##_na);	\
+})
+
+int __register_nmi_handler(unsigned int, unsigned int, struct nmiaction
*);
 
 void unregister_nmi_handler(unsigned int, const char *);
 
diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 47acaf3..a097559 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -31,14 +31,6 @@
 #include <asm/nmi.h>
 #include <asm/x86_init.h>
 
-#define NMI_MAX_NAMELEN	16
-struct nmiaction {
-	struct list_head list;
-	nmi_handler_t handler;
-	unsigned int flags;
-	char *name;
-};
-
 struct nmi_desc {
 	spinlock_t lock;
 	struct list_head head;
@@ -160,51 +152,21 @@ static struct nmiaction *__free_nmi(unsigned int
type, const char *name)
 	return (n);
 }
 
-int register_nmi_handler(unsigned int type, nmi_handler_t handler,
-			unsigned long nmiflags, const char *devname)
+int __register_nmi_handler(unsigned int type, unsigned int nmiflags,
+						struct nmiaction *na)
 {
-	struct nmiaction *action;
-	int retval = -ENOMEM;
-
-	if (!handler)
+	if (!na->handler)
 		return -EINVAL;
 
-	action = kzalloc(sizeof(struct nmiaction), GFP_KERNEL);
-	if (!action)
-		goto fail_action;
-
-	action->handler = handler;
-	action->flags = nmiflags;
-	action->name = kstrndup(devname, NMI_MAX_NAMELEN, GFP_KERNEL);
-	if (!action->name)
-		goto fail_action_name;
-
-	retval = __setup_nmi(type, action);
-
-	if (retval)
-		goto fail_setup_nmi;
+	na->flags = nmiflags;
 
-	return retval;
-
-fail_setup_nmi:
-	kfree(action->name);
-fail_action_name:
-	kfree(action);
-fail_action:	
-
-	return retval;
+	return __setup_nmi(type, na);
 }
-EXPORT_SYMBOL_GPL(register_nmi_handler);
+EXPORT_SYMBOL_GPL(__register_nmi_handler);
 
 void unregister_nmi_handler(unsigned int type, const char *name)
 {
-	struct nmiaction *a;
-
-	a = __free_nmi(type, name);
-	if (a) {
-		kfree(a->name);
-		kfree(a);
-	}
+	__free_nmi(type, name);
 }
 
 EXPORT_SYMBOL_GPL(unregister_nmi_handler);
-- 
1.7.1

Re: [PATCH v3 x86 1/2] fix page faults by nmiaction in nmi if kmemcheck is enabled

[Vegard Nossum]

On 6 March 2012 11:09, Li Zhong <zhong@linux.vnet.ibm.com> wrote:
> This patch tries to fix the problem of page fault exception caused by
> accessing nmiaction structure in nmi if kmemcheck is enabled.
>
> If kmemcheck is enabled, the memory allocated through slab are in pages
> that are marked non-present, so that some checks could be done in the
> page fault handling code ( e.g. whether the memory is read before
> written to ).
> As nmiaction is allocated in this way, so it resides in a non-present
> page. Then there is a page fault while the nmi code accessing the
> nmiaction structure, which would then cause a warning by
> WARN_ON_ONCE(in_nmi()) in kmemcheck_fault(), called by do_page_fault().
>
> v2: as Peter suggested, changed the nmiaction to use static storage.
>
> v3: as Peter suggested, use macro to shorten the codes. Also keep the
> original usage of register_nmi_handler, so users of this call doesn't
> need change.
>
> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>

Looks like you've solved this now. Thanks.

For the record, another way to prevent the page fault from happening
in the first place is to set up a new slab cache with the flag
SLAB_NOTRACK. This is different from the GFP_NOTRACK flag which, as
you noted, doesn't prevent page faults, just inhibits
checking/warnings for those memory areas.

It's a bit of a hassle, I admit. Maybe we could create an additional,
separate set of slab caches (using SLAB_NOTRACK) and a new GFP flag
which selects this set of caches instead. This would allow anything
that takes a gfp_t to allocate memory that is guaranteed not to page
fault when using kmemcheck. Pekka, any thoughts?


Vegard

Re: [PATCH v3 x86 1/2] fix page faults by nmiaction in nmi if kmemcheck is enabled

[Don Zickus]

On Tue, Mar 06, 2012 at 06:09:43PM +0800, Li Zhong wrote:
> This patch tries to fix the problem of page fault exception caused by
> accessing nmiaction structure in nmi if kmemcheck is enabled. 
> 
> If kmemcheck is enabled, the memory allocated through slab are in pages
> that are marked non-present, so that some checks could be done in the
> page fault handling code ( e.g. whether the memory is read before
> written to ). 
> As nmiaction is allocated in this way, so it resides in a non-present
> page. Then there is a page fault while the nmi code accessing the
> nmiaction structure, which would then cause a warning by
> WARN_ON_ONCE(in_nmi()) in kmemcheck_fault(), called by do_page_fault().
> 
> v2: as Peter suggested, changed the nmiaction to use static storage.
> 
> v3: as Peter suggested, use macro to shorten the codes. Also keep the
> original usage of register_nmi_handler, so users of this call doesn't
> need change. 

Thanks Li!  Looks fine to me.  I'll run some tests to make sure things
still work correctly.

Cheers,
Don

Re: [PATCH v3 x86 1/2] fix page faults by nmiaction in nmi if kmemcheck is enabled

[Li Zhong]

On Tue, 2012-03-06 at 11:27 +0100, Vegard Nossum wrote:
> On 6 March 2012 11:09, Li Zhong <zhong@linux.vnet.ibm.com> wrote:
> > This patch tries to fix the problem of page fault exception caused by
> > accessing nmiaction structure in nmi if kmemcheck is enabled.
> >
> > If kmemcheck is enabled, the memory allocated through slab are in pages
> > that are marked non-present, so that some checks could be done in the
> > page fault handling code ( e.g. whether the memory is read before
> > written to ).
> > As nmiaction is allocated in this way, so it resides in a non-present
> > page. Then there is a page fault while the nmi code accessing the
> > nmiaction structure, which would then cause a warning by
> > WARN_ON_ONCE(in_nmi()) in kmemcheck_fault(), called by do_page_fault().
> >
> > v2: as Peter suggested, changed the nmiaction to use static storage.
> >
> > v3: as Peter suggested, use macro to shorten the codes. Also keep the
> > original usage of register_nmi_handler, so users of this call doesn't
> > need change.
> >
> > Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> 
> Looks like you've solved this now. Thanks.

There is still one place [2/2] not solved ... and I guess it might need
the way you suggested below.

> 
> For the record, another way to prevent the page fault from happening
> in the first place is to set up a new slab cache with the flag
> SLAB_NOTRACK. This is different from the GFP_NOTRACK flag which, as
> you noted, doesn't prevent page faults, just inhibits
> checking/warnings for those memory areas.
> 
> It's a bit of a hassle, I admit. Maybe we could create an additional,
> separate set of slab caches (using SLAB_NOTRACK) and a new GFP flag
> which selects this set of caches instead. This would allow anything
> that takes a gfp_t to allocate memory that is guaranteed not to page
> fault when using kmemcheck. Pekka, any thoughts?
> 

I'm not sure whether I understand it correctly? 
  If CONFIG_KMEMCHECK is enabled, create another two sets of
malloc_sizes caches, one for cs_cachep, one for cs_dmacachep, both with
SLAB_NOTRACK flag. 

  Create a new GFP flag, like __GFP_NO_PF for those places where page
fault is not allowed, and return memory from the caches created above.
This new GFP flag is set to 0 if CONFIG_KMEMCHECK is not enabled. 

I think there shouldn't be many such cases, so most of these caches
wouldn't actually be used ...

Thanks,
Zhong

> 
> Vegard
>