[PATCH] fix altsysrq deadlock

[PATCH] fix altsysrq deadlock

[Jason Baron]

hi,

This patch fixes a deadlock in the handle_sysrq function. The handle_sysrq
function is called in both process as well as interrupt context.  The
__sysrq_lock_table() function does not disable interrupts when taking the
sysrq_key_table_lock. Thus, it is fairly easy to to do a keyboard
sys-altrq-foo, while a 'echo foo > /proc/sysrq-trigger' is running and
cause a deadlock. It can be annoying to have the machine lockup exactly
when you are trying to debug something else.

Usually a spin_lock_irqsave would be used. However, I don't think that is
appropriate here because some of the altsysrq functions can take a while 
to run.

Thus, the patch below simply uses a spin_trylock and on failure does not
spin if we are in interrupt context. A trylock in all cases would be ok
too, but this patch preserves existing behavior as much as possible.  An
altsyrq that produces no output might seem troublesome, but it is
primarily used as a debugging tool, so trying it again seems reasonable.

Please apply.

-Jason 

--- linux/drivers/char/sysrq.c.orig	Tue Oct 26 12:49:59 2004
+++ linux/drivers/char/sysrq.c	Tue Oct 26 12:54:51 2004
@@ -291,6 +291,8 @@ void __sysrq_lock_table (void) { spin_lo
 
 void __sysrq_unlock_table (void) { spin_unlock(&sysrq_key_table_lock); }
 
+#define __sysrq_trylock_table() spin_trylock(&sysrq_key_table_lock)
+
 /*
  * get and put functions for the table, exposed to modules.
  */
@@ -324,7 +326,13 @@ void __handle_sysrq(int key, struct pt_r
 	int orig_log_level;
 	int i, j;
 
-	__sysrq_lock_table();
+	if(!__sysrq_trylock_table()) {
+		if(in_interrupt())
+			return;
+		else
+			__sysrq_lock_table();
+	}
+
 	orig_log_level = console_loglevel;
 	console_loglevel = 7;
 	printk(KERN_INFO "SysRq : ");

Re: [PATCH] fix altsysrq deadlock

[John Richard Moser]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



Jason Baron wrote:
| hi,
|

HI!  ^_^

[...]

|  An
| altsyrq that produces no output might seem troublesome, but it is
| primarily used as a debugging tool, so trying it again seems reasonable.

Actually, I use sysrq as if it's just another feature.  It should (I
think it does. . . not sure) only work on the console directly, for
security reasons; but it's great when things like X misbehave, or when
I've damaged something and the system doesn't want to shut down.  AS-E
AS-I AS-U AS-S AS-O.  :)  I actually tried making an N sysrq, for
"semi-Normal shutdown."  It would send TERM, wait 5S, send KILL, wait
5S, unmount, sync, reboot.

Just thought it might be interesting to point out that magic-sysrq can
be a helpful feature for someone not hacking the kernel.

[...]
- --
All content of all messages exchanged herein are left in the
Public Domain, unless otherwise explicitly stated.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFBfozEhDd4aOud5P8RAo/xAJ9sumPFUpGkwIf4ipR2+6g0bmwUYQCdGQ+U
SPvEFbVzUCx+8zdNQqnT8F8=
=OQrV
-----END PGP SIGNATURE-----

Re: [PATCH] fix altsysrq deadlock

[Jason Baron]

On Tue, 26 Oct 2004, John Richard Moser wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> 
> 
> Jason Baron wrote:
> | hi,
> |
> 
> HI!  ^_^
> 
> [...]
> 
> |  An
> | altsyrq that produces no output might seem troublesome, but it is
> | primarily used as a debugging tool, so trying it again seems reasonable.
> 
> Actually, I use sysrq as if it's just another feature.  It should (I
> think it does. . . not sure) only work on the console directly, for
> security reasons; but it's great when things like X misbehave, or when
> I've damaged something and the system doesn't want to shut down.  AS-E
> AS-I AS-U AS-S AS-O.  :)  I actually tried making an N sysrq, for
> "semi-Normal shutdown."  It would send TERM, wait 5S, send KILL, wait
> 5S, unmount, sync, reboot.
> 
> Just thought it might be interesting to point out that magic-sysrq can
> be a helpful feature for someone not hacking the kernel.
> 

The patch only drops a sysrq that is about to cause a system deadlock. So
if you haven't had any deadlocks this patch shouldn't have a noticeable
affect.

If a caller wants to rely on handle_sysrq, then it should not be called
from interrupt context. handle_sysrq can not defer the work, since the
point of sysrq is to be able get information out when the system is
potentially unusable. How would you know when to defer the work and when
not to?

-Jason

Re: [PATCH] fix altsysrq deadlock

[John Richard Moser]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



Jason Baron wrote:
| On Tue, 26 Oct 2004, John Richard Moser wrote:
|
|

[...]

|
|
| The patch only drops a sysrq that is about to cause a system deadlock. So
| if you haven't had any deadlocks this patch shouldn't have a noticeable
| affect.
|
| If a caller wants to rely on handle_sysrq, then it should not be called
| from interrupt context. handle_sysrq can not defer the work, since the
| point of sysrq is to be able get information out when the system is
| potentially unusable. How would you know when to defer the work and when
| not to?

Eh?  Wha?  I was just pointing out that it's useful beyond just
debugging, not proposing any feature changes or stuff.

|
| -Jason
|
|
|
|
|

- --
All content of all messages exchanged herein are left in the
Public Domain, unless otherwise explicitly stated.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFBfpiKhDd4aOud5P8RAlyhAJ9/qfkA27SGGvgZ4WDPOzVm2VaKKQCdHrsk
R4V/S9phuxUVOXB8Zq7d5oI=
=9UZt
-----END PGP SIGNATURE-----

Re: [PATCH] fix altsysrq deadlock

[Andrew Morton]

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.

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.

Re: [PATCH] fix altsysrq deadlock

[Jason Baron]

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