[PATCH] efivars: Allow disabling use as a pstore backend

[PATCH] efivars: Allow disabling use as a pstore backend

[Seth Forshee]

We know that with some firmware implementations writing too much data to
UEFI variables can lead to bricking machines. Recent changes attempt to
address this issue, but for some it may still be prudent to avoid
writing large amounts of data until the solution has been proven on a
wide variety of hardware.

Crash dumps or other data from pstore can potentially be a large data
source. Add a new option, CONFIG_EFI_VARS_PSTORE, which can be set to N
to avoid using efivars as a backend to pstore.

Signed-off-by: Seth Forshee <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 drivers/firmware/Kconfig   |    9 +++++++
 drivers/firmware/efivars.c |   64 ++++++++++++++------------------------------
 2 files changed, 29 insertions(+), 44 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 9b00072..898023d 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -53,6 +53,15 @@ config EFI_VARS
 	  Subsequent efibootmgr releases may be found at:
 	  <http://linux.dell.com/efibootmgr>
 
+config EFI_VARS_PSTORE
+	bool "Register efivars backend for pstore"
+	depends on EFI_VARS && PSTORE
+	default y
+	help
+	  Say Y here to enable use efivars as a backend to pstore. This
+	  will allow writing console messages, crash dumps, or anything
+	  else supported by pstore to EFI variables.
+
 config EFI_PCDP
 	bool "Console device selection via EFI PCDP or HCDP table"
 	depends on ACPI && EFI && IA64
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 7320bf8..73aa2c5 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1253,9 +1253,7 @@ static const struct inode_operations efivarfs_dir_inode_operations = {
 	.create = efivarfs_create,
 };
 
-static struct pstore_info efi_pstore_info;
-
-#ifdef CONFIG_PSTORE
+#if defined(CONFIG_PSTORE) && defined(CONFIG_EFI_VARS_PSTORE)
 
 static int efi_pstore_open(struct pstore_info *psi)
 {
@@ -1459,38 +1457,6 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
 
 	return 0;
 }
-#else
-static int efi_pstore_open(struct pstore_info *psi)
-{
-	return 0;
-}
-
-static int efi_pstore_close(struct pstore_info *psi)
-{
-	return 0;
-}
-
-static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type, int *count,
-			       struct timespec *timespec,
-			       char **buf, struct pstore_info *psi)
-{
-	return -1;
-}
-
-static int efi_pstore_write(enum pstore_type_id type,
-		enum kmsg_dump_reason reason, u64 *id,
-		unsigned int part, int count, size_t size,
-		struct pstore_info *psi)
-{
-	return 0;
-}
-
-static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
-			    struct timespec time, struct pstore_info *psi)
-{
-	return 0;
-}
-#endif
 
 static struct pstore_info efi_pstore_info = {
 	.owner		= THIS_MODULE,
@@ -1502,6 +1468,24 @@ static struct pstore_info efi_pstore_info = {
 	.erase		= efi_pstore_erase,
 };
 
+static void efivar_pstore_register(struct efivars *efivars)
+{
+	efivars->efi_pstore_info = efi_pstore_info;
+	efivars->efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
+	if (efivars->efi_pstore_info.buf) {
+		efivars->efi_pstore_info.bufsize = 1024;
+		efivars->efi_pstore_info.data = efivars;
+		spin_lock_init(&efivars->efi_pstore_info.buf_lock);
+		pstore_register(&efivars->efi_pstore_info);
+	}
+}
+#else
+static void efivar_pstore_register(struct efivars *efivars)
+{
+	return;
+}
+#endif
+
 static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
 			     struct bin_attribute *bin_attr,
 			     char *buf, loff_t pos, size_t count)
