[PATCH 0/5] Move internal definitions out of kernel-wide include

[PATCH 0/5] Move internal definitions out of kernel-wide include

[Kees Cook]

Hi,

Here's a set of changes for a few things, inspried by recent discussions
with Guilherme, Ard, and Paramjit.

Thanks!

-Kees

Kees Cook (5):
  pstore/ram: Consolidate kfree() paths
  pstore/ram: Move pmsg init earlier
  pstore/ram: Move internal definitions out of kernel-wide include
  pstore/ram: Set freed addresses to NULL
  MAINTAINERS: Update pstore maintainers

 MAINTAINERS                |  8 +--
 fs/pstore/ram.c            | 42 ++++++++--------
 fs/pstore/ram_core.c       | 14 ++++--
 fs/pstore/ram_internal.h   | 98 +++++++++++++++++++++++++++++++++++++
 include/linux/pstore_ram.h | 99 --------------------------------------
 5 files changed, 136 insertions(+), 125 deletions(-)
 create mode 100644 fs/pstore/ram_internal.h

-- 
2.34.1

[PATCH 1/5] pstore/ram: Consolidate kfree() paths

[Kees Cook]

There's no reason to keep separate kfree() paths: either all allocations
succeeded, or not. Everything is torn down in the case of failure, so
adjust the callers to reflect this.

Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Tony Luck <tony.luck@intel.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/ram.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index fefe3d391d3a..348820a3ce88 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -451,6 +451,12 @@ static void ramoops_free_przs(struct ramoops_context *cxt)
 {
 	int i;
 
+	/* Free pmsg PRZ */
+	persistent_ram_free(cxt->mprz);
+
+	/* Free console PRZ */
+	persistent_ram_free(cxt->cprz);
+
 	/* Free dump PRZs */
 	if (cxt->dprzs) {
 		for (i = 0; i < cxt->max_dump_cnt; i++)
@@ -772,12 +778,12 @@ static int ramoops_probe(struct platform_device *pdev)
 				dump_mem_sz, cxt->record_size,
 				&cxt->max_dump_cnt, 0, 0);
 	if (err)
-		goto fail_out;
+		goto fail_init;
 
 	err = ramoops_init_prz("console", dev, cxt, &cxt->cprz, &paddr,
 			       cxt->console_size, 0);
 	if (err)
-		goto fail_init_cprz;
+		goto fail_init;
 
 	cxt->max_ftrace_cnt = (cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)
 				? nr_cpu_ids
@@ -788,12 +794,12 @@ static int ramoops_probe(struct platform_device *pdev)
 				(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)
 					? PRZ_FLAG_NO_LOCK : 0);
 	if (err)
-		goto fail_init_fprz;
+		goto fail_init;
 
 	err = ramoops_init_prz("pmsg", dev, cxt, &cxt->mprz, &paddr,
 				cxt->pmsg_size, 0);
 	if (err)
