lkml.org 
[lkml]   [2004]   [Oct]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] fix altsysrq deadlock


On Tue, 26 Oct 2004, Andrew Morton wrote:

> Jason Baron <jbaron@redhat.com> wrote:
> >
> >
> > This patch fixes a deadlock in the handle_sysrq function.
> > ...
> >> - __sysrq_lock_table();
> > + if(!__sysrq_trylock_table()) {
> > + if(in_interrupt())
> > + return;
> > + else
> > + __sysrq_lock_table();
> > + }
> > +
>
> This is only a partial solution - __sysrq_trylock_table() is exported to
> modules which do who know what with it and they don't know how to handle
> locking failures - they'll just go ahead and do a spin_unlock() of an
> unlocked lock and mayhem will ensue.

__sysrq_trylock_table was added by the proposed patch and not not exported
to modules.

>
> What we need to do is to move all those inlined functions out of sysrq.h,
> into sysrq.c then withdraw all those exported-to-modules helper functions
> then remove __sysrq_trylock_table() altogether and then use
> spin_lock_irqsave() in the appropriate places.
>

I was reluctant to use the spin_lock_irqsave() approach b/c alt-sysrq
operations have high latency in my experience, so I didn't want to disable
interrupts across them. In fact, I implemented the above suggestion (patch
below) and while running 'echo t > /proc/sysrq-trigger' in a tight loop I
did in fact get irq timeouts:

Oct 27 14:48:55 foo kernel: ide-cd: cmd 0x3 timed out
Oct 27 14:48:55 foo kernel: hda: irq timeout: status=0xd0 { Busy }
Oct 27 14:48:55 foo kernel: hda: irq timeout: error=0xd0LastFailedSense
0x0d
Oct 27 14:48:55 foo kernel: hda: ATAPI reset complete

However, I would consider this scenario a bit contrived.

thanks,

-Jason


--- linux-2.6.9/drivers/char/sysrq.c Wed Oct 27 12:14:08 2004
+++ linux-2.6.9-sysrq/drivers/char/sysrq.c Wed Oct 27 12:25:20 2004
@@ -295,14 +295,6 @@ static int sysrq_key_table_key2index(int
}

/*
- * table lock and unlocking functions, exposed to modules
- */
-
-void __sysrq_lock_table (void) { spin_lock(&sysrq_key_table_lock); }
-
-void __sysrq_unlock_table (void) { spin_unlock(&sysrq_key_table_lock); }
-
-/*
* get and put functions for the table, exposed to modules.
*/

@@ -334,8 +326,9 @@ void __handle_sysrq(int key, struct pt_r
struct sysrq_key_op *op_p;
int orig_log_level;
int i, j;
+ unsigned long flags;

- __sysrq_lock_table();
+ spin_lock_irqsave(&sysrq_key_table_lock, flags);
orig_log_level = console_loglevel;
console_loglevel = 7;
printk(KERN_INFO "SysRq : ");
@@ -357,7 +350,7 @@ void __handle_sysrq(int key, struct pt_r
printk ("\n");
console_loglevel = orig_log_level;
}
- __sysrq_unlock_table();
+ spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
}

/*
@@ -372,8 +365,34 @@ void handle_sysrq(int key, struct pt_reg
__handle_sysrq(key, pt_regs, tty);
}

+int __sysrq_swap_key_ops(int key, struct sysrq_key_op *insert_op_p,
+ struct sysrq_key_op *remove_op_p) {
+
+ int retval;
+ unsigned long flags;
+
+ spin_lock_irqsave(&sysrq_key_table_lock, flags);
+ if (__sysrq_get_key_op(key) == remove_op_p) {
+ __sysrq_put_key_op(key, insert_op_p);
+ retval = 0;
+ } else {
+ retval = -1;
+ }
+ spin_unlock_irqrestore(&sysrq_key_table_lock, flags);
+
+ return retval;
+}
+
+int register_sysrq_key(int key, struct sysrq_key_op *op_p)
+{
+ return __sysrq_swap_key_ops(key, op_p, NULL);
+}
+
+int unregister_sysrq_key(int key, struct sysrq_key_op *op_p)
+{
+ return __sysrq_swap_key_ops(key, NULL, op_p);
+}
+
EXPORT_SYMBOL(handle_sysrq);
-EXPORT_SYMBOL(__sysrq_lock_table);
-EXPORT_SYMBOL(__sysrq_unlock_table);
-EXPORT_SYMBOL(__sysrq_get_key_op);
-EXPORT_SYMBOL(__sysrq_put_key_op);
+EXPORT_SYMBOL(register_sysrq_key);
+EXPORT_SYMBOL(unregister_sysrq_key);
--- linux-2.6.9/include/linux/sysrq.h Mon Oct 18 17:53:06 2004
+++ linux-2.6.9-sysrq/include/linux/sysrq.h Wed Oct 27 12:20:58 2004
@@ -31,49 +31,8 @@ struct sysrq_key_op {

void handle_sysrq(int, struct pt_regs *, struct tty_struct *);
void __handle_sysrq(int, struct pt_regs *, struct tty_struct *);
-
-/*
- * Sysrq registration manipulation functions
- */
-
-void __sysrq_lock_table (void);
-void __sysrq_unlock_table (void);
-struct sysrq_key_op *__sysrq_get_key_op (int key);
-void __sysrq_put_key_op (int key, struct sysrq_key_op *op_p);
-
-extern __inline__ int
-__sysrq_swap_key_ops_nolock(int key, struct sysrq_key_op *insert_op_p,
- struct sysrq_key_op *remove_op_p)
-{
- int retval;
- if (__sysrq_get_key_op(key) == remove_op_p) {
- __sysrq_put_key_op(key, insert_op_p);
- retval = 0;
- } else {
- retval = -1;
- }
- return retval;
-}
-
-extern __inline__ int
-__sysrq_swap_key_ops(int key, struct sysrq_key_op *insert_op_p,
- struct sysrq_key_op *remove_op_p) {
- int retval;
- __sysrq_lock_table();
- retval = __sysrq_swap_key_ops_nolock(key, insert_op_p, remove_op_p);
- __sysrq_unlock_table();
- return retval;
-}
-
-static inline int register_sysrq_key(int key, struct sysrq_key_op *op_p)
-{
- return __sysrq_swap_key_ops(key, op_p, NULL);
-}
-
-static inline int unregister_sysrq_key(int key, struct sysrq_key_op *op_p)
-{
- return __sysrq_swap_key_ops(key, NULL, op_p);
-}
+int register_sysrq_key(int, struct sysrq_key_op *);
+int unregister_sysrq_key(int, struct sysrq_key_op *);

#else

-
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/

\
 
 \ /
  Last update: 2005-03-22 14:07    [W:2.254 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site