[RFC PATCH] sysrq: don't hold the sysrq_key_table_lock during the handler

[RFC PATCH] sysrq: don't hold the sysrq_key_table_lock during the handler

[Xiaotian Feng]

sysrq_key_table_lock is used to protect the sysrq_key_table, make sure
we get/replace the right operation for the sysrq. But in __handle_sysrq,
kernel will hold this lock and disable irqs until we finished op_p->handler().
This may cause false positive watchdog alert when we're doing "show-task-states"
on a system with many tasks.

Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
 drivers/char/sysrq.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/char/sysrq.c b/drivers/char/sysrq.c
index 878ac0c..0856e2e 100644
--- a/drivers/char/sysrq.c
+++ b/drivers/char/sysrq.c
@@ -520,9 +520,11 @@ void __handle_sysrq(int key, struct tty_struct *tty, int check_mask)
 		if (!check_mask || sysrq_on_mask(op_p->enable_mask)) {
 			printk("%s\n", op_p->action_msg);
 			console_loglevel = orig_log_level;
+			spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
 			op_p->handler(key, tty);
 		} else {
 			printk("This sysrq operation is disabled.\n");
+			spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
 		}
 	} else {
 		printk("HELP : ");
@@ -541,8 +543,8 @@ void __handle_sysrq(int key, struct tty_struct *tty, int check_mask)
 		}
 		printk("\n");
 		console_loglevel = orig_log_level;
+		spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
 	}
-	spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
 }
 
 void handle_sysrq(int key, struct tty_struct *tty)
-- 
1.7.2

Re: [RFC PATCH] sysrq: don't hold the sysrq_key_table_lock during the handler

[Neil Horman]

On Mon, Jul 26, 2010 at 05:54:02PM +0800, Xiaotian Feng wrote:
> sysrq_key_table_lock is used to protect the sysrq_key_table, make sure
> we get/replace the right operation for the sysrq. But in __handle_sysrq,
> kernel will hold this lock and disable irqs until we finished op_p->handler().
> This may cause false positive watchdog alert when we're doing "show-task-states"
> on a system with many tasks.
> 
> Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Neil Horman <nhorman@tuxdriver.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> ---
>  drivers/char/sysrq.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/char/sysrq.c b/drivers/char/sysrq.c
> index 878ac0c..0856e2e 100644
> --- a/drivers/char/sysrq.c
> +++ b/drivers/char/sysrq.c
> @@ -520,9 +520,11 @@ void __handle_sysrq(int key, struct tty_struct *tty, int check_mask)
>  		if (!check_mask || sysrq_on_mask(op_p->enable_mask)) {
>  			printk("%s\n", op_p->action_msg);
>  			console_loglevel = orig_log_level;
> +			spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
>  			op_p->handler(key, tty);
>  		} else {
>  			printk("This sysrq operation is disabled.\n");
> +			spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
>  		}
>  	} else {
>  		printk("HELP : ");
> @@ -541,8 +543,8 @@ void __handle_sysrq(int key, struct tty_struct *tty, int check_mask)
>  		}
>  		printk("\n");
>  		console_loglevel = orig_log_level;
> +		spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
>  	}
> -	spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
>  }
>  
>  void handle_sysrq(int key, struct tty_struct *tty)
> -- 
> 1.7.2
> 
> 

This creates the possibility of a race in the handler.  Not that it happens
often, but sysrq keys can be registered and unregistered dynamically.  If that
lock isn't held while we call the keys handler, the code implementing that
handler can live in a module that gets removed while its executing, leading to
an oops, etc.  I think the better solution would be to use an rcu lock here.
Neil

Re: [RFC PATCH] sysrq: don't hold the sysrq_key_table_lock during the handler

[Dmitry Torokhov]