-		goto fail_init_mprz;
+		goto fail_init;
 
 	cxt->pstore.data = cxt;
 	/*
@@ -857,11 +863,7 @@ static int ramoops_probe(struct platform_device *pdev)
 	kfree(cxt->pstore.buf);
 fail_clear:
 	cxt->pstore.bufsize = 0;
-	persistent_ram_free(cxt->mprz);
-fail_init_mprz:
-fail_init_fprz:
-	persistent_ram_free(cxt->cprz);
-fail_init_cprz:
+fail_init:
 	ramoops_free_przs(cxt);
 fail_out:
 	return err;
@@ -876,8 +878,6 @@ static int ramoops_remove(struct platform_device *pdev)
 	kfree(cxt->pstore.buf);
 	cxt->pstore.bufsize = 0;
 
-	persistent_ram_free(cxt->mprz);
-	persistent_ram_free(cxt->cprz);
 	ramoops_free_przs(cxt);
 
 	return 0;
-- 
2.34.1

Re: [PATCH 1/5] pstore/ram: Consolidate kfree() paths

[Guilherme G. Piccoli]

On 11/10/2022 17:01, Kees Cook wrote:
> There's no reason to keep separate kfree() paths: either all allocations
> succeeded, or not. Everything is torn down in the case of failure, so
> adjust the callers to reflect this.
> 
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  fs/pstore/ram.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 

Reviewed-and-tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Tested by building / booting and triggering crashes with efi and ramoops
backends.
Cheers,


Guilherme

[PATCH 2/5] pstore/ram: Move pmsg init earlier

[Kees Cook]

Since the ftrace area can vary in size based on CPU count, move pmsg
initialization earlier so it will have a stable location.

Suggested-by: Paramjit Oberoi <pso@chromium.org>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Tony Luck <tony.luck@intel.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/ram.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 348820a3ce88..2f18563c8141 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -785,6 +785,11 @@ static int ramoops_probe(struct platform_device *pdev)
 	if (err)
 		goto fail_init;
 
+	err = ramoops_init_prz("pmsg", dev, cxt, &cxt->mprz, &paddr,
+				cxt->pmsg_size, 0);
+	if (err)
+		goto fail_init;
+
 	cxt->max_ftrace_cnt = (cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)
 				? nr_cpu_ids
 				: 1;
@@ -796,11 +801,6 @@ static int ramoops_probe(struct platform_device *pdev)
 	if (err)
 		goto fail_init;
 
-	err = ramoops_init_prz("pmsg", dev, cxt, &cxt->mprz, &paddr,
-				cxt->pmsg_size, 0);
-	if (err)
-		goto fail_init;
-
 	cxt->pstore.data = cxt;
 	/*
 	 * Prepare frontend flags based on which areas are initialized.
-- 
2.34.1

Re: [PATCH 2/5] pstore/ram: Move pmsg init earlier

[Guilherme G. Piccoli]

On 11/10/2022 17:01, Kees Cook wrote:
> Since the ftrace area can vary in size based on CPU count, move pmsg
> initialization earlier so it will have a stable location.
> 
> Suggested-by: Paramjit Oberoi <pso@chromium.org>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

Reviewed: Guilherme G. Piccoli <gpiccoli@igalia.com>

Since I'm not using pmsg my tests are of limited scope here, so better
to consider I just reviewed the patch heh
Cheers,


Guilherme

[PATCH 3/5] pstore/ram: Move internal definitions out of kernel-wide include

[Kees Cook]

Most of the details of the ram backend are entirely internal to the
backend itself. Leave only what is needed to instantiate a ram backend
in the kernel-wide header.

Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Tony Luck <tony.luck@intel.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/ram.c            |  3 +-
 fs/pstore/ram_core.c       |  3 +-
 fs/pstore/ram_internal.h   | 98 +++++++++++++++++++++++++++++++++++++
 include/linux/pstore_ram.h | 99 --------------------------------------
 4 files changed, 102 insertions(+), 101 deletions(-)
 create mode 100644 fs/pstore/ram_internal.h

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 2f18563c8141..f5bf360cf905 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -18,10 +18,11 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/compiler.h>
-#include <linux/pstore_ram.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+
 #include "internal.h"
+#include "ram_internal.h"
 
 #define RAMOOPS_KERNMSG_HDR "===="
 #define MIN_MEM_SIZE 4096UL
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index a89e33719fcf..9e1047f4316d 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -13,13 +13,14 @@
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/memblock.h>
-#include <linux/pstore_ram.h>
 #include <linux/rslib.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/vmalloc.h>
 #include <asm/page.h>
 
+#include "ram_internal.h"
+
 /**
  * struct persistent_ram_buffer - persistent circular RAM buffer
  *
diff --git a/fs/pstore/ram_internal.h b/fs/pstore/ram_internal.h
new file mode 100644
index 000000000000..440ee7a35e10
--- /dev/null
+++ b/fs/pstore/ram_internal.h
@@ -0,0 +1,98 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2010 Marco Stornelli <marco.stornelli@gmail.com>
+ * Copyright (C) 2011 Kees Cook <keescook@chromium.org>
+ * Copyright (C) 2011 Google, Inc.
+ */
+
+#include <linux/pstore_ram.h>
+
+/*
+ * Choose whether access to the RAM zone requires locking or not.  If a zone
+ * can be written to from different CPUs like with ftrace for example, then
+ * PRZ_FLAG_NO_LOCK is used. For all other cases, locking is required.
+ */
+#define PRZ_FLAG_NO_LOCK	BIT(0)
+/*
+ * If a PRZ should only have a single-boot lifetime, this marks it as
+ * getting wiped after its contents get copied out after boot.
+ */
+#define PRZ_FLAG_ZAP_OLD	BIT(1)
+
+/**
+ * struct persistent_ram_zone - Details of a persistent RAM zone (PRZ)
+ *                              used as a pstore backend
+ *
+ * @paddr:	physical address of the mapped RAM area
+ * @size:	size of mapping
+ * @label:	unique name of this PRZ
+ * @type:	frontend type for this PRZ
+ * @flags:	holds PRZ_FLAGS_* bits
+ *
+ * @buffer_lock:
+ *	locks access to @buffer "size" bytes and "start" offset
+ * @buffer:
+ *	pointer to actual RAM area managed by this PRZ
+ * @buffer_size:
+ *	bytes in @buffer->data (not including any trailing ECC bytes)
+ *
+ * @par_buffer:
+ *	pointer into @buffer->data containing ECC bytes for @buffer->data
+ * @par_header:
+ *	pointer into @buffer->data containing ECC bytes for @buffer header
+ *	(i.e. all fields up to @data)
+ * @rs_decoder:
+ *	RSLIB instance for doing ECC calculations
+ * @corrected_bytes:
+ *	ECC corrected bytes accounting since boot
+ * @bad_blocks:
+ *	ECC uncorrectable bytes accounting since boot
+ * @ecc_info:
+ *	ECC configuration details
+ *
+ * @old_log:
+ *	saved copy of @buffer->data prior to most recent wipe
+ * @old_log_size:
+ *	bytes contained in @old_log
+ *
+ */
+struct persistent_ram_zone {
+	phys_addr_t paddr;
+	size_t size;
+	void *vaddr;
+	char *label;
+	enum pstore_type_id type;
+	u32 flags;
+
+	raw_spinlock_t buffer_lock;
+	struct persistent_ram_buffer *buffer;
+	size_t buffer_size;
+
+	char *par_buffer;
+	char *par_header;
+	struct rs_control *rs_decoder;
+	int corrected_bytes;
+	int bad_blocks;
+	struct persistent_ram_ecc_info ecc_info;
+
+	char *old_log;
+	size_t old_log_size;
+};
+
+struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
+			u32 sig, struct persistent_ram_ecc_info *ecc_info,
+			unsigned int memtype, u32 flags, char *label);
+void persistent_ram_free(struct persistent_ram_zone *prz);
+void persistent_ram_zap(struct persistent_ram_zone *prz);
+
+int persistent_ram_write(struct persistent_ram_zone *prz, const void *s,
+			 unsigned int count);
+int persistent_ram_write_user(struct persistent_ram_zone *prz,
+			      const void __user *s, unsigned int count);
+
+void persistent_ram_save_old(struct persistent_ram_zone *prz);
+size_t persistent_ram_old_size(struct persistent_ram_zone *prz);
+void *persistent_ram_old(struct persistent_ram_zone *prz);
+void persistent_ram_free_old(struct persistent_ram_zone *prz);
+ssize_t persistent_ram_ecc_string(struct persistent_ram_zone *prz,
+	char *str, size_t len);
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 9f16afec7290..9d65ff94e216 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -8,28 +8,7 @@
 #ifndef __LINUX_PSTORE_RAM_H__
 #define __LINUX_PSTORE_RAM_H__
 
