linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] The UEFI panic notification mechanism, 2nd round
@ 2022-07-29 19:45 Guilherme G. Piccoli
  2022-07-29 19:45 ` [PATCH v2 1/3] efi: Add a generic helper to convert strings to unicode Guilherme G. Piccoli
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Guilherme G. Piccoli @ 2022-07-29 19:45 UTC (permalink / raw)
  To: ardb, linux-efi
  Cc: linux-kernel, kernel-dev, kernel, anton, ccross, keescook, matt,
	mjg59, tony.luck, Guilherme G. Piccoli

Hey folks, this is the 2nd iteration of the patchset adding a simple
mechanism to notify the UEFI firmware about a panic event in the kernel.
V1 here:
https://lore.kernel.org/lkml/20220520195028.1347426-1-gpiccoli@igalia.com/


First thing, the differences in this V2:

- Ardb response in V1 mentioned a refactor aimed for v5.20 that removes an
obsolete/confusing way of setting EFI variables - this led to a weird
condition, deleted variables stayed in sysfs after deletion. Well, I've
refactored the code based on efi/next, so I'm using the recommended API
now - thanks a bunch for the advice Ardb!

- I've changed NULL-terminating char in patch 1 to the format I've seen
in Ardb's code, L'\0'.

- Patch 2 is new, it's somewhat a fix for a patch only in efi/next, part
of the efivar refactor.


In the V1 review, it was mentioned we could maybe use efi-pstore as a way
to signal the firmware about a panic event - in the end, the efi-pstore
mechanism can collect a dmesg, so it's even richer in the information level.
But I disagree that it is the way to go, for 3 main reasons:

a) efi-pstore could be impossible to use, if the users are already using
another pstore backend (like ramoops), which is _exactly_ our case!
Of course, we could rework pstore and allow 2 backends, quite a bit of work,
but...see next points!

b) Even if (a) is a not an issue, we have another one, even more important:
signaling the firmware about a panic is *different* than collecting a bunch
of data, a full dmesg even. This could be considered a security issue for
some users; also, the dmesg collected consumes a bunch more memory in the
(potentially scarce) UEFI available memory.
Although related, the goal of pstore is orthogonal to our mechanism here:
users rely on pstore to collect data, our proposal is a simple infrastructure
to just let the firmware know about a panic. Our kernel module also shows a
message and automatically clears the UEFI variable, so it tracks a single
panic, whereas efi-pstore logs are kept by default, in order to provide
data to users.

c) Finally, it's faster and less "invasive"/risky to just write a byte in a
variable on a panic event than having a ksmg dumper collecting the full dmesg
and writing it to the UEFI memory; again, some users wish to have the logs,
but not all of them.


With all of that said, I think this module makes sense, it's a very simple
solution that opens doors to firmware panic handling approaches, like in our
proposed case (a different splash screen on panic).

Finally, the variable name (PanicWarn) and value (0xFF by default, can be
changed by a module parameter) are just my personal choices but I'm open to
suggestions, not strongly attached to them heh

Thanks again for the reviews/suggestions!
Cheers,


Guilherme


Guilherme G. Piccoli (3):
  efi: Add a generic helper to convert strings to unicode
  efi: efibc: Guard against allocation failure
  efi-panic: Introduce the UEFI panic notification mechanism

 drivers/firmware/efi/Kconfig      | 10 ++++
 drivers/firmware/efi/Makefile     |  1 +
 drivers/firmware/efi/efi-panic.c  | 89 +++++++++++++++++++++++++++++++
 drivers/firmware/efi/efi-pstore.c |  4 +-
 drivers/firmware/efi/efibc.c      | 11 ++--
 include/linux/efi.h               | 18 +++++++
 6 files changed, 125 insertions(+), 8 deletions(-)
 create mode 100644 drivers/firmware/efi/efi-panic.c

-- 
2.37.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 1/3] efi: Add a generic helper to convert strings to unicode
  2022-07-29 19:45 [PATCH v2 0/3] The UEFI panic notification mechanism, 2nd round Guilherme G. Piccoli
@ 2022-07-29 19:45 ` Guilherme G. Piccoli
  2022-07-29 19:45 ` [PATCH v2 2/3] efi: efibc: Guard against allocation failure Guilherme G. Piccoli
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: Guilherme G. Piccoli @ 2022-07-29 19:45 UTC (permalink / raw)
  To: ardb, linux-efi
  Cc: linux-kernel, kernel-dev, kernel, anton, ccross, keescook, matt,
	mjg59, tony.luck, Guilherme G. Piccoli

Currently both efi-pstore and efibc rely in simple for-loops to convert
from regular char pointers to u16/unicode strings; so let's export this
functionality as a helper to common EFI code to prevent code duplication.
This helper will also be used in a subsequent patch (adding a new module).

Notice that efi-pstore didn't write the end NULL char in the unicode
string before this patch, but this should not change anything in a
relevant way for this module - other than that, no functional change is
expected.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 drivers/firmware/efi/efi-pstore.c |  4 +---
 drivers/firmware/efi/efibc.c      |  9 ++++-----
 include/linux/efi.h               | 17 +++++++++++++++++
 3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index 3bddc152fcd4..965a4c0d6e60 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -167,7 +167,6 @@ static int efi_pstore_write(struct pstore_record *record)
 	char name[DUMP_NAME_LEN];
 	efi_char16_t efi_name[DUMP_NAME_LEN];
 	efi_status_t status;
-	int i;
 
 	record->id = generic_id(record->time.tv_sec, record->part,
 				record->count);
@@ -180,8 +179,7 @@ static int efi_pstore_write(struct pstore_record *record)
 		 (long long)record->time.tv_sec,
 		 record->compressed ? 'C' : 'D');
 
-	for (i = 0; i < DUMP_NAME_LEN; i++)
-		efi_name[i] = name[i];
+	efi_str8_to_str16(efi_name, name, DUMP_NAME_LEN - 1);
 
 	if (efivar_trylock())
 		return -EBUSY;
diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
index 8ced7af8e56d..7e3bf60d24e0 100644
--- a/drivers/firmware/efi/efibc.c
+++ b/drivers/firmware/efi/efibc.c
@@ -39,7 +39,7 @@ static int efibc_reboot_notifier_call(struct notifier_block *notifier,
 						    : L"shutdown";
 	const u8 *str = data;
 	efi_char16_t *wdata;
-	unsigned long l;
+	size_t len;
 	int ret;
 
 	ret = efibc_set_variable(L"LoaderEntryRebootReason", reason,
@@ -48,11 +48,10 @@ static int efibc_reboot_notifier_call(struct notifier_block *notifier,
 		return NOTIFY_DONE;
 
 	wdata = kmalloc(MAX_DATA_LEN * sizeof(efi_char16_t), GFP_KERNEL);
-	for (l = 0; l < MAX_DATA_LEN - 1 && str[l] != '\0'; l++)
-		wdata[l] = str[l];
-	wdata[l] = L'\0';
 
-	efibc_set_variable(L"LoaderEntryOneShot", wdata, l);
+	len = efi_str8_to_str16(wdata, str, MAX_DATA_LEN - 1);
+
+	efibc_set_variable(L"LoaderEntryOneShot", wdata, len);
 
 	kfree(wdata);
 	return NOTIFY_DONE;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index d2b84c2fec39..c0ea01be3772 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1030,6 +1030,23 @@ struct efivars {
 
 #define EFI_VAR_NAME_LEN	1024
 
+/*
+ * Helper that converts regular char buffer to u16 unicode-like strings;
+ * notice that the unicode buffer requires to be at least len+1 in size.
+ */
+static inline size_t
+efi_str8_to_str16(efi_char16_t *str16, const char *str8, size_t len)
+{
+	size_t i;
+
+	for (i = 0; i < len && str8[i] != '\0'; i++)
+		str16[i] = str8[i];
+
+	str16[i] = L'\0';
+
+	return i;
+}
+
 int efivars_register(struct efivars *efivars,
 		     const struct efivar_operations *ops,
 		     struct kobject *kobject);
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/3] efi: efibc: Guard against allocation failure
  2022-07-29 19:45 [PATCH v2 0/3] The UEFI panic notification mechanism, 2nd round Guilherme G. Piccoli
  2022-07-29 19:45 ` [PATCH v2 1/3] efi: Add a generic helper to convert strings to unicode Guilherme G. Piccoli