On Mon, Jul 26, 2010 at 06:51:48AM -0400, Neil Horman wrote:
> On Mon, Jul 26, 2010 at 05:54:02PM +0800, Xiaotian Feng wrote:
> > sysrq_key_table_lock is used to protect the sysrq_key_table, make sure
> > we get/replace the right operation for the sysrq. But in __handle_sysrq,
> > kernel will hold this lock and disable irqs until we finished op_p->handler().
> > This may cause false positive watchdog alert when we're doing "show-task-states"
> > on a system with many tasks.
> > 
> > Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Neil Horman <nhorman@tuxdriver.com>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > ---
> >  drivers/char/sysrq.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/char/sysrq.c b/drivers/char/sysrq.c
> > index 878ac0c..0856e2e 100644
> > --- a/drivers/char/sysrq.c
> > +++ b/drivers/char/sysrq.c
> > @@ -520,9 +520,11 @@ void __handle_sysrq(int key, struct tty_struct *tty, int check_mask)
> >  		if (!check_mask || sysrq_on_mask(op_p->enable_mask)) {
> >  			printk("%s\n", op_p->action_msg);
> >  			console_loglevel = orig_log_level;
> > +			spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
> >  			op_p->handler(key, tty);
> >  		} else {
> >  			printk("This sysrq operation is disabled.\n");
> > +			spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
> >  		}
> >  	} else {
> >  		printk("HELP : ");
> > @@ -541,8 +543,8 @@ void __handle_sysrq(int key, struct tty_struct *tty, int check_mask)
> >  		}
> >  		printk("\n");
> >  		console_loglevel = orig_log_level;
> > +		spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
> >  	}
> > -	spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
> >  }
> >  
> >  void handle_sysrq(int key, struct tty_struct *tty)
> > -- 
> > 1.7.2
> > 
> > 
> 
> This creates the possibility of a race in the handler.  Not that it happens
> often, but sysrq keys can be registered and unregistered dynamically.  If that
> lock isn't held while we call the keys handler, the code implementing that
> handler can live in a module that gets removed while its executing, leading to
> an oops, etc.  I think the better solution would be to use an rcu lock here.

I'd simply changed spinlock to a mutex.

-- 
Dmitry

Re: [RFC PATCH] sysrq: don't hold the sysrq_key_table_lock during the handler

[Neil Horman]

On Mon, Jul 26, 2010 at 10:41:54AM -0700, Dmitry Torokhov wrote:
> On Mon, Jul 26, 2010 at 06:51:48AM -0400, Neil Horman wrote:
> > On Mon, Jul 26, 2010 at 05:54:02PM +0800, Xiaotian Feng wrote:
> > > sysrq_key_table_lock is used to protect the sysrq_key_table, make sure
> > > we get/replace the right operation for the sysrq. But in __handle_sysrq,
> > > kernel will hold this lock and disable irqs until we finished op_p->handler().
> > > This may cause false positive watchdog alert when we're doing "show-task-states"
> > > on a system with many tasks.
> > > 
> > > Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
> > > Cc: Ingo Molnar <mingo@elte.hu>
> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Neil Horman <nhorman@tuxdriver.com>
> > > Cc: "David S. Miller" <davem@davemloft.net>
> > > ---
> > >  drivers/char/sysrq.c |    4 +++-
> > >  1 files changed, 3 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/char/sysrq.c b/drivers/char/sysrq.c
> > > index 878ac0c..0856e2e 100644
> > > --- a/drivers/char/sysrq.c
> > > +++ b/drivers/char/sysrq.c
> > > @@ -520,9 +520,11 @@ void __handle_sysrq(int key, struct tty_struct *tty, int check_mask)
> > >  		if (!check_mask || sysrq_on_mask(op_p->enable_mask)) {
> > >  			printk("%s\n", op_p->action_msg);
> > >  			console_loglevel = orig_log_level;
> > > +			spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
> > >  			op_p->handler(key, tty);
> > >  		} else {
> > >  			printk("This sysrq operation is disabled.\n");
> > > +			spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
> > >  		}
> > >  	} else {
> > >  		printk("HELP : ");
> > > @@ -541,8 +543,8 @@ void __handle_sysrq(int key, struct tty_struct *tty, int check_mask)
> > >  		}
> > >  		printk("\n");
> > >  		console_loglevel = orig_log_level;
> > > +		spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
> > >  	}
> > > -	spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
> > >  }
> > >  
> > >  void handle_sysrq(int key, struct tty_struct *tty)
> > > -- 
> > > 1.7.2
> > > 
> > > 
> > 
> > This creates the possibility of a race in the handler.  Not that it happens
> > often, but sysrq keys can be registered and unregistered dynamically.  If that
> > lock isn't held while we call the keys handler, the code implementing that
> > handler can live in a module that gets removed while its executing, leading to
> > an oops, etc.  I think the better solution would be to use an rcu lock here.
> 
> I'd simply changed spinlock to a mutex.
> 
I don't think you can do that safely in this path, as sysrqs will be looked up
in both process (echo t > /proc/sysrq-trigger) context and in interrupt
(alt-sysrq-t) context.  If a mutex is locked and you try to take it in interrupt
context, you get a sleeping-in-interrupt panic IIRC

