[PATCH] efi: pstore: Allow dynamic initialization based on module parameter

[PATCH] efi: pstore: Allow dynamic initialization based on module parameter

[Guilherme G. Piccoli]

The efi-pstore module parameter "pstore_disable" warrants that users
are able to deactivate such backend. There is also a Kconfig option
for the default value of this parameter. It was originally added due
to some bad UEFI FW implementations that could break with many variables
written.

Some distros (such as Arch Linux) set this in their config file still
nowadays. And once it is set, even being a writable module parameter,
there is effectively no way to make use of efi-pstore anymore.
If "pstore_disable" is set to true, the init function of the module exits
early and is never called again after the initcall processing.

Let's switch this module parameter to have a callback and perform the
pstore backend registration again each time it's set from Y->N (and
vice-versa). With this, the writable nature of the parameter starts to
make sense, given that users now can switch back to using efi-pstore
or not during runtime by writing into it.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 drivers/firmware/efi/efi-pstore.c | 43 +++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index e7b9ec6f8a86..833cbb995dd3 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -14,16 +14,43 @@ static unsigned int record_size = 1024;
 module_param(record_size, uint, 0444);
 MODULE_PARM_DESC(record_size, "size of each pstore UEFI var (in bytes, min/default=1024)");
 
-static bool efivars_pstore_disable =
-	IS_ENABLED(CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE);
-
-module_param_named(pstore_disable, efivars_pstore_disable, bool, 0644);
-
 #define PSTORE_EFI_ATTRIBUTES \
 	(EFI_VARIABLE_NON_VOLATILE | \
 	 EFI_VARIABLE_BOOTSERVICE_ACCESS | \
 	 EFI_VARIABLE_RUNTIME_ACCESS)
 