-#include <linux/compiler.h>
-#include <linux/device.h>
-#include <linux/init.h>
-#include <linux/kernel.h>
-#include <linux/list.h>
 #include <linux/pstore.h>
-#include <linux/types.h>
-
-/*
- * Choose whether access to the RAM zone requires locking or not.  If a zone
- * can be written to from different CPUs like with ftrace for example, then
- * PRZ_FLAG_NO_LOCK is used. For all other cases, locking is required.
- */
-#define PRZ_FLAG_NO_LOCK	BIT(0)
-/*
- * If a PRZ should only have a single-boot lifetime, this marks it as
- * getting wiped after its contents get copied out after boot.
- */
-#define PRZ_FLAG_ZAP_OLD	BIT(1)
-
-struct persistent_ram_buffer;
-struct rs_control;
 
 struct persistent_ram_ecc_info {
 	int block_size;
@@ -39,84 +18,6 @@ struct persistent_ram_ecc_info {
 	uint16_t *par;
 };
 
-/**
- * struct persistent_ram_zone - Details of a persistent RAM zone (PRZ)
- *                              used as a pstore backend
- *
- * @paddr:	physical address of the mapped RAM area
- * @size:	size of mapping
- * @label:	unique name of this PRZ
- * @type:	frontend type for this PRZ
- * @flags:	holds PRZ_FLAGS_* bits
- *
- * @buffer_lock:
- *	locks access to @buffer "size" bytes and "start" offset
- * @buffer:
- *	pointer to actual RAM area managed by this PRZ
- * @buffer_size:
- *	bytes in @buffer->data (not including any trailing ECC bytes)
- *
- * @par_buffer:
- *	pointer into @buffer->data containing ECC bytes for @buffer->data
- * @par_header:
- *	pointer into @buffer->data containing ECC bytes for @buffer header
- *	(i.e. all fields up to @data)
- * @rs_decoder:
- *	RSLIB instance for doing ECC calculations
- * @corrected_bytes:
- *	ECC corrected bytes accounting since boot
- * @bad_blocks:
- *	ECC uncorrectable bytes accounting since boot
- * @ecc_info:
- *	ECC configuration details
- *
- * @old_log:
- *	saved copy of @buffer->data prior to most recent wipe
- * @old_log_size:
- *	bytes contained in @old_log
- *
- */
-struct persistent_ram_zone {
-	phys_addr_t paddr;
-	size_t size;
-	void *vaddr;
-	char *label;
-	enum pstore_type_id type;
-	u32 flags;
-
-	raw_spinlock_t buffer_lock;
-	struct persistent_ram_buffer *buffer;
-	size_t buffer_size;
-
-	char *par_buffer;
-	char *par_header;
-	struct rs_control *rs_decoder;
-	int corrected_bytes;
-	int bad_blocks;
-	struct persistent_ram_ecc_info ecc_info;
-
-	char *old_log;
-	size_t old_log_size;
-};
-
-struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
-			u32 sig, struct persistent_ram_ecc_info *ecc_info,
-			unsigned int memtype, u32 flags, char *label);
-void persistent_ram_free(struct persistent_ram_zone *prz);
-void persistent_ram_zap(struct persistent_ram_zone *prz);
-
-int persistent_ram_write(struct persistent_ram_zone *prz, const void *s,
-			 unsigned int count);
-int persistent_ram_write_user(struct persistent_ram_zone *prz,
-			      const void __user *s, unsigned int count);
-
-void persistent_ram_save_old(struct persistent_ram_zone *prz);
-size_t persistent_ram_old_size(struct persistent_ram_zone *prz);
-void *persistent_ram_old(struct persistent_ram_zone *prz);
-void persistent_ram_free_old(struct persistent_ram_zone *prz);
-ssize_t persistent_ram_ecc_string(struct persistent_ram_zone *prz,
-	char *str, size_t len);
-
 /*
  * Ramoops platform data
  * @mem_size	memory size for ramoops
-- 
2.34.1

Re: [PATCH 3/5] pstore/ram: Move internal definitions out of kernel-wide include

[Guilherme G. Piccoli]

On 11/10/2022 17:01, Kees Cook wrote:
> Most of the details of the ram backend are entirely internal to the
> backend itself. Leave only what is needed to instantiate a ram backend
> in the kernel-wide header.
> 
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

Reviewed-and-tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Tested by building and booting the kernel - since it's just header file
rework, guess this is enough right?

BTW, let me know if you prefer me to respond on the series once or per
patch Kees (as I'm doing here).

Cheers,

Guilherme

Re: [PATCH 3/5] pstore/ram: Move internal definitions out of kernel-wide include

[Kees Cook]

On Wed, Oct 12, 2022 at 04:46:44PM -0300, Guilherme G. Piccoli wrote:
> On 11/10/2022 17:01, Kees Cook wrote:
> > Most of the details of the ram backend are entirely internal to the
> > backend itself. Leave only what is needed to instantiate a ram backend
> > in the kernel-wide header.
> > 
> > Cc: Anton Vorontsov <anton@enomsg.org>
> > Cc: Colin Cross <ccross@android.com>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> 
> Reviewed-and-tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> 
> Tested by building and booting the kernel - since it's just header file
> rework, guess this is enough right?
> 
> BTW, let me know if you prefer me to respond on the series once or per
> patch Kees (as I'm doing here).

Thanks! Yeah, per-patch is good. :)

-- 
Kees Cook

[PATCH 4/5] pstore/ram: Set freed addresses to NULL

[Kees Cook]

For good measure, set all the freed addresses to NULL when managing
przs.

Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Tony Luck <tony.luck@intel.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/ram.c          | 13 ++++++++-----
 fs/pstore/ram_core.c     | 11 +++++++++--
 fs/pstore/ram_internal.h |  2 +-
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index f5bf360cf905..5da31565f93a 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -453,25 +453,27 @@ static void ramoops_free_przs(struct ramoops_context *cxt)
 	int i;
 
 	/* Free pmsg PRZ */