@@ -1962,15 +1946,7 @@ int register_efivars(struct efivars *efivars,
 	if (error)
 		unregister_efivars(efivars);
 
-	efivars->efi_pstore_info = efi_pstore_info;
-
-	efivars->efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
-	if (efivars->efi_pstore_info.buf) {
-		efivars->efi_pstore_info.bufsize = 1024;
-		efivars->efi_pstore_info.data = efivars;
-		spin_lock_init(&efivars->efi_pstore_info.buf_lock);
-		pstore_register(&efivars->efi_pstore_info);
-	}
+	efivar_pstore_register(efivars);
 
 	register_filesystem(&efivarfs_type);
 
-- 
1.7.9.5

Re: [PATCH] efivars: Allow disabling use as a pstore backend

[H. Peter Anvin]

On 03/07/2013 09:40 AM, Seth Forshee wrote:
> We know that with some firmware implementations writing too much data to
> UEFI variables can lead to bricking machines. Recent changes attempt to
> address this issue, but for some it may still be prudent to avoid
> writing large amounts of data until the solution has been proven on a
> wide variety of hardware.
> 
> Crash dumps or other data from pstore can potentially be a large data
> source. Add a new option, CONFIG_EFI_VARS_PSTORE, which can be set to N
> to avoid using efivars as a backend to pstore.

What about a command line option?

> -#ifdef CONFIG_PSTORE
> +#if defined(CONFIG_PSTORE) && defined(CONFIG_EFI_VARS_PSTORE)

Redundant.

	-hpa

Re: [PATCH] efivars: Allow disabling use as a pstore backend

[Seth Forshee]

On Thu, Mar 07, 2013 at 12:38:49PM -0800, H. Peter Anvin wrote:
> On 03/07/2013 09:40 AM, Seth Forshee wrote:
> > We know that with some firmware implementations writing too much data to
> > UEFI variables can lead to bricking machines. Recent changes attempt to
> > address this issue, but for some it may still be prudent to avoid
> > writing large amounts of data until the solution has been proven on a
> > wide variety of hardware.
> > 
> > Crash dumps or other data from pstore can potentially be a large data
> > source. Add a new option, CONFIG_EFI_VARS_PSTORE, which can be set to N
> > to avoid using efivars as a backend to pstore.
> 
> What about a command line option?

That could be done. I'm assuming you want this instead of a config
option, defaulted to on?

How would you feel about a config option for the default value of the
command line option? I intend to default this to off in Ubuntu for a
while as a colleague's machine was recently bricked on two different
occasions after the kernel oopsed. The config option would simplify
things a bit for us, though we could always carry a patch changing the
default value.

> > -#ifdef CONFIG_PSTORE
> > +#if defined(CONFIG_PSTORE) && defined(CONFIG_EFI_VARS_PSTORE)
> 
> Redundant.

Yes, good point.

Thanks,
Seth

Re: [PATCH] efivars: Allow disabling use as a pstore backend

[Matt Fleming]

On Thu, 2013-03-07 at 14:59 -0600, Seth Forshee wrote:
> On Thu, Mar 07, 2013 at 12:38:49PM -0800, H. Peter Anvin wrote:
> > On 03/07/2013 09:40 AM, Seth Forshee wrote:
> > > We know that with some firmware implementations writing too much data to
> > > UEFI variables can lead to bricking machines. Recent changes attempt to
> > > address this issue, but for some it may still be prudent to avoid
> > > writing large amounts of data until the solution has been proven on a
> > > wide variety of hardware.
> > > 
> > > Crash dumps or other data from pstore can potentially be a large data
> > > source. Add a new option, CONFIG_EFI_VARS_PSTORE, which can be set to N
> > > to avoid using efivars as a backend to pstore.
> > 
> > What about a command line option?
> 
> That could be done. I'm assuming you want this instead of a config
> option, defaulted to on?
> 
> How would you feel about a config option for the default value of the
> command line option? I intend to default this to off in Ubuntu for a
> while as a colleague's machine was recently bricked on two different
> occasions after the kernel oopsed. The config option would simplify
> things a bit for us, though we could always carry a patch changing the
> default value.

Not sure why a command line option would be a more natural solution to
this problem? The original patch looked fine to me. All other pstore
backends are guarded by Kconfig options.

Though I could see an argument for a kernel parameter to turn off the
EFI pstore backend code, even if it's compiled into the kernel. But that
would be better as an additional patch.

Re: [PATCH] efivars: Allow disabling use as a pstore backend

[H. Peter Anvin]

On 03/07/2013 02:15 PM, Matt Fleming wrote:
>>>
>>> What about a command line option?
>>
>> That could be done. I'm assuming you want this instead of a config
>> option, defaulted to on?
>>
>> How would you feel about a config option for the default value of the
>> command line option? I intend to default this to off in Ubuntu for a
>> while as a colleague's machine was recently bricked on two different
>> occasions after the kernel oopsed. The config option would simplify
>> things a bit for us, though we could always carry a patch changing the
>> default value.
> 
> Not sure why a command line option would be a more natural solution to
> this problem? The original patch looked fine to me. All other pstore
> backends are guarded by Kconfig options.
> 
> Though I could see an argument for a kernel parameter to turn off the
> EFI pstore backend code, even if it's compiled into the kernel. But that
> would be better as an additional patch.
> 

That way it can be enabled if needed, without the user having to build
their own kernel.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] efivars: Allow disabling use as a pstore backend