Neil

> -- 
> Dmitry
> 

Re: [RFC PATCH] sysrq: don't hold the sysrq_key_table_lock during the handler

[Dmitry Torokhov]

On Mon, Jul 26, 2010 at 04:34:20PM -0400, Neil Horman wrote:
> On Mon, Jul 26, 2010 at 10:41:54AM -0700, Dmitry Torokhov wrote:
> > On Mon, Jul 26, 2010 at 06:51:48AM -0400, Neil Horman wrote:
> > > On Mon, Jul 26, 2010 at 05:54:02PM +0800, Xiaotian Feng wrote:
> > > > sysrq_key_table_lock is used to protect the sysrq_key_table, make sure
> > > > we get/replace the right operation for the sysrq. But in __handle_sysrq,
> > > > kernel will hold this lock and disable irqs until we finished op_p->handler().
> > > > This may cause false positive watchdog alert when we're doing "show-task-states"
> > > > on a system with many tasks.
> > > > 
> > > > Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
> > > > Cc: Ingo Molnar <mingo@elte.hu>
> > > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > Cc: Neil Horman <nhorman@tuxdriver.com>
> > > > Cc: "David S. Miller" <davem@davemloft.net>
> > > > ---
> > > >  drivers/char/sysrq.c |    4 +++-
> > > >  1 files changed, 3 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/drivers/char/sysrq.c b/drivers/char/sysrq.c
> > > > index 878ac0c..0856e2e 100644
> > > > --- a/drivers/char/sysrq.c
> > > > +++ b/drivers/char/sysrq.c
> > > > @@ -520,9 +520,11 @@ void __handle_sysrq(int key, struct tty_struct *tty, int check_mask)
> > > >  		if (!check_mask || sysrq_on_mask(op_p->enable_mask)) {
> > > >  			printk("%s\n", op_p->action_msg);
> > > >  			console_loglevel = orig_log_level;
> > > > +			spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
> > > >  			op_p->handler(key, tty);
> > > >  		} else {
> > > >  			printk("This sysrq operation is disabled.\n");
> > > > +			spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
> > > >  		}
> > > >  	} else {
> > > >  		printk("HELP : ");
> > > > @@ -541,8 +543,8 @@ void __handle_sysrq(int key, struct tty_struct *tty, int check_mask)
> > > >  		}
> > > >  		printk("\n");
> > > >  		console_loglevel = orig_log_level;
> > > > +		spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
> > > >  	}
> > > > -	spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
> > > >  }
> > > >  
> > > >  void handle_sysrq(int key, struct tty_struct *tty)
> > > > -- 
> > > > 1.7.2
> > > > 
> > > > 
> > > 
> > > This creates the possibility of a race in the handler.  Not that it happens
> > > often, but sysrq keys can be registered and unregistered dynamically.  If that
> > > lock isn't held while we call the keys handler, the code implementing that
> > > handler can live in a module that gets removed while its executing, leading to
> > > an oops, etc.  I think the better solution would be to use an rcu lock here.
> > 
> > I'd simply changed spinlock to a mutex.
> > 
> I don't think you can do that safely in this path, as sysrqs will be looked up
> in both process (echo t > /proc/sysrq-trigger) context and in interrupt
> (alt-sysrq-t) context.  If a mutex is locked and you try to take it in interrupt
> context, you get a sleeping-in-interrupt panic IIRC
> 

Yes, indeed. But then even RCU will not really help us since keyboard
driver will have inpterrupts disabled anyways.

-- 
Dmitry

Re: [RFC PATCH] sysrq: don't hold the sysrq_key_table_lock during the handler

[Neil Horman]