@ 2022-07-29 19:45 ` Guilherme G. Piccoli
  2022-07-29 19:45 ` [PATCH v2 3/3] efi-panic: Introduce the UEFI panic notification mechanism Guilherme G. Piccoli
  2022-08-29 13:04 ` [PATCH v2 0/3] The UEFI panic notification mechanism, 2nd round Guilherme G. Piccoli
  3 siblings, 0 replies; 7+ messages in thread
From: Guilherme G. Piccoli @ 2022-07-29 19:45 UTC (permalink / raw)
  To: ardb, linux-efi
  Cc: linux-kernel, kernel-dev, kernel, anton, ccross, keescook, matt,
	mjg59, tony.luck, Guilherme G. Piccoli

There is a single kmalloc in this driver, and it's not currently
guarded against allocation failure. Do it here by just letting the
reboot handler to proceed, in case this tentative allocation fails.

Fixes: 416581e48679 ("efi: efibc: avoid efivar API for setting variables")
Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---

Notice the sha-1 hash of the efibc patch we're fixing is from efi/next,
might change in upstream once such patch is merged.
Feel free to remove the fixes tag if it makes sense, no issues from me =)

 drivers/firmware/efi/efibc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
index 7e3bf60d24e0..9a8d914f91a6 100644
--- a/drivers/firmware/efi/efibc.c
+++ b/drivers/firmware/efi/efibc.c
@@ -48,6 +48,8 @@ static int efibc_reboot_notifier_call(struct notifier_block *notifier,
 		return NOTIFY_DONE;
 
 	wdata = kmalloc(MAX_DATA_LEN * sizeof(efi_char16_t), GFP_KERNEL);
+	if (!wdata)
+		return NOTIFY_DONE;
 
 	len = efi_str8_to_str16(wdata, str, MAX_DATA_LEN - 1);
 
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 3/3] efi-panic: Introduce the UEFI panic notification mechanism
  2022-07-29 19:45 [PATCH v2 0/3] The UEFI panic notification mechanism, 2nd round Guilherme G. Piccoli
  2022-07-29 19:45 ` [PATCH v2 1/3] efi: Add a generic helper to convert strings to unicode Guilherme G. Piccoli
  2022-07-29 19:45 ` [PATCH v2 2/3] efi: efibc: Guard against allocation failure Guilherme G. Piccoli
@ 2022-07-29 19:45 ` Guilherme G. Piccoli
  2022-08-29 13:04 ` [PATCH v2 0/3] The UEFI panic notification mechanism, 2nd round Guilherme G. Piccoli
  3 siblings, 0 replies; 7+ messages in thread
From: Guilherme G. Piccoli @ 2022-07-29 19:45 UTC (permalink / raw)
  To: ardb, linux-efi
  Cc: linux-kernel, kernel-dev, kernel, anton, ccross, keescook, matt,
	mjg59, tony.luck, Guilherme G. Piccoli

Despite we have pstore, kdump and other panic-time mechanisms,
there's no generic way to let the firmware know a panic event
happened. Some hypervisors might have special code for that, but
that's not generic.

The UEFI panic notification module hereby proposed aims to fill
this need: once we face a panic, through the panic notifier
infrastructure we set a UEFI variable named PanicWarn with a
value - default is 0xFF, but it's adjustable via module parameter.
The firmware is then enabled to take a proper action.

The module also let the users know that last execution likely
panicked if the UEFI variable is present on module load - then
it clears that to avoid confusing users, only the last panic
event is recorded.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 drivers/firmware/efi/Kconfig     | 10 ++++
 drivers/firmware/efi/Makefile    |  1 +
 drivers/firmware/efi/efi-panic.c | 89 ++++++++++++++++++++++++++++++++
 include/linux/efi.h              |  1 +
 4 files changed, 101 insertions(+)
 create mode 100644 drivers/firmware/efi/efi-panic.c

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 6cb7384ad2ac..f1ed6128d623 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -147,6 +147,16 @@ config EFI_BOOTLOADER_CONTROL
 	  bootloader reads this reboot reason and takes particular
 	  action according to its policy.
 
+config EFI_PANIC_NOTIFIER
+	tristate "UEFI Panic Notifier"
+	depends on EFI
+	default n
+	help
+	  With this module, kernel creates the UEFI variable "PanicWarn" if
+	  the system faces a panic event. With that, the firmware is entitled
+	  to take an action if the last kernel panicked; it also shows a
+	  message during boot time if last run faced a panic event.
+
 config EFI_CAPSULE_LOADER
 	tristate "EFI capsule loader"
 	depends on EFI && !IA64
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 8d151e332584..c7cf3dcb284e 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)	+= runtime-wrappers.o
 subdir-$(CONFIG_EFI_STUB)		+= libstub
 obj-$(CONFIG_EFI_FAKE_MEMMAP)		+= fake_map.o
 obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
+obj-$(CONFIG_EFI_PANIC_NOTIFIER)	+= efi-panic.o
 obj-$(CONFIG_EFI_TEST)			+= test/
 obj-$(CONFIG_EFI_DEV_PATH_PARSER)	+= dev-path-parser.o
 obj-$(CONFIG_APPLE_PROPERTIES)		+= apple-properties.o
diff --git a/drivers/firmware/efi/efi-panic.c b/drivers/firmware/efi/efi-panic.c
new file mode 100644
index 000000000000..02ffa4930039
--- /dev/null
+++ b/drivers/firmware/efi/efi-panic.c
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * efi-panic: UEFI panic notification mechanism
+ */
+
+#define pr_fmt(fmt) "efi-panic: " fmt
+
+#include <linux/efi.h>
+#include <linux/module.h>
+#include <linux/panic_notifier.h>
+
+#define EFI_PANIC_ATTR		(EFI_VARIABLE_NON_VOLATILE |\
+				 EFI_VARIABLE_BOOTSERVICE_ACCESS |\
+				 EFI_VARIABLE_RUNTIME_ACCESS)
+
+#define EFI_DATA_SIZE		(2 * sizeof(efi_char16_t))
+
+static efi_char16_t *panic_warn_name = L"PanicWarn";
+
+static u8 panic_warn_val = 0xFF;
+module_param(panic_warn_val, byte, 0644);
+MODULE_PARM_DESC(panic_warn_val,
+		 "Value written to PanicWarn efivar on panic event [default=0xFF]");
+
+static int efi_panic_cb(struct notifier_block *nb, unsigned long ev,
+			void *unused)
+{
+	efi_char16_t val[2];
+	efi_status_t ret;
+
+	efi_str8_to_str16(val, &panic_warn_val, 1);
+	ret = efi.set_variable_nonblocking(panic_warn_name,
+					      &LINUX_EFI_PANIC_WARN_GUID,
+					      EFI_PANIC_ATTR, EFI_DATA_SIZE,
+					      val);
+
+	if (ret != EFI_SUCCESS)
+		pr_err("failed notifying UEFI about the panic (0x%lx)\n", ret);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block efi_panic_notifier = {
+	.notifier_call = efi_panic_cb,
+};
+
+static int __init efi_panic_init(void)
+{
+	unsigned long sz = EFI_DATA_SIZE;
+	u8 data[EFI_DATA_SIZE];
+	efi_status_t ret;
+
+	if (!efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE))
+		return -ENODEV;
+
+	ret = efi.get_variable(panic_warn_name, &LINUX_EFI_PANIC_WARN_GUID,
+			       NULL, &sz, data);
+
+	if (ret == EFI_SUCCESS) {
+		pr_info("previous kernel (likely) had a panic\n");
+
+		ret = efi.set_variable(panic_warn_name,
+				       &LINUX_EFI_PANIC_WARN_GUID,
+				       EFI_PANIC_ATTR, 0, NULL);
+		if (ret != EFI_SUCCESS)
+			pr_warn("cannot delete the UEFI variable\n");
+
+	} else {
+		if (ret != EFI_NOT_FOUND)
+			pr_err("can't read the UEFI variable (err=%lx)\n", ret);
+	}
+
+	atomic_notifier_chain_register(&panic_notifier_list,
+				       &efi_panic_notifier);
+
+	return 0;
+}
+module_init(efi_panic_init);
+
+static void __exit efi_panic_exit(void)
+{
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+					 &efi_panic_notifier);
+}
+module_exit(efi_panic_exit);
+
+MODULE_AUTHOR("Guilherme G. Piccoli <gpiccoli@igalia.com>");
+MODULE_DESCRIPTION("UEFI Panic Notification Mechanism");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/efi.h b/include/linux/efi.h
index c0ea01be3772..38a5056cc72a 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -367,6 +367,7 @@ void efi_native_runtime_setup(void);
 #define EFI_GLOBAL_VARIABLE_GUID		EFI_GUID(0x8be4df61, 0x93ca, 0x11d2,  0xaa, 0x0d, 0x00, 0xe0, 0x98, 0x03, 0x2b, 0x8c)
 #define UV_SYSTEM_TABLE_GUID			EFI_GUID(0x3b13a7d4, 0x633e, 0x11dd,  0x93, 0xec, 0xda, 0x25, 0x56, 0xd8, 0x95, 0x93)
 #define LINUX_EFI_CRASH_GUID			EFI_GUID(0xcfc8fc79, 0xbe2e, 0x4ddc,  0x97, 0xf0, 0x9f, 0x98, 0xbf, 0xe2, 0x98, 0xa0)
+#define LINUX_EFI_PANIC_WARN_GUID		EFI_GUID(0x9e85b665, 0x08d4, 0x42c9,  0x83, 0x24, 0x47, 0xed, 0x5f, 0xe5, 0x0b, 0xf3)
 #define LOADED_IMAGE_PROTOCOL_GUID		EFI_GUID(0x5b1b31a1, 0x9562, 0x11d2,  0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
 #define EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID	EFI_GUID(0x9042a9de, 0x23dc, 0x4a38,  0x96, 0xfb, 0x7a, 0xde, 0xd0, 0x80, 0x51, 0x6a)
 #define EFI_UGA_PROTOCOL_GUID			EFI_GUID(0x982c298b, 0xf4fa, 0x41cb,  0xb8, 0x38, 0x77, 0xaa, 0x68, 0x8f, 0xb8, 0x39)
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 0/3] The UEFI panic notification mechanism, 2nd round
  2022-07-29 19:45 [PATCH v2 0/3] The UEFI panic notification mechanism, 2nd round Guilherme G. Piccoli
                   ` (2 preceding siblings ...)
  2022-07-29 19:45 ` [PATCH v2 3/3] efi-panic: Introduce the UEFI panic notification mechanism Guilherme G. Piccoli
@ 2022-08-29 13:04 ` Guilherme G. Piccoli
  2022-09-05  9:50   ` Ard Biesheuvel
  3 siblings, 1 reply; 7+ messages in thread
From: Guilherme G. Piccoli @ 2022-08-29 13:04 UTC (permalink / raw)
  To: ardb, linux-efi
  Cc: linux-kernel, kernel-dev, kernel, anton, ccross, keescook, matt,
	mjg59, tony.luck

On 29/07/2022 16:45, Guilherme G. Piccoli wrote:
> Hey folks, this is the 2nd iteration of the patchset adding a simple
> mechanism to notify the UEFI firmware about a panic event in the kernel.
> V1 here:
> https://lore.kernel.org/lkml/20220520195028.1347426-1-gpiccoli@igalia.com/
> 
> 
> First thing, the differences in this V2:
> 
> - Ardb response in V1 mentioned a refactor aimed for v5.20 that removes an
> obsolete/confusing way of setting EFI variables - this led to a weird
> condition, deleted variables stayed in sysfs after deletion. Well, I've
> refactored the code based on efi/next, so I'm using the recommended API
> now - thanks a bunch for the advice Ardb!
> 
> - I've changed NULL-terminating char in patch 1 to the format I've seen
> in Ardb's code, L'\0'.
> 
> - Patch 2 is new, it's somewhat a fix for a patch only in efi/next, part
> of the efivar refactor.
> 
> 
> In the V1 review, it was mentioned we could maybe use efi-pstore as a way
> to signal the firmware about a panic event - in the end, the efi-pstore
> mechanism can collect a dmesg, so it's even richer in the information level.
> But I disagree that it is the way to go, for 3 main reasons:
> 
> a) efi-pstore could be impossible to use, if the users are already using
> another pstore backend (like ramoops), which is _exactly_ our case!
> Of course, we could rework pstore and allow 2 backends, quite a bit of work,
> but...see next points!
> 
> b) Even if (a) is a not an issue, we have another one, even more important:
> signaling the firmware about a panic is *different* than collecting a bunch
> of data, a full dmesg even. This could be considered a security issue for
> some users; also, the dmesg collected consumes a bunch more memory in the
> (potentially scarce) UEFI available memory.
> Although related, the goal of pstore is orthogonal to our mechanism here:
> users rely on pstore to collect data, our proposal is a simple infrastructure
> to just let the firmware know about a panic. Our kernel module also shows a
> message and automatically clears the UEFI variable, so it tracks a single
> panic, whereas efi-pstore logs are kept by default, in order to provide
> data to users.
> 
> c) Finally, it's faster and less "invasive"/risky to just write a byte in a
> variable on a panic event than having a ksmg dumper collecting the full dmesg
> and writing it to the UEFI memory; again, some users wish to have the logs,
> but not all of them.
> 
> 
> With all of that said, I think this module makes sense, it's a very simple
> solution that opens doors to firmware panic handling approaches, like in our
> proposed case (a different splash screen on panic).
> 
> Finally, the variable name (PanicWarn) and value (0xFF by default, can be
> changed by a module parameter) are just my personal choices but I'm open to
> suggestions, not strongly attached to them heh
> 
> Thanks again for the reviews/suggestions!
> Cheers,
> 
> 
> Guilherme
> 
> 

Hi Ard, sorry for the ping =]

Any opinions in this one? Patch 2 is a simple fix, BTW.
Cheers,


Guilherme

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 0/3] The UEFI panic notification mechanism, 2nd round
  2022-08-29 13:04 ` [PATCH v2 0/3] The UEFI panic notification mechanism, 2nd round Guilherme G. Piccoli