[Seth Forshee]

On Thu, Mar 07, 2013 at 02:17:29PM -0800, H. Peter Anvin wrote:
> On 03/07/2013 02:15 PM, Matt Fleming wrote:
> >>>
> >>> What about a command line option?
> >>
> >> That could be done. I'm assuming you want this instead of a config
> >> option, defaulted to on?
> >>
> >> How would you feel about a config option for the default value of the
> >> command line option? I intend to default this to off in Ubuntu for a
> >> while as a colleague's machine was recently bricked on two different
> >> occasions after the kernel oopsed. The config option would simplify
> >> things a bit for us, though we could always carry a patch changing the
> >> default value.
> > 
> > Not sure why a command line option would be a more natural solution to
> > this problem? The original patch looked fine to me. All other pstore
> > backends are guarded by Kconfig options.
> > 
> > Though I could see an argument for a kernel parameter to turn off the
> > EFI pstore backend code, even if it's compiled into the kernel. But that
> > would be better as an additional patch.
> > 
> 
> That way it can be enabled if needed, without the user having to build
> their own kernel.

Here's a patch that does the command line option. I'm happy with either
one.


>From 2febf7014f6fa0abf05326504423749505430b3f Mon Sep 17 00:00:00 2001
From: Seth Forshee <seth.forshee@canonical.com>
Date: Wed, 6 Mar 2013 14:25:36 -0600
Subject: [PATCH] efivars: Add module parameter to disable use as a pstore
 backend

We know that with some firmware implementations writing too much data to
UEFI variables can lead to bricking machines. Recent changes attempt to
address this issue, but for some it may still be prudent to avoid
writing large amounts of data until the solution has been proven on a
wide variety of hardware.

Crash dumps or other data from pstore can potentially be a large data
source. Add a pstore_module parameter to efivars to allow disabling its
use as a backend for pstore. Also add a config option,
CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE, to allow setting the default
value of this paramter to true (i.e. disabled by default).

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/firmware/Kconfig   |    9 ++++++
 drivers/firmware/efivars.c |   70 +++++++++++++++++---------------------------
 2 files changed, 36 insertions(+), 43 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 9b00072..8d556a5 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -53,6 +53,15 @@ config EFI_VARS
 	  Subsequent efibootmgr releases may be found at:
 	  <http://linux.dell.com/efibootmgr>
 