On Tue, Jul 27, 2010 at 01:15:52AM -0700, Dmitry Torokhov wrote:
> On Mon, Jul 26, 2010 at 04:34:20PM -0400, Neil Horman wrote:
> > On Mon, Jul 26, 2010 at 10:41:54AM -0700, Dmitry Torokhov wrote:
> > > On Mon, Jul 26, 2010 at 06:51:48AM -0400, Neil Horman wrote:
> > > > On Mon, Jul 26, 2010 at 05:54:02PM +0800, Xiaotian Feng wrote:
> > > > <snip>
> > > > 
> > > > This creates the possibility of a race in the handler.  Not that it happens
> > > > often, but sysrq keys can be registered and unregistered dynamically.  If that
> > > > lock isn't held while we call the keys handler, the code implementing that
> > > > handler can live in a module that gets removed while its executing, leading to
> > > > an oops, etc.  I think the better solution would be to use an rcu lock here.
> > > 
> > > I'd simply changed spinlock to a mutex.
> > > 
> > I don't think you can do that safely in this path, as sysrqs will be looked up
> > in both process (echo t > /proc/sysrq-trigger) context and in interrupt
> > (alt-sysrq-t) context.  If a mutex is locked and you try to take it in interrupt
> > context, you get a sleeping-in-interrupt panic IIRC
> > 
> 
> Yes, indeed. But then even RCU will not really help us since keyboard
> driver will have inpterrupts disabled anyways.
> 

Hm, thats true.  I suppose the right thing to do then is grab a reference on any
sysrq implemented within code that might be considered transient before
releasing the lock.  I've not tested this patch out, but it should do what we
need, in that it allows us to release the lock without having to worry about the
op list changing underneath us, or having the module with the handler code
dissappear

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>



 arch/arm/kernel/etm.c           |    1 +
 arch/powerpc/xmon/xmon.c        |    1 +
 arch/sparc/kernel/process_64.c  |    1 +
 drivers/char/sysrq.c            |   19 ++++++++++++++++++-
 drivers/gpu/drm/drm_fb_helper.c |    1 +
 drivers/net/ibm_newemac/debug.c |    1 +
 include/linux/sysrq.h           |    1 +
 kernel/debug/debug_core.c       |    1 +
 kernel/power/poweroff.c         |    1 +
 9 files changed, 26 insertions(+), 1 deletion(-)


diff --git a/arch/arm/kernel/etm.c b/arch/arm/kernel/etm.c
index 8277539..fb7c393 100644
--- a/arch/arm/kernel/etm.c
+++ b/arch/arm/kernel/etm.c
@@ -240,6 +240,7 @@ static struct sysrq_key_op sysrq_etm_op = {
 	.handler = sysrq_etm_dump,
 	.help_msg = "ETM buffer dump",
 	.action_msg = "etm",
+	.module = THIS_MODULE,
 };
 
 static int etb_open(struct inode *inode, struct file *file)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 8bad7d5..b9b7aee 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -2740,6 +2740,7 @@ static struct sysrq_key_op sysrq_xmon_op =
 	.handler =	sysrq_handle_xmon,
 	.help_msg =	"Xmon",
 	.action_msg =	"Entering xmon",
+	.module =	THIS_MODULE,
 };
 
 static int __init setup_xmon_sysrq(void)
diff --git a/arch/sparc/kernel/process_64.c b/arch/sparc/kernel/process_64.c
index dbe81a3..f5a2efc 100644
--- a/arch/sparc/kernel/process_64.c
+++ b/arch/sparc/kernel/process_64.c
@@ -312,6 +312,7 @@ static struct sysrq_key_op sparc_globalreg_op = {
 	.handler	= sysrq_handle_globreg,
 	.help_msg	= "Globalregs",
 	.action_msg	= "Show Global CPU Regs",
+	.module		= THIS_MODULE,
 };
 
 static int __init sparc_globreg_init(void)
diff --git a/drivers/char/sysrq.c b/drivers/char/sysrq.c
index 878ac0c..3dd4034 100644
--- a/drivers/char/sysrq.c
+++ b/drivers/char/sysrq.c
@@ -520,7 +520,24 @@ void __handle_sysrq(int key, struct tty_struct *tty, int check_mask)
 		if (!check_mask || sysrq_on_mask(op_p->enable_mask)) {
 			printk("%s\n", op_p->action_msg);
 			console_loglevel = orig_log_level;
-			op_p->handler(key, tty);
+
+			/*
+			 * We want to run the handler any time we can get
+			 * a reference on the module, or anytime we don't
+			 * have any module to get.  In both of these cases
+			 * its safe to do a module_put, as NULLS are skipped
+			 * there.
+			 */
+			if ((try_module_get(op_p->module) == 0) ||
+			    (!op_p->module)) {
+				spin_unlock_irqrestore(&sysrq_key_table_lock,
+						       flags);
+				op_p->handler(key, tty);
+				module_put(op_p->module);
+			} else
+				printk("Could not lock this sysrq key\n");
+
+			return;
 		} else {
 			printk("This sysrq operation is disabled.\n");
 		}
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 7196620..623e701 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -304,6 +304,7 @@ static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = {
 	.handler = drm_fb_helper_sysrq,
 	.help_msg = "force-fb(V)",
 	.action_msg = "Restore framebuffer console",
+	.module	= THIS_MODULE,
 };
 #else
 static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { };
diff --git a/drivers/net/ibm_newemac/debug.c b/drivers/net/ibm_newemac/debug.c
index 3995faf..b82aa35 100644
--- a/drivers/net/ibm_newemac/debug.c
+++ b/drivers/net/ibm_newemac/debug.c
@@ -247,6 +247,7 @@ static struct sysrq_key_op emac_sysrq_op = {
 	.handler = emac_sysrq_handler,
 	.help_msg = "emaC",
 	.action_msg = "Show EMAC(s) status",
+	.module	= THIS_MODULE,
 };
 
 int __init emac_init_debug(void)
diff --git a/include/linux/sysrq.h b/include/linux/sysrq.h
index 609e8ca..4f219de 100644
--- a/include/linux/sysrq.h
+++ b/include/linux/sysrq.h
@@ -35,6 +35,7 @@ struct sysrq_key_op {
 	char *help_msg;
 	char *action_msg;
 	int enable_mask;
+	struct module *module;
 };
 
 #ifdef CONFIG_MAGIC_SYSRQ
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index 8bc5eef..6b64063 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -761,6 +761,7 @@ static struct sysrq_key_op sysrq_dbg_op = {
 	.handler	= sysrq_handle_dbg,
 	.help_msg	= "debug(G)",
 	.action_msg	= "DEBUG",
+	.module		= THIS_MODULE,
 };
 #endif
 
diff --git a/kernel/power/poweroff.c b/kernel/power/poweroff.c
index e8b3370..58df039 100644
--- a/kernel/power/poweroff.c
+++ b/kernel/power/poweroff.c
@@ -35,6 +35,7 @@ static struct sysrq_key_op	sysrq_poweroff_op = {
 	.help_msg       = "powerOff",
 	.action_msg     = "Power Off",
 	.enable_mask	= SYSRQ_ENABLE_BOOT,
+	.module		= THIS_MODULE,
 };
 
 static int pm_sysrq_init(void)

> -- 
> Dmitry
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [RFC PATCH] sysrq: don't hold the sysrq_key_table_lock during the handler

[Dmitry Torokhov]

On Tue, Jul 27, 2010 at 07:57:54AM -0400, Neil Horman wrote:
> On Tue, Jul 27, 2010 at 01:15:52AM -0700, Dmitry Torokhov wrote:
> > On Mon, Jul 26, 2010 at 04:34:20PM -0400, Neil Horman wrote:
> > > On Mon, Jul 26, 2010 at 10:41:54AM -0700, Dmitry Torokhov wrote:
> > > > On Mon, Jul 26, 2010 at 06:51:48AM -0400, Neil Horman wrote:
> > > > > On Mon, Jul 26, 2010 at 05:54:02PM +0800, Xiaotian Feng wrote:
> > > > > <snip>
> > > > > 
> > > > > This creates the possibility of a race in the handler.  Not that it happens
> > > > > often, but sysrq keys can be registered and unregistered dynamically.  If that
> > > > > lock isn't held while we call the keys handler, the code implementing that
> > > > > handler can live in a module that gets removed while its executing, leading to
> > > > > an oops, etc.  I think the better solution would be to use an rcu lock here.
> > > > 
> > > > I'd simply changed spinlock to a mutex.
> > > > 
> > > I don't think you can do that safely in this path, as sysrqs will be looked up
> > > in both process (echo t > /proc/sysrq-trigger) context and in interrupt
> > > (alt-sysrq-t) context.  If a mutex is locked and you try to take it in interrupt
> > > context, you get a sleeping-in-interrupt panic IIRC
> > > 
> > 
> > Yes, indeed. But then even RCU will not really help us since keyboard
> > driver will have inpterrupts disabled anyways.
> > 
> 
> Hm, thats true.  I suppose the right thing to do then is grab a reference on any
> sysrq implemented within code that might be considered transient before
> releasing the lock.  I've not tested this patch out, but it should do what we
> need, in that it allows us to release the lock without having to worry about the
> op list changing underneath us, or having the module with the handler code
> dissappear
> 