-	persistent_ram_free(cxt->mprz);
+	persistent_ram_free(&cxt->mprz);
 
 	/* Free console PRZ */
-	persistent_ram_free(cxt->cprz);
+	persistent_ram_free(&cxt->cprz);
 
 	/* Free dump PRZs */
 	if (cxt->dprzs) {
 		for (i = 0; i < cxt->max_dump_cnt; i++)
-			persistent_ram_free(cxt->dprzs[i]);
+			persistent_ram_free(&cxt->dprzs[i]);
 
 		kfree(cxt->dprzs);
+		cxt->fprzs = NULL;
 		cxt->max_dump_cnt = 0;
 	}
 
 	/* Free ftrace PRZs */
 	if (cxt->fprzs) {
 		for (i = 0; i < cxt->max_ftrace_cnt; i++)
-			persistent_ram_free(cxt->fprzs[i]);
+			persistent_ram_free(&cxt->fprzs[i]);
 		kfree(cxt->fprzs);
+		cxt->fprzs = NULL;
 		cxt->max_ftrace_cnt = 0;
 	}
 }
@@ -555,9 +557,10 @@ static int ramoops_init_przs(const char *name,
 
 			while (i > 0) {
 				i--;
-				persistent_ram_free(prz_ar[i]);
+				persistent_ram_free(&prz_ar[i]);
 			}
 			kfree(prz_ar);
+			prz_ar = NULL;
 			goto fail;
 		}
 		*paddr += zone_sz;
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 9e1047f4316d..97dde525150a 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -544,8 +544,14 @@ static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig,
 	return 0;
 }
 