+config EFI_VARS_PSTORE_DEFAULT_DISABLE
+	bool "Disable using efivars as a pstore backend by default"
+	depends on EFI_VARS && PSTORE
+	default n
+	help
+	  Saying Y here will disable the use of efivars as a storage
+	  backend for pstore by default. This setting can be overridden
+	  using the efivars module's pstore_disable parameter.
+
 config EFI_PCDP
 	bool "Console device selection via EFI PCDP or HCDP table"
 	depends on ACPI && EFI && IA64
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index fe62aa3..7de25b7 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -103,6 +103,13 @@ MODULE_VERSION(EFIVARS_VERSION);
  */
 #define GUID_LEN 36
 
+#ifdef CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE
+static bool efivars_pstore_disable = true;
+#else
+static bool efivars_pstore_disable = false;
+#endif
+module_param_named(pstore_disable, efivars_pstore_disable, bool, 0644);
+
 /*
  * The maximum size of VariableName + Data = 1024
  * Therefore, it's reasonable to save that much
@@ -1309,8 +1316,6 @@ static const struct inode_operations efivarfs_dir_inode_operations = {
 	.create = efivarfs_create,
 };
 
-static struct pstore_info efi_pstore_info;
-
 #ifdef CONFIG_PSTORE
 
 static int efi_pstore_open(struct pstore_info *psi)
@@ -1514,38 +1519,6 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
 
 	return 0;
 }
-#else
-static int efi_pstore_open(struct pstore_info *psi)
-{
-	return 0;
-}
-
-static int efi_pstore_close(struct pstore_info *psi)
-{
-	return 0;
-}
-
-static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type, int *count,
-			       struct timespec *timespec,
-			       char **buf, struct pstore_info *psi)
-{
-	return -1;
-}
-
-static int efi_pstore_write(enum pstore_type_id type,
-		enum kmsg_dump_reason reason, u64 *id,
-		unsigned int part, int count, size_t size,
-		struct pstore_info *psi)
-{
-	return 0;
-}
-
-static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
-			    struct timespec time, struct pstore_info *psi)
-{
-	return 0;
-}
-#endif
 
 static struct pstore_info efi_pstore_info = {
 	.owner		= THIS_MODULE,
@@ -1557,6 +1530,24 @@ static struct pstore_info efi_pstore_info = {
 	.erase		= efi_pstore_erase,
 };
 
+static void efivar_pstore_register(struct efivars *efivars)
+{
+	efivars->efi_pstore_info = efi_pstore_info;
+	efivars->efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
+	if (efivars->efi_pstore_info.buf) {
+		efivars->efi_pstore_info.bufsize = 1024;
+		efivars->efi_pstore_info.data = efivars;
+		spin_lock_init(&efivars->efi_pstore_info.buf_lock);
+		pstore_register(&efivars->efi_pstore_info);
+	}
+}
+#else
+static void efivar_pstore_register(struct efivars *efivars)
+{
+	return;
+}
+#endif
+
 static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
 			     struct bin_attribute *bin_attr,
 			     char *buf, loff_t pos, size_t count)
@@ -2025,15 +2016,8 @@ int register_efivars(struct efivars *efivars,
 	if (error)
 		unregister_efivars(efivars);
 
-	efivars->efi_pstore_info = efi_pstore_info;
-
-	efivars->efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
-	if (efivars->efi_pstore_info.buf) {
-		efivars->efi_pstore_info.bufsize = 1024;
-		efivars->efi_pstore_info.data = efivars;
-		spin_lock_init(&efivars->efi_pstore_info.buf_lock);
-		pstore_register(&efivars->efi_pstore_info);
-	}
+	if (!efivars_pstore_disable)
+		efivar_pstore_register(efivars);
 
 	register_filesystem(&efivarfs_type);
 
-- 
1.7.9.5

Re: [PATCH] efivars: Allow disabling use as a pstore backend

[Matt Fleming]

On Mon, 2013-03-11 at 16:17 -0500, Seth Forshee wrote:
> Here's a patch that does the command line option. I'm happy with either
> one.

I like the idea, but isn't the logic backwards? I would have expected
s/EFI_VARS_PSTORE_DEFAULT_DISABLE/EFI_VARS_PSTORE/g and then 'default y'
in the Kconfig file to maintain backward compatibility?

Is there a reason that wouldn't work?

I know that Linus has previously denounced setting new Kconfig symbols
to 'y' by default, but I think there's a case here for doing exactly
that since the previous behaviour was always enabled. The networking
folks did something similar recently, where they introduced new Kconfig
symbols for existing functionality that was previously on by default.

[...]

> +#ifdef CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE
> +static bool efivars_pstore_disable = true;
> +#else
> +static bool efivars_pstore_disable = false;
> +#endif
> +module_param_named(pstore_disable, efivars_pstore_disable, bool, 0644);
> +

static bool efivars_pstore_enable = IS_ENABLED(CONFIG_EFI_VARS_PSTORE) ?

Re: [PATCH] efivars: Allow disabling use as a pstore backend

[Seth Forshee]

On Tue, Mar 12, 2013 at 07:54:33PM +0000, Matt Fleming wrote:
> On Mon, 2013-03-11 at 16:17 -0500, Seth Forshee wrote:
> > Here's a patch that does the command line option. I'm happy with either
> > one.
> 
> I like the idea, but isn't the logic backwards? I would have expected
> s/EFI_VARS_PSTORE_DEFAULT_DISABLE/EFI_VARS_PSTORE/g and then 'default y'
> in the Kconfig file to maintain backward compatibility?
> 
> Is there a reason that wouldn't work?

It should work, just a change of perspective I guess. When the feature
is enabled by default a foo_disable parameter feels more natural to me
than foo_enable, but it's just semantics. I've switched it to your
suggestion in the patch below. I must admit that the name of the config
option is much better this way.

> I know that Linus has previously denounced setting new Kconfig symbols
> to 'y' by default, but I think there's a case here for doing exactly
> that since the previous behaviour was always enabled. The networking
> folks did something similar recently, where they introduced new Kconfig
> symbols for existing functionality that was previously on by default.
> 
> [...]
> 
> > +#ifdef CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE
> > +static bool efivars_pstore_disable = true;
> > +#else
> > +static bool efivars_pstore_disable = false;
> > +#endif
> > +module_param_named(pstore_disable, efivars_pstore_disable, bool, 0644);
> > +
> 
> static bool efivars_pstore_enable = IS_ENABLED(CONFIG_EFI_VARS_PSTORE) ?

Yes, that's much nicer.


>From 91df4dd0d1e20f44ea16b3653cffecd507fdb500 Mon Sep 17 00:00:00 2001
From: Seth Forshee <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Date: Wed, 6 Mar 2013 14:25:36 -0600
Subject: [PATCH] efivars: Add module parameter to allow disabling use as a
 pstore backend

We know that with some firmware implementations writing too much data to
UEFI variables can lead to bricking machines. Recent changes attempt to
address this issue, but for some it may still be prudent to avoid
writing large amounts of data until the solution has been proven on a
wide variety of hardware.

Crash dumps or other data from pstore can potentially be a large data
source. Add a pstore_enable parameter to efivars to allow disabling its
use as a backend for pstore. Also add a config option,
CONFIG_EFI_VARS_PSTORE, which will specify the default value for this
parameter. Default this option to Y to maintain the existing behavior.

Signed-off-by: Seth Forshee <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 drivers/firmware/Kconfig   |    9 ++++++
 drivers/firmware/efivars.c |   66 +++++++++++++++-----------------------------
 2 files changed, 32 insertions(+), 43 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 9b00072..35f7d16 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -53,6 +53,15 @@ config EFI_VARS
 	  Subsequent efibootmgr releases may be found at:
 	  <http://linux.dell.com/efibootmgr>
 
+config EFI_VARS_PSTORE
+	bool "Use efivars as a pstore backend"
+	depends on EFI_VARS
+	default y
+	help
+	  Say Y here to enable the use of efivars as a storage backend
+	  for pstore. This setting can be overridden using the
+	  efivars.pstore_enable parameter.
+
 config EFI_PCDP
 	bool "Console device selection via EFI PCDP or HCDP table"
 	depends on ACPI && EFI && IA64
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index fe62aa3..1e5d326 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -103,6 +103,9 @@ MODULE_VERSION(EFIVARS_VERSION);
  */
 #define GUID_LEN 36
 