+static bool pstore_disable = IS_ENABLED(CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE);
+
+static int efivars_pstore_init(void);
+static void efivars_pstore_exit(void);
+
+static int efi_pstore_disable_set(const char *val, const struct kernel_param *kp)
+{
+	int err;
+	bool old_pstore_disable = pstore_disable;
+
+	err = param_set_bool(val, kp);
+	if (err)
+		return err;
+
+	if (old_pstore_disable != pstore_disable) {
+		if (pstore_disable)
+			efivars_pstore_exit();
+		else
+			efivars_pstore_init();
+	}
+
+	return 0;
+}
+
+static const struct kernel_param_ops pstore_disable_ops = {
+	.set	= efi_pstore_disable_set,
+	.get	= param_get_bool,
+};
+
+module_param_cb(pstore_disable, &pstore_disable_ops, &pstore_disable, 0644);
+__MODULE_PARM_TYPE(pstore_disable, "bool");
+
 static int efi_pstore_open(struct pstore_info *psi)
 {
 	int err;
@@ -218,12 +245,12 @@ static struct pstore_info efi_pstore_info = {
 	.erase		= efi_pstore_erase,
 };
 
-static __init int efivars_pstore_init(void)
+static int efivars_pstore_init(void)
 {
 	if (!efivar_supports_writes())
 		return 0;
 
-	if (efivars_pstore_disable)
+	if (pstore_disable)
 		return 0;
 
 	/*
@@ -250,7 +277,7 @@ static __init int efivars_pstore_init(void)
 	return 0;
 }
 
-static __exit void efivars_pstore_exit(void)
+static void efivars_pstore_exit(void)
 {
 	if (!efi_pstore_info.bufsize)
 		return;
-- 
2.43.0

Re: [PATCH] efi: pstore: Allow dynamic initialization based on module parameter

[Guilherme G. Piccoli]

On 03/01/2024 15:40, Guilherme G. Piccoli wrote:
> The efi-pstore module parameter "pstore_disable" warrants that users
> are able to deactivate such backend. There is also a Kconfig option
> for the default value of this parameter. It was originally added due
> to some bad UEFI FW implementations that could break with many variables
> written.
> 
> Some distros (such as Arch Linux) set this in their config file still
> nowadays. And once it is set, even being a writable module parameter,
> there is effectively no way to make use of efi-pstore anymore.
> If "pstore_disable" is set to true, the init function of the module exits
> early and is never called again after the initcall processing.
> 
> Let's switch this module parameter to have a callback and perform the
> pstore backend registration again each time it's set from Y->N (and
> vice-versa). With this, the writable nature of the parameter starts to
> make sense, given that users now can switch back to using efi-pstore
> or not during runtime by writing into it.

Hi folks, a friendly ping - any comments on this one, suggestions?
Thanks in advance,


Guilherme

Re: [PATCH] efi: pstore: Allow dynamic initialization based on module parameter

[Greg KH]

On Mon, Jan 15, 2024 at 06:20:45PM -0300, Guilherme G. Piccoli wrote:
> On 03/01/2024 15:40, Guilherme G. Piccoli wrote:
> > The efi-pstore module parameter "pstore_disable" warrants that users
> > are able to deactivate such backend. There is also a Kconfig option
> > for the default value of this parameter. It was originally added due
> > to some bad UEFI FW implementations that could break with many variables
> > written.
> > 
> > Some distros (such as Arch Linux) set this in their config file still
> > nowadays. And once it is set, even being a writable module parameter,
> > there is effectively no way to make use of efi-pstore anymore.
> > If "pstore_disable" is set to true, the init function of the module exits
> > early and is never called again after the initcall processing.
> > 
> > Let's switch this module parameter to have a callback and perform the
> > pstore backend registration again each time it's set from Y->N (and
> > vice-versa). With this, the writable nature of the parameter starts to
> > make sense, given that users now can switch back to using efi-pstore
> > or not during runtime by writing into it.
> 
> Hi folks, a friendly ping - any comments on this one, suggestions?

It's the middle of the merge window, not much anyone can do until after
that is over to pick up new stuff.

thanks,

greg k-h

Re: [PATCH] efi: pstore: Allow dynamic initialization based on module parameter

[Guilherme G. Piccoli]

On 16/01/2024 05:46, Greg KH wrote:
> On Mon, Jan 15, 2024 at 06:20:45PM -0300, Guilherme G. Piccoli wrote:
>> On 03/01/2024 15:40, Guilherme G. Piccoli wrote:
>>> The efi-pstore module parameter "pstore_disable" warrants that users
>>> are able to deactivate such backend. There is also a Kconfig option
>>> for the default value of this parameter. It was originally added due
>>> to some bad UEFI FW implementations that could break with many variables
>>> written.
>>>
>>> Some distros (such as Arch Linux) set this in their config file still
>>> nowadays. And once it is set, even being a writable module parameter,
>>> there is effectively no way to make use of efi-pstore anymore.
>>> If "pstore_disable" is set to true, the init function of the module exits
>>> early and is never called again after the initcall processing.
>>>
>>> Let's switch this module parameter to have a callback and perform the
>>> pstore backend registration again each time it's set from Y->N (and
>>> vice-versa). With this, the writable nature of the parameter starts to
>>> make sense, given that users now can switch back to using efi-pstore
>>> or not during runtime by writing into it.
>>
>> Hi folks, a friendly ping - any comments on this one, suggestions?
> 
> It's the middle of the merge window, not much anyone can do until after
> that is over to pick up new stuff.
> 
> thanks,
> 
> greg k-h
> 

OK, that's fine! No hurries, and thanks for your response Greg :)

Re: [PATCH] efi: pstore: Allow dynamic initialization based on module parameter

[Kees Cook]

On Wed, 03 Jan 2024 15:40:32 -0300, Guilherme G. Piccoli wrote:
> The efi-pstore module parameter "pstore_disable" warrants that users
> are able to deactivate such backend. There is also a Kconfig option
> for the default value of this parameter. It was originally added due
> to some bad UEFI FW implementations that could break with many variables
> written.
> 
> Some distros (such as Arch Linux) set this in their config file still
> nowadays. And once it is set, even being a writable module parameter,
> there is effectively no way to make use of efi-pstore anymore.
> If "pstore_disable" is set to true, the init function of the module exits
> early and is never called again after the initcall processing.
> 
> [...]

Applied to for-next/pstore, thanks!

[1/1] efi: pstore: Allow dynamic initialization based on module parameter
      https://git.kernel.org/kees/c/c3f849caf81b

Take care,

-- 
Kees Cook