-void persistent_ram_free(struct persistent_ram_zone *prz)
+void persistent_ram_free(struct persistent_ram_zone **_prz)
 {
+	struct persistent_ram_zone *prz;
+
+	if (!_prz)
+		return;
+
+	prz = *_prz;
 	if (!prz)
 		return;
 
@@ -569,6 +575,7 @@ void persistent_ram_free(struct persistent_ram_zone *prz)
 	persistent_ram_free_old(prz);
 	kfree(prz->label);
 	kfree(prz);
+	*_prz = NULL;
 }
 
 struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
@@ -605,6 +612,6 @@ struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
 
 	return prz;
 err:
-	persistent_ram_free(prz);
+	persistent_ram_free(&prz);
 	return ERR_PTR(ret);
 }
diff --git a/fs/pstore/ram_internal.h b/fs/pstore/ram_internal.h
index 440ee7a35e10..5f694698351f 100644
--- a/fs/pstore/ram_internal.h
+++ b/fs/pstore/ram_internal.h
@@ -82,7 +82,7 @@ struct persistent_ram_zone {
 struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
 			u32 sig, struct persistent_ram_ecc_info *ecc_info,
 			unsigned int memtype, u32 flags, char *label);
-void persistent_ram_free(struct persistent_ram_zone *prz);
+void persistent_ram_free(struct persistent_ram_zone **_prz);
 void persistent_ram_zap(struct persistent_ram_zone *prz);
 
 int persistent_ram_write(struct persistent_ram_zone *prz, const void *s,
-- 
2.34.1

Re: [PATCH 4/5] pstore/ram: Set freed addresses to NULL

[Guilherme G. Piccoli]

On 11/10/2022 17:01, Kees Cook wrote:
> For good measure, set all the freed addresses to NULL when managing
> przs.
> 
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

Reviewed-and-tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Tested by building / booting and triggering crashes with efi and ramoops
backends.
Cheers,


Guilherme

[PATCH 5/5] MAINTAINERS: Update pstore maintainers

[Kees Cook]

Update pstore to better reflect reality of active contributors:

- Remove Anton and Colin (thank you for your help through the years!)
- Move Tony to Reviewer
- Add Guilherme as Reviewer
- Add mailing list
- Upgrade to Supported

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 MAINTAINERS | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9d7f64dc0efe..bb18a6c91c4e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16458,10 +16458,10 @@ F:	net/psample
 
 PSTORE FILESYSTEM
 M:	Kees Cook <keescook@chromium.org>
-M:	Anton Vorontsov <anton@enomsg.org>
-M:	Colin Cross <ccross@android.com>
-M:	Tony Luck <tony.luck@intel.com>
-S:	Maintained
+R:	Tony Luck <tony.luck@intel.com>
+R:	Guilherme G. Piccoli <gpiccoli@igalia.com>
+L:	linux-hardening@vger.kernel.org
+S:	Supported
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/pstore
 F:	Documentation/admin-guide/ramoops.rst
 F:	Documentation/admin-guide/pstore-blk.rst
-- 
2.34.1

Re: [PATCH 5/5] MAINTAINERS: Update pstore maintainers

[Colin Cross]

On Tue, Oct 11, 2022 at 1:01 PM Kees Cook <keescook@chromium.org> wrote:
>
> Update pstore to better reflect reality of active contributors:
>
> - Remove Anton and Colin (thank you for your help through the years!)
> - Move Tony to Reviewer
> - Add Guilherme as Reviewer
> - Add mailing list
> - Upgrade to Supported
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  MAINTAINERS | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9d7f64dc0efe..bb18a6c91c4e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16458,10 +16458,10 @@ F:    net/psample
>
>  PSTORE FILESYSTEM
>  M:     Kees Cook <keescook@chromium.org>
> -M:     Anton Vorontsov <anton@enomsg.org>
> -M:     Colin Cross <ccross@android.com>
> -M:     Tony Luck <tony.luck@intel.com>
> -S:     Maintained
> +R:     Tony Luck <tony.luck@intel.com>
> +R:     Guilherme G. Piccoli <gpiccoli@igalia.com>
> +L:     linux-hardening@vger.kernel.org
> +S:     Supported
>  T:     git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/pstore
>  F:     Documentation/admin-guide/ramoops.rst
>  F:     Documentation/admin-guide/pstore-blk.rst
> --
> 2.34.1
>

Acked-by: Colin Cross <ccross@android.com>

Re: [PATCH 5/5] MAINTAINERS: Update pstore maintainers

[Guilherme G. Piccoli]

On 11/10/2022 17:01, Kees Cook wrote:
> Update pstore to better reflect reality of active contributors:
> 
> - Remove Anton and Colin (thank you for your help through the years!)
> - Move Tony to Reviewer
> - Add Guilherme as Reviewer
> - Add mailing list
> - Upgrade to Supported
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  MAINTAINERS | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9d7f64dc0efe..bb18a6c91c4e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16458,10 +16458,10 @@ F:	net/psample
>  
>  PSTORE FILESYSTEM
>  M:	Kees Cook <keescook@chromium.org>
> -M:	Anton Vorontsov <anton@enomsg.org>
> -M:	Colin Cross <ccross@android.com>
> -M:	Tony Luck <tony.luck@intel.com>
> -S:	Maintained
> +R:	Tony Luck <tony.luck@intel.com>
> +R:	Guilherme G. Piccoli <gpiccoli@igalia.com>
> +L:	linux-hardening@vger.kernel.org
> +S:	Supported
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/pstore
>  F:	Documentation/admin-guide/ramoops.rst
>  F:	Documentation/admin-guide/pstore-blk.rst

Thanks Kees!
Acked-by: Guilherme G. Piccoli <gpiccoli@igalia.com>


Just curious, why linux-hardening was the picked list?
Cheers,


Guilherme

Re: [PATCH 5/5] MAINTAINERS: Update pstore maintainers

[Kees Cook]

On Tue, Oct 11, 2022 at 05:35:48PM -0300, Guilherme G. Piccoli wrote:
> On 11/10/2022 17:01, Kees Cook wrote:
> > +L:	linux-hardening@vger.kernel.org
> 
> Just curious, why linux-hardening was the picked list?

It's where the bulk of other things I work on end up living, and there's
an active patchwork instance, so it'll do patch lifetime tracking for
us:
https://patchwork.kernel.org/project/linux-hardening/list/

-- 
Kees Cook

Re: [PATCH 5/5] MAINTAINERS: Update pstore maintainers

[Guilherme G. Piccoli]

On 11/10/2022 19:20, Kees Cook wrote:
> On Tue, Oct 11, 2022 at 05:35:48PM -0300, Guilherme G. Piccoli wrote:
>> On 11/10/2022 17:01, Kees Cook wrote:
>>> +L:	linux-hardening@vger.kernel.org
>>
>> Just curious, why linux-hardening was the picked list?
> 
> It's where the bulk of other things I work on end up living, and there's
> an active patchwork instance, so it'll do patch lifetime tracking for
> us:
> https://patchwork.kernel.org/project/linux-hardening/list/
> 

Makes sense, and very nice that it has the patchwork set!
Thanks,


Guilherme

Re: [PATCH 5/5] MAINTAINERS: Update pstore maintainers

[Guilherme G. Piccoli]

On 11/10/2022 17:01, Kees Cook wrote:
> Update pstore to better reflect reality of active contributors:
> 
> - Remove Anton and Colin (thank you for your help through the years!)
> - Move Tony to Reviewer
> - Add Guilherme as Reviewer
> - Add mailing list
> - Upgrade to Supported
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  MAINTAINERS | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Hi Kees, sorry to revamp this thread for a "tangential" topic, but it
feels a "kinda" proper thread.

Since I was added as a reviewer on pstore (in linux-next so far), I
started to receive a bunch of emails from ARM device-tree folks; they're
adding ramoops entries to their DTs and looping pstore folks.

Examples:

https://lore.kernel.org/linux-hardening/20221111120156.48040-1-angelogioacchino.delregno@collabora.com/

https://lore.kernel.org/linux-hardening/20221116145616.17884-1-luca@z3ntu.xyz/


Personally, I have no knowledge of these HW to evaluate if the ramoops
setting is appropriate, so they're nop from my side, I just delete them.
But that raises the question - are you/Tony reviewing this kind of
change? It's not related to pstore/ramoops code, it's just users setting
ramoops in their DTs, so seems to me a bit far from the purpose of the
pstore entry.

What do you/Tony think about that? Likely the DT folks are following
this entry in the MAINTAINERS to send these emails:

PSTORE FILESYSTEM
M:      Kees Cook <keescook@chromium.org>
[...]
F:      include/linux/pstore*
K:      \b(pstore|ramoops) <------

Should this be kept? Maybe only the ramoops entry could be removed?

Again, totally fine be me to keep'em, it's just that I'm receiving and
ignoring the emails, so if everybody else is doing the same, better to
just remove this entry from MAINTAINERS.

Thanks,


Guilherme

Re: [PATCH 5/5] MAINTAINERS: Update pstore maintainers

[Kees Cook]

On Wed, Nov 16, 2022 at 12:42:53PM -0300, Guilherme G. Piccoli wrote:
> On 11/10/2022 17:01, Kees Cook wrote:
> > Update pstore to better reflect reality of active contributors:
> > 
> > - Remove Anton and Colin (thank you for your help through the years!)
> > - Move Tony to Reviewer
> > - Add Guilherme as Reviewer
> > - Add mailing list
> > - Upgrade to Supported
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  MAINTAINERS | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> Hi Kees, sorry to revamp this thread for a "tangential" topic, but it
> feels a "kinda" proper thread.
> 
> Since I was added as a reviewer on pstore (in linux-next so far), I
> started to receive a bunch of emails from ARM device-tree folks; they're
> adding ramoops entries to their DTs and looping pstore folks.
> 
> Examples:
> 
> https://lore.kernel.org/linux-hardening/20221111120156.48040-1-angelogioacchino.delregno@collabora.com/
> 
> https://lore.kernel.org/linux-hardening/20221116145616.17884-1-luca@z3ntu.xyz/
> 
> 
> Personally, I have no knowledge of these HW to evaluate if the ramoops
> setting is appropriate, so they're nop from my side, I just delete them.
> But that raises the question - are you/Tony reviewing this kind of
> change? It's not related to pstore/ramoops code, it's just users setting
> ramoops in their DTs, so seems to me a bit far from the purpose of the
> pstore entry.

I usually look at it very quickly, but I can't meaningfully positively
review it because I don't know the hardware, etc.

> What do you/Tony think about that? Likely the DT folks are following
> this entry in the MAINTAINERS to send these emails:
> 
> PSTORE FILESYSTEM
> M:      Kees Cook <keescook@chromium.org>
> [...]
> F:      include/linux/pstore*
> K:      \b(pstore|ramoops) <------
> 
> Should this be kept? Maybe only the ramoops entry could be removed?

I would like to keep it -- if something mentions pstore and ramoops, I'd
like to see it. I can't review all of it, but I'd like it to at least
show up in my inbox. :)

-- 
Kees Cook

Re: [PATCH 5/5] MAINTAINERS: Update pstore maintainers

[Guilherme G. Piccoli]

On 17/11/2022 19:01, Kees Cook wrote:
> [...]
>> PSTORE FILESYSTEM
>> M:      Kees Cook <keescook@chromium.org>
>> [...]
>> F:      include/linux/pstore*
>> K:      \b(pstore|ramoops) <------
>>
>> Should this be kept? Maybe only the ramoops entry could be removed?
> 
> I would like to keep it -- if something mentions pstore and ramoops, I'd
> like to see it. I can't review all of it, but I'd like it to at least
> show up in my inbox. :)
> 

Totally fine by me, was curious and now I understand this is not legacy,
but something of your interest indeed.
Thanks for the prompt response!

Cheers,


Guilherme