+static bool efivars_pstore_enable = IS_ENABLED(CONFIG_EFI_VARS_PSTORE);
+module_param_named(pstore_enable, efivars_pstore_enable, bool, 0644);
+
 /*
  * The maximum size of VariableName + Data = 1024
  * Therefore, it's reasonable to save that much
@@ -1309,8 +1312,6 @@ static const struct inode_operations efivarfs_dir_inode_operations = {
 	.create = efivarfs_create,
 };
 
-static struct pstore_info efi_pstore_info;
-
 #ifdef CONFIG_PSTORE
 
 static int efi_pstore_open(struct pstore_info *psi)
@@ -1514,38 +1515,6 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
 
 	return 0;
 }
-#else
-static int efi_pstore_open(struct pstore_info *psi)
-{
-	return 0;
-}
-
-static int efi_pstore_close(struct pstore_info *psi)
-{
-	return 0;
-}
-
-static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type, int *count,
-			       struct timespec *timespec,
-			       char **buf, struct pstore_info *psi)
-{
-	return -1;
-}
-
-static int efi_pstore_write(enum pstore_type_id type,
-		enum kmsg_dump_reason reason, u64 *id,
-		unsigned int part, int count, size_t size,
-		struct pstore_info *psi)
-{
-	return 0;
-}
-
-static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
-			    struct timespec time, struct pstore_info *psi)
-{
-	return 0;
-}
-#endif
 
 static struct pstore_info efi_pstore_info = {
 	.owner		= THIS_MODULE,
@@ -1557,6 +1526,24 @@ static struct pstore_info efi_pstore_info = {
 	.erase		= efi_pstore_erase,
 };
 
+static void efivar_pstore_register(struct efivars *efivars)
+{
+	efivars->efi_pstore_info = efi_pstore_info;
+	efivars->efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
+	if (efivars->efi_pstore_info.buf) {
+		efivars->efi_pstore_info.bufsize = 1024;
+		efivars->efi_pstore_info.data = efivars;
+		spin_lock_init(&efivars->efi_pstore_info.buf_lock);
+		pstore_register(&efivars->efi_pstore_info);
+	}
+}
+#else
+static void efivar_pstore_register(struct efivars *efivars)
+{
+	return;
+}
+#endif
+
 static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
 			     struct bin_attribute *bin_attr,
 			     char *buf, loff_t pos, size_t count)
@@ -2025,15 +2012,8 @@ int register_efivars(struct efivars *efivars,
 	if (error)
 		unregister_efivars(efivars);
 
-	efivars->efi_pstore_info = efi_pstore_info;
-
-	efivars->efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
-	if (efivars->efi_pstore_info.buf) {
-		efivars->efi_pstore_info.bufsize = 1024;
-		efivars->efi_pstore_info.data = efivars;
-		spin_lock_init(&efivars->efi_pstore_info.buf_lock);
-		pstore_register(&efivars->efi_pstore_info);
-	}
+	if (efivars_pstore_enable)
+		efivar_pstore_register(efivars);
 
 	register_filesystem(&efivarfs_type);
 
-- 
1.7.9.5

Re: [PATCH] efivars: Allow disabling use as a pstore backend

[Matt Fleming]

On Tue, 2013-03-12 at 16:14 -0500, Seth Forshee wrote:
> From 91df4dd0d1e20f44ea16b3653cffecd507fdb500 Mon Sep 17 00:00:00 2001
> From: Seth Forshee <seth.forshee@canonical.com>
> Date: Wed, 6 Mar 2013 14:25:36 -0600
> Subject: [PATCH] efivars: Add module parameter to allow disabling use as a
>  pstore backend
> 
> We know that with some firmware implementations writing too much data to
> UEFI variables can lead to bricking machines. Recent changes attempt to
> address this issue, but for some it may still be prudent to avoid
> writing large amounts of data until the solution has been proven on a
> wide variety of hardware.
> 
> Crash dumps or other data from pstore can potentially be a large data
> source. Add a pstore_enable parameter to efivars to allow disabling its
> use as a backend for pstore. Also add a config option,
> CONFIG_EFI_VARS_PSTORE, which will specify the default value for this
> parameter. Default this option to Y to maintain the existing behavior.
> 
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
>  drivers/firmware/Kconfig   |    9 ++++++
>  drivers/firmware/efivars.c |   66 +++++++++++++++-----------------------------
>  2 files changed, 32 insertions(+), 43 deletions(-)

Thanks for sticking with this Seth. I think this is a nice solution.
I've tossed this patch into my urgent queue and tagged it for stable and
added some Cc's.

I did make one minor adjustment below. Let me know if you disagree.

Peter, are you happy with this version?

> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 9b00072..35f7d16 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -53,6 +53,15 @@ config EFI_VARS
>  	  Subsequent efibootmgr releases may be found at:
>  	  <http://linux.dell.com/efibootmgr>
>  
> +config EFI_VARS_PSTORE
> +	bool "Use efivars as a pstore backend"
> +	depends on EFI_VARS
> +	default y
> +	help
> +	  Say Y here to enable the use of efivars as a storage backend
> +	  for pstore. This setting can be overridden using the
> +	  efivars.pstore_enable parameter.
> +

The depends here should probably include PSTORE, which I think it did on
previous iterations. I can't think of a situation where you'd want to
disable CONFIG_PSTORE but enable CONFIG_EFI_VARS_PSTORE. Also, we don't
need to use CONFIG_EFI_VARS_PSTORE solely for the purpose of setting the
initial value of efivars_pstore_enabled - there's merit in also using it
to leave out the pstore code in efivars.c during compilation.

Granted, this change does mean that your decision at compile time may be
final, and you can't enable the pstore code at runtime if you built your
kernel/efivars module with CONFIG_EFI_VARS_PSTORE=n. Arguably, I think
that's a feature that some people will want, e.g. "Don't allow enabling
the efivars pstore backend at all... no, srsly!".

[...]

> @@ -1309,8 +1312,6 @@ static const struct inode_operations efivarfs_dir_inode_operations = {
>  	.create = efivarfs_create,
>  };
>  
> -static struct pstore_info efi_pstore_info;
> -
>  #ifdef CONFIG_PSTORE

... I've changed this to #ifdef CONFIG_EFI_VARS_PSTORE, since if
CONFIG_EFI_VARS_PSTORE=n there's no point compiling the pstore code.

Re: [PATCH] efivars: Allow disabling use as a pstore backend

[Seth Forshee]

On Wed, Mar 13, 2013 at 03:49:30PM +0000, Matt Fleming wrote:
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index 9b00072..35f7d16 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -53,6 +53,15 @@ config EFI_VARS
> >  	  Subsequent efibootmgr releases may be found at:
> >  	  <http://linux.dell.com/efibootmgr>
> >  
> > +config EFI_VARS_PSTORE
> > +	bool "Use efivars as a pstore backend"
> > +	depends on EFI_VARS
> > +	default y
> > +	help
> > +	  Say Y here to enable the use of efivars as a storage backend
> > +	  for pstore. This setting can be overridden using the
> > +	  efivars.pstore_enable parameter.
> > +
> 
> The depends here should probably include PSTORE, which I think it did on
> previous iterations. I can't think of a situation where you'd want to
> disable CONFIG_PSTORE but enable CONFIG_EFI_VARS_PSTORE. Also, we don't
> need to use CONFIG_EFI_VARS_PSTORE solely for the purpose of setting the
> initial value of efivars_pstore_enabled - there's merit in also using it
> to leave out the pstore code in efivars.c during compilation.
> 
> Granted, this change does mean that your decision at compile time may be
> final, and you can't enable the pstore code at runtime if you built your
> kernel/efivars module with CONFIG_EFI_VARS_PSTORE=n. Arguably, I think
> that's a feature that some people will want, e.g. "Don't allow enabling
> the efivars pstore backend at all... no, srsly!".

Hmm. Since my original patch was to compile pstore support out
completely, I interpreted Peter's suggestion to be allowing the user
override the default even if the default is disabled. That's the
assumption my implementation was based off of, but maybe I
misunderstood.

But if Peter agrees with your changes then I'm fine with them too.

Thanks,
Seth

Re: [PATCH] efivars: Allow disabling use as a pstore backend

[H. Peter Anvin]

On 03/13/2013 10:07 AM, Seth Forshee wrote:
>>
>> Granted, this change does mean that your decision at compile time may be
>> final, and you can't enable the pstore code at runtime if you built your
>> kernel/efivars module with CONFIG_EFI_VARS_PSTORE=n. Arguably, I think
>> that's a feature that some people will want, e.g. "Don't allow enabling
>> the efivars pstore backend at all... no, srsly!".
> 
> Hmm. Since my original patch was to compile pstore support out
> completely, I interpreted Peter's suggestion to be allowing the user
> override the default even if the default is disabled. That's the
> assumption my implementation was based off of, but maybe I
> misunderstood.
> 
> But if Peter agrees with your changes then I'm fine with them too.
> 

I think there should be two compile-time options (one to compile in the
EFI pstore code at all, and one to set its default.)

	-hpa

Re: [PATCH] efivars: Allow disabling use as a pstore backend

[Matt Fleming]

On Wed, 2013-03-13 at 10:25 -0700, H. Peter Anvin wrote:
> I think there should be two compile-time options (one to compile in the
> EFI pstore code at all, and one to set its default.)

OK, sorry Seth, looks like I'm the one that misunderstood. I've picked
up your first two patches and pushed them out to the urgent branch.
Please check that they look OK - they required minor fixups.

Re: [PATCH] efivars: Allow disabling use as a pstore backend

[Seth Forshee]

On Wed, Mar 13, 2013 at 06:33:37PM +0000, Matt Fleming wrote:
> On Wed, 2013-03-13 at 10:25 -0700, H. Peter Anvin wrote:
> > I think there should be two compile-time options (one to compile in the
> > EFI pstore code at all, and one to set its default.)
> 
> OK, sorry Seth, looks like I'm the one that misunderstood. I've picked
> up your first two patches and pushed them out to the urgent branch.
> Please check that they look OK - they required minor fixups.

A minor nit from the second patch:

+static bool efivars_pstore_disable = \
+       IS_ENABLED(EFI_VARS_PSTORE_DEFAULT_DISABLE);

The backslash isn't necessary.

Otherwise everything looks fine afaict.

Thanks,
Seth

Re: [PATCH] efivars: Allow disabling use as a pstore backend

[Matt Fleming]

On Wed, 2013-03-13 at 14:14 -0500, Seth Forshee wrote:
> On Wed, Mar 13, 2013 at 06:33:37PM +0000, Matt Fleming wrote:
> > On Wed, 2013-03-13 at 10:25 -0700, H. Peter Anvin wrote:
> > > I think there should be two compile-time options (one to compile in the
> > > EFI pstore code at all, and one to set its default.)
> > 
> > OK, sorry Seth, looks like I'm the one that misunderstood. I've picked
> > up your first two patches and pushed them out to the urgent branch.
> > Please check that they look OK - they required minor fixups.
> 
> A minor nit from the second patch:
> 
> +static bool efivars_pstore_disable = \
> +       IS_ENABLED(EFI_VARS_PSTORE_DEFAULT_DISABLE);
> 
> The backslash isn't necessary.

Whoops. Fixed.

> Otherwise everything looks fine afaict.

Thanks!