That would only help if you also offload execution to a workqueue (which
may not be desirable in all cases) since keyboard driver^H^H input core
still calls into SysRq code holding [another] spinlock with interrupts
disabled.

-- 
Dmitry

Re: [RFC PATCH] sysrq: don't hold the sysrq_key_table_lock during the handler

[Neil Horman]

On Tue, Jul 27, 2010 at 09:38:52AM -0700, Dmitry Torokhov wrote:
> On Tue, Jul 27, 2010 at 07:57:54AM -0400, Neil Horman wrote:
> > On Tue, Jul 27, 2010 at 01:15:52AM -0700, Dmitry Torokhov wrote:
> > > On Mon, Jul 26, 2010 at 04:34:20PM -0400, Neil Horman wrote:
> > > > On Mon, Jul 26, 2010 at 10:41:54AM -0700, Dmitry Torokhov wrote:
> > > > > On Mon, Jul 26, 2010 at 06:51:48AM -0400, Neil Horman wrote:
> > > > > > On Mon, Jul 26, 2010 at 05:54:02PM +0800, Xiaotian Feng wrote:
> > > > > > <snip>
> > > > > > 
> > > > > > This creates the possibility of a race in the handler.  Not that it happens
> > > > > > often, but sysrq keys can be registered and unregistered dynamically.  If that
> > > > > > lock isn't held while we call the keys handler, the code implementing that
> > > > > > handler can live in a module that gets removed while its executing, leading to
> > > > > > an oops, etc.  I think the better solution would be to use an rcu lock here.
> > > > > 
> > > > > I'd simply changed spinlock to a mutex.
> > > > > 
> > > > I don't think you can do that safely in this path, as sysrqs will be looked up
> > > > in both process (echo t > /proc/sysrq-trigger) context and in interrupt
> > > > (alt-sysrq-t) context.  If a mutex is locked and you try to take it in interrupt
> > > > context, you get a sleeping-in-interrupt panic IIRC
> > > > 
> > > 
> > > Yes, indeed. But then even RCU will not really help us since keyboard
> > > driver will have inpterrupts disabled anyways.
> > > 
> > 
> > Hm, thats true.  I suppose the right thing to do then is grab a reference on any
> > sysrq implemented within code that might be considered transient before
> > releasing the lock.  I've not tested this patch out, but it should do what we
> > need, in that it allows us to release the lock without having to worry about the
> > op list changing underneath us, or having the module with the handler code
> > dissappear
> > 
> 
> That would only help if you also offload execution to a workqueue (which
> may not be desirable in all cases) since keyboard driver^H^H input core
> still calls into SysRq code holding [another] spinlock with interrupts
> disabled.
> 

Um, no, I don't think so.  The concern that I had with the patch was that after
you unlock that spinlock, a module which previously registered a sysrq handler
could be removed  during its execution leaving it executing in unknown memory.
By doing a successful try_module_get we prevent the module remove code from
deleting a module from the kernel, avoiding that condition until the execution
of the requested sysrq handler completes.  Offloading execution of the handler
to a workqueue does nothing here, unless you see another problem, independent of
the one I was addressing.

I suppose there is a possibiliy that the o_op value could change after we unlock
the lock, but we could manage that by copying the pointer (although I don't
think its needed unless some module tries to unregister sysrq handlers outside
of the module_exit routine it has.

Neil

> -- 
> Dmitry
> 

Re: [RFC PATCH] sysrq: don't hold the sysrq_key_table_lock during the handler

[Andrew Morton]

On Mon, 26 Jul 2010 17:54:02 +0800
Xiaotian Feng <dfeng@redhat.com> wrote:

> sysrq_key_table_lock is used to protect the sysrq_key_table, make sure
> we get/replace the right operation for the sysrq. But in __handle_sysrq,
> kernel will hold this lock and disable irqs until we finished op_p->handler().
> This may cause false positive watchdog alert when we're doing "show-task-states"
> on a system with many tasks.
> 

It would be better to find a suitable point in an inner loop and add an
appropriately-commented touch_nmi_watchdog().

That way the problem gets fixed for all irqs-off callers, not just one
of them.