@ 2022-09-05  9:50   ` Ard Biesheuvel
  2022-09-09 19:54     ` Guilherme G. Piccoli
  0 siblings, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2022-09-05  9:50 UTC (permalink / raw)
  To: Guilherme G. Piccoli
  Cc: linux-efi, linux-kernel, kernel-dev, kernel, anton, ccross,
	keescook, matt, mjg59, tony.luck

On Mon, 29 Aug 2022 at 15:04, Guilherme G. Piccoli <gpiccoli@igalia.com> wrote:
>
> On 29/07/2022 16:45, Guilherme G. Piccoli wrote:
> > Hey folks, this is the 2nd iteration of the patchset adding a simple
> > mechanism to notify the UEFI firmware about a panic event in the kernel.
> > V1 here:
> > https://lore.kernel.org/lkml/20220520195028.1347426-1-gpiccoli@igalia.com/
> >
> >
> > First thing, the differences in this V2:
> >
> > - Ardb response in V1 mentioned a refactor aimed for v5.20 that removes an
> > obsolete/confusing way of setting EFI variables - this led to a weird
> > condition, deleted variables stayed in sysfs after deletion. Well, I've
> > refactored the code based on efi/next, so I'm using the recommended API
> > now - thanks a bunch for the advice Ardb!
> >
> > - I've changed NULL-terminating char in patch 1 to the format I've seen
> > in Ardb's code, L'\0'.
> >
> > - Patch 2 is new, it's somewhat a fix for a patch only in efi/next, part
> > of the efivar refactor.
> >
> >
> > In the V1 review, it was mentioned we could maybe use efi-pstore as a way
> > to signal the firmware about a panic event - in the end, the efi-pstore
> > mechanism can collect a dmesg, so it's even richer in the information level.
> > But I disagree that it is the way to go, for 3 main reasons:
> >
> > a) efi-pstore could be impossible to use, if the users are already using
> > another pstore backend (like ramoops), which is _exactly_ our case!
> > Of course, we could rework pstore and allow 2 backends, quite a bit of work,
> > but...see next points!
> >
> > b) Even if (a) is a not an issue, we have another one, even more important:
> > signaling the firmware about a panic is *different* than collecting a bunch
> > of data, a full dmesg even. This could be considered a security issue for
> > some users; also, the dmesg collected consumes a bunch more memory in the
> > (potentially scarce) UEFI available memory.
> > Although related, the goal of pstore is orthogonal to our mechanism here:
> > users rely on pstore to collect data, our proposal is a simple infrastructure
> > to just let the firmware know about a panic. Our kernel module also shows a
> > message and automatically clears the UEFI variable, so it tracks a single
> > panic, whereas efi-pstore logs are kept by default, in order to provide
> > data to users.
> >
> > c) Finally, it's faster and less "invasive"/risky to just write a byte in a
> > variable on a panic event than having a ksmg dumper collecting the full dmesg
> > and writing it to the UEFI memory; again, some users wish to have the logs,
> > but not all of them.
> >
> >
> > With all of that said, I think this module makes sense, it's a very simple
> > solution that opens doors to firmware panic handling approaches, like in our
> > proposed case (a different splash screen on panic).
> >
> > Finally, the variable name (PanicWarn) and value (0xFF by default, can be
> > changed by a module parameter) are just my personal choices but I'm open to
> > suggestions, not strongly attached to them heh
> >
> > Thanks again for the reviews/suggestions!
> > Cheers,
> >
> >
> > Guilherme
> >
> >
>
> Hi Ard, sorry for the ping =]
>
> Any opinions in this one? Patch 2 is a simple fix, BTW.

