sysrq: don't hold the sysrq_key_table_lock during the handler
in [Kernel]
|
Prev: writeback: the kupdate expire timestamp should be a moving target
Next: hw-breakpoints, kgdb, x86: add a flag topassDIE_DEBUG notification
From: Neil Horman on 26 Jul 2010 07:00 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(a)redhat.com> > Cc: Ingo Molnar <mingo(a)elte.hu> > Cc: Dmitry Torokhov <dmitry.torokhov(a)gmail.com> > Cc: Andrew Morton <akpm(a)linux-foundation.org> > Cc: Neil Horman <nhorman(a)tuxdriver.com> > Cc: "David S. Miller" <davem(a)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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Dmitry Torokhov on 26 Jul 2010 13:50 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(a)redhat.com> > > Cc: Ingo Molnar <mingo(a)elte.hu> > > Cc: Dmitry Torokhov <dmitry.torokhov(a)gmail.com> > > Cc: Andrew Morton <akpm(a)linux-foundation.org> > > Cc: Neil Horman <nhorman(a)tuxdriver.com> > > Cc: "David S. Miller" <davem(a)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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Neil Horman on 26 Jul 2010 16:40 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(a)redhat.com> > > > Cc: Ingo Molnar <mingo(a)elte.hu> > > > Cc: Dmitry Torokhov <dmitry.torokhov(a)gmail.com> > > > Cc: Andrew Morton <akpm(a)linux-foundation.org> > > > Cc: Neil Horman <nhorman(a)tuxdriver.com> > > > Cc: "David S. Miller" <davem(a)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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Dmitry Torokhov on 27 Jul 2010 04:20 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(a)redhat.com> > > > > Cc: Ingo Molnar <mingo(a)elte.hu> > > > > Cc: Dmitry Torokhov <dmitry.torokhov(a)gmail.com> > > > > Cc: Andrew Morton <akpm(a)linux-foundation.org> > > > > Cc: Neil Horman <nhorman(a)tuxdriver.com> > > > > Cc: "David S. Miller" <davem(a)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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Neil Horman on 27 Jul 2010 08:10
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(a)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(a)vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |