Re: [PATCH v2] x86,nmi: Fix page faults by nmiaction if kmemcheck is enabled

[Li Zhong <zhong@linux.vnet.ibm.com>]

On Thu, 2012-03-08 at 16:02 -0500, Don Zickus wrote:
> From: Li Zhong <zhong@linux.vnet.ibm.com>
> 
> 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.
> 
> [simplified wrappers -dcz]
> 
> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> Signed-off-by: Don Zickus <dzickus@redhat.com>
> ---
>  arch/x86/include/asm/nmi.h |   20 ++++++++++++-
>  arch/x86/kernel/nmi.c      |   65 ++++---------------------------------------
>  2 files changed, 24 insertions(+), 61 deletions(-)
> 
> diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
> index fd3f9f1..2a17e57 100644
> --- a/arch/x86/include/asm/nmi.h
> +++ b/arch/x86/include/asm/nmi.h
> @@ -35,8 +35,24 @@ 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),				\
> +		.flags = (fg),				\
> +	};						\
> +	__register_nmi_handler((t), &fn##_na);	\
> +})

Thank you, Don. 

As flags is moved into the macro, it may cause following compile error, 
drivers/watchdog/hpwdt.c: In function 'hpwdt_init_nmi_decoding':
drivers/watchdog/hpwdt.c:737: error: initializer element is not constant
drivers/watchdog/hpwdt.c:737: error: (near initialization for
'hpwdt_pretimeout_na.flags')

So following fix might be needed: 

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 3c166d3..e1161ea 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -734,9 +734,12 @@ 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)
+		retval = register_nmi_handler(NMI_UNKNOWN, hpwdt_pretimeout,
+					NMI_FLAG_FIRST, "hpwdt");
+	else
+		retval = register_nmi_handler(NMI_UNKNOWN, hpwdt_pretimeout,
+					0, "hpwdt");
 	if (retval != 0) {
 		dev_warn(&dev->dev,
 			"Unable to register a die notifier (err=%d).\n",