Hey,

No worries about the ping - apologies for the late response, I was on vacation.

I still don't see the point of this series, but I will take the fix if
you could please rebase it so it doesn't depend on the first patch.

Thanks,
Ard.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 0/3] The UEFI panic notification mechanism, 2nd round
  2022-09-05  9:50   ` Ard Biesheuvel
@ 2022-09-09 19:54     ` Guilherme G. Piccoli
  0 siblings, 0 replies; 7+ messages in thread
From: Guilherme G. Piccoli @ 2022-09-09 19:54 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: linux-efi, linux-kernel, kernel-dev, kernel, anton, ccross,
	keescook, matt, mjg59, tony.luck

On 05/09/2022 06:50, Ard Biesheuvel wrote:
> [...]
>> Hi Ard, sorry for the ping =]
>>
>> Any opinions in this one? Patch 2 is a simple fix, BTW.
> 
> Hey,
> 
> No worries about the ping - apologies for the late response, I was on vacation.

Hi Ard, no need for apologies at all - this is a known vacation period
in the year =)


> 
> I still don't see the point of this series, but I will take the fix if
> you could please rebase it so it doesn't depend on the first patch.
> 

That part is sad heheh
I mean, it's a pity I could convince you - I still don't see how can
kernel let UEFI know about a panic without this patch (or pstore, which
I still think is the wrong way and sometimes impossible to use, due to
other pstore backend usage).

Anyway, nothing I can do about that it seems, unfortunately heheh

Submitted the V2 fix here:
https://lore.kernel.org/linux-efi/20220909194214.186731-1-gpiccoli@igalia.com/

Thanks,


Guilherme


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-09-09 19:55 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-29 19:45 [PATCH v2 0/3] The UEFI panic notification mechanism, 2nd round Guilherme G. Piccoli
2022-07-29 19:45 ` [PATCH v2 1/3] efi: Add a generic helper to convert strings to unicode Guilherme G. Piccoli
2022-07-29 19:45 ` [PATCH v2 2/3] efi: efibc: Guard against allocation failure Guilherme G. Piccoli
2022-07-29 19:45 ` [PATCH v2 3/3] efi-panic: Introduce the UEFI panic notification mechanism Guilherme G. Piccoli
2022-08-29 13:04 ` [PATCH v2 0/3] The UEFI panic notification mechanism, 2nd round Guilherme G. Piccoli
2022-09-05  9:50   ` Ard Biesheuvel
2022-09-09 19:54     ` Guilherme G. Piccoli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).