[PATCH 0/1] sysrq is half-enabled by default

[PATCH 0/1] sysrq is half-enabled by default

[Andy Whitcroft]

It seems that recent cleanups to sysrq handling have lead to us having
two copies of the sysrq enable status.  At boot these are currently
inconsistently specified leading to sysrq actually being enabled but
reported as not enabled in sysfs.  Following this email is a patch to
link both of these copies to a common configuration setting.

-apw

Andy Whitcroft (1):
  Input: sysrq -- ensure sysrq_enabled and __sysrq_enabled are
    consistent

 drivers/tty/sysrq.c |    2 +-
 kernel/sysctl.c     |    3 ++-
 lib/Kconfig.debug   |    9 +++++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

-- 
1.7.2.3

[PATCH 1/1] Input: sysrq -- ensure sysrq_enabled and __sysrq_enabled are consistent

[Andy Whitcroft]

Currently sysrq_enabled and __sysrq_enabled are initialised separatly
and inconsitantly, leading to sysrq being actually enabled by reported
as not enabled in sysfs.  The first change to the sysfs configurable
synchronises these two:

    static int __read_mostly sysrq_enabled = 1;
    static int __sysrq_enabled;

Add a new configuration value which sets the default for these preventing
them becoming out of sync again.  Default this to 1 to mirror previous
behaviour.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 drivers/tty/sysrq.c |    2 +-
 kernel/sysctl.c     |    3 ++-
 lib/Kconfig.debug   |    9 +++++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index c556ed9..ba8cf87 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -46,7 +46,7 @@
 #include <asm/irq_regs.h>
 
 /* Whether we react on sysrq keys or just ignore them */
-static int __read_mostly sysrq_enabled = 1;
+static int __read_mostly sysrq_enabled = CONFIG_MAGIC_SYSRQ_DEFAULT;
 static bool __read_mostly sysrq_always_enabled;
 
 static bool sysrq_on(void)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index bc86bb3..85fd935 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -170,7 +170,8 @@ static int proc_taint(struct ctl_table *table, int write,
 #endif
 
 #ifdef CONFIG_MAGIC_SYSRQ
-static int __sysrq_enabled; /* Note: sysrq code ises it's own private copy */
+/* Note: sysrq code uses it's own private copy */
+static int __sysrq_enabled = CONFIG_MAGIC_SYSRQ_DEFAULT;
 
 static int sysrq_sysctl_handler(ctl_table *table, int write,
 				void __user *buffer, size_t *lenp,
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 2d05adb..459105e 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -50,6 +50,15 @@ config MAGIC_SYSRQ
 	  keys are documented in <file:Documentation/sysrq.txt>. Don't say Y
 	  unless you really know what this hack does.
 
+config MAGIC_SYSRQ_DEFAULT
+	int "Magic SysRq key default"
+	default 1
+        range 0 1
+	help
+	  Set the default value for the sysrq sysfs control to this value.
+	  Setting this to 1 will enable sysrq on boot, to 0 will disable
+	  sysrq on boot.
+
 config STRIP_ASM_SYMS
 	bool "Strip assembler-generated symbols during link"
 	default n
-- 
1.7.2.3

Re: [PATCH 0/1] sysrq is half-enabled by default

[Dmitry Torokhov]

Hi Andy,

On Sat, Jan 15, 2011 at 08:23:56PM +0000, Andy Whitcroft wrote:
> It seems that recent cleanups to sysrq handling have lead to us having
> two copies of the sysrq enable status.  At boot these are currently
> inconsistently specified leading to sysrq actually being enabled but
> reported as not enabled in sysfs.  Following this email is a patch to
> link both of these copies to a common configuration setting.
> 

Thanks for noticing this, but I do not think we need a new configuration
parameter. We did not have it for about 10 years and distributions know
how to disable it upon boot if they want to.

Do youthink you could cange the patch and add a default value (1) to
inlude/linux/sysrq.h and use it in sysctl and the handler code?

Thanks!

-- 
Dmitry

Re: [PATCH 0/1] sysrq is half-enabled by default

[Andy Whitcroft]

On Sat, Jan 15, 2011 at 01:54:50PM -0800, Dmitry Torokhov wrote:
> Hi Andy,
> 
> On Sat, Jan 15, 2011 at 08:23:56PM +0000, Andy Whitcroft wrote:
> > It seems that recent cleanups to sysrq handling have lead to us having
> > two copies of the sysrq enable status.  At boot these are currently
> > inconsistently specified leading to sysrq actually being enabled but
> > reported as not enabled in sysfs.  Following this email is a patch to
> > link both of these copies to a common configuration setting.
> > 
> 
> Thanks for noticing this, but I do not think we need a new configuration
> parameter. We did not have it for about 10 years and distributions know
> how to disable it upon boot if they want to.
> 
> Do youthink you could cange the patch and add a default value (1) to
> inlude/linux/sysrq.h and use it in sysctl and the handler code?

Sure thing.

-apw

[PATCH 0/1] sysrq is half-enabled by default V2

[Andy Whitcroft]

It seems that recent cleanups to sysrq handling have lead to us having
two copies of the sysrq enable status.  At boot these are currently
inconsistently specified leading to sysrq actually being enabled but
reported as not enabled in sysfs.  Following this email is a patch to
link both of these copies to a common header define to prevent them becoming
out of sync again.

-apw

Andy Whitcroft (1):
  Input: sysrq -- ensure sysrq_enabled and __sysrq_enabled are
    consistent

 drivers/tty/sysrq.c   |    2 +-
 include/linux/sysrq.h |    3 +++
 kernel/sysctl.c       |    3 ++-
 3 files changed, 6 insertions(+), 2 deletions(-)

[PATCH 1/1] Input: sysrq -- ensure sysrq_enabled and __sysrq_enabled are consistent

[Andy Whitcroft]

Currently sysrq_enabled and __sysrq_enabled are initialised separatly
and inconsitantly, leading to sysrq being actually enabled by reported
as not enabled in sysfs.  The first change to the sysfs configurable
synchronises these two:

    static int __read_mostly sysrq_enabled = 1;
    static int __sysrq_enabled;

Add a common define to carry the default for these preventing them becoming
out of sync again.  Default this to 1 to mirror previous behaviour.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 drivers/tty/sysrq.c   |    2 +-
 include/linux/sysrq.h |    3 +++
 kernel/sysctl.c       |    3 ++-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index c556ed9..8e0dd25 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -46,7 +46,7 @@
 #include <asm/irq_regs.h>
 
 /* Whether we react on sysrq keys or just ignore them */
-static int __read_mostly sysrq_enabled = 1;
+static int __read_mostly sysrq_enabled = SYSRQ_DEFAULT_ENABLE;
 static bool __read_mostly sysrq_always_enabled;
 
 static bool sysrq_on(void)
diff --git a/include/linux/sysrq.h b/include/linux/sysrq.h
index 387fa7d..7faf933 100644
--- a/include/linux/sysrq.h
+++ b/include/linux/sysrq.h
@@ -17,6 +17,9 @@
 #include <linux/errno.h>
 #include <linux/types.h>
 
+/* Enable/disable SYSRQ support by default (0==no, 1==yes). */
+#define SYSRQ_DEFAULT_ENABLE	1
+
 /* Possible values of bitmask for enabling sysrq functions */
 /* 0x0001 is reserved for enable everything */
 #define SYSRQ_ENABLE_LOG	0x0002
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index bc86bb3..0f1bd83 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -170,7 +170,8 @@ static int proc_taint(struct ctl_table *table, int write,
 #endif
 
 #ifdef CONFIG_MAGIC_SYSRQ
-static int __sysrq_enabled; /* Note: sysrq code ises it's own private copy */
+/* Note: sysrq code uses it's own private copy */
+static int __sysrq_enabled = SYSRQ_DEFAULT_ENABLE;
 
 static int sysrq_sysctl_handler(ctl_table *table, int write,
 				void __user *buffer, size_t *lenp,
-- 
1.7.1

Re: [PATCH 1/1] Input: sysrq -- ensure sysrq_enabled and __sysrq_enabled are consistent

[Randy Dunlap]

On Mon, 24 Jan 2011 14:06:12 +0000 Andy Whitcroft wrote:

> Currently sysrq_enabled and __sysrq_enabled are initialised separatly
> and inconsitantly, leading to sysrq being actually enabled by reported
> as not enabled in sysfs.  The first change to the sysfs configurable
> synchronises these two:
> 
>     static int __read_mostly sysrq_enabled = 1;
>     static int __sysrq_enabled;
> 
> Add a common define to carry the default for these preventing them becoming
> out of sync again.  Default this to 1 to mirror previous behaviour.
> 
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  drivers/tty/sysrq.c   |    2 +-
>  include/linux/sysrq.h |    3 +++
>  kernel/sysctl.c       |    3 ++-
>  3 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> index c556ed9..8e0dd25 100644
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -46,7 +46,7 @@
>  #include <asm/irq_regs.h>
>  
>  /* Whether we react on sysrq keys or just ignore them */
> -static int __read_mostly sysrq_enabled = 1;
> +static int __read_mostly sysrq_enabled = SYSRQ_DEFAULT_ENABLE;
>  static bool __read_mostly sysrq_always_enabled;
>  
>  static bool sysrq_on(void)
> diff --git a/include/linux/sysrq.h b/include/linux/sysrq.h
> index 387fa7d..7faf933 100644
> --- a/include/linux/sysrq.h
> +++ b/include/linux/sysrq.h
> @@ -17,6 +17,9 @@
>  #include <linux/errno.h>
>  #include <linux/types.h>
>  
> +/* Enable/disable SYSRQ support by default (0==no, 1==yes). */
> +#define SYSRQ_DEFAULT_ENABLE	1
> +
>  /* Possible values of bitmask for enabling sysrq functions */
>  /* 0x0001 is reserved for enable everything */
>  #define SYSRQ_ENABLE_LOG	0x0002
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index bc86bb3..0f1bd83 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -170,7 +170,8 @@ static int proc_taint(struct ctl_table *table, int write,
>  #endif
>  
>  #ifdef CONFIG_MAGIC_SYSRQ
> -static int __sysrq_enabled; /* Note: sysrq code ises it's own private copy */
> +/* Note: sysrq code uses it's own private copy */

                            its

Meta-comment:  There's no good reason to have patch 0/1.  Just put all of the
patch description in patch 1/1.


> +static int __sysrq_enabled = SYSRQ_DEFAULT_ENABLE;
>  
>  static int sysrq_sysctl_handler(ctl_table *table, int write,
>  				void __user *buffer, size_t *lenp,
> -- 


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 1/1] Input: sysrq -- ensure sysrq_enabled and __sysrq_enabled are consistent

[Dmitry Torokhov]

On Mon, Jan 24, 2011 at 02:06:12PM +0000, Andy Whitcroft wrote:
> Currently sysrq_enabled and __sysrq_enabled are initialised separatly
> and inconsitantly, leading to sysrq being actually enabled by reported
> as not enabled in sysfs.  The first change to the sysfs configurable
> synchronises these two:
> 
>     static int __read_mostly sysrq_enabled = 1;
>     static int __sysrq_enabled;
> 
> Add a common define to carry the default for these preventing them becoming
> out of sync again.  Default this to 1 to mirror previous behaviour.
> 
> Signed-off-by: Andy Whitcroft <apw@canonical.com>

Applied and will also tag for stable. Thank you Andy.

-- 
Dmitry