[PATCH 0/8] pstore improvements (pstore-next)

[PATCH 0/8] pstore improvements (pstore-next)

[Kees Cook]

This is a posting of several patches I've been working on to improve
pstore. Most of it is better comments, output, and naming, but one
bug fix stands out to fix head-truncationg of compressed records.
Details in the individual patches. Review appreciated! :)

-Kees

Kees Cook (8):
  pstore/ram: Standardize module name in ramoops
  pstore: Do not use crash buffer for decompression
  pstore/ram: Report backend assignments with finer granularity
  pstore/ram: Add kern-doc for struct persistent_ram_zone
  pstore: Improve and update some comments and status output
  pstore: Replace open-coded << with BIT()
  pstore: Remove needless lock during console writes
  pstore/ram: Correctly calculate usable PRZ bytes

 fs/pstore/platform.c       | 92 ++++++++++++++------------------------
 fs/pstore/ram.c            | 19 ++++----
 fs/pstore/ram_core.c       | 25 +++++++++--
 include/linux/pstore.h     | 15 ++++---
 include/linux/pstore_ram.h | 46 +++++++++++++++++--
 5 files changed, 116 insertions(+), 81 deletions(-)

-- 
2.17.1

[PATCH linux-next 1/8] pstore/ram: Standardize module name in ramoops

[Kees Cook]

With both ram.c and ram_core.c built into ramoops.ko, it doesn't make
sense to have differing pr_fmt prefixes. This fixes ram_core.c to use
the module name (as ram.c already does). Additionally improves region
reservation error to include the region name.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/ram_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 23ca6f2c98a0..f5d0173901aa 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -12,7 +12,7 @@
  *
  */
 
-#define pr_fmt(fmt) "persistent_ram: " fmt
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/device.h>
 #include <linux/err.h>
@@ -443,7 +443,8 @@ static void *persistent_ram_iomap(phys_addr_t start, size_t size,
 	void *va;
 
 	if (!request_mem_region(start, size, label ?: "ramoops")) {
-		pr_err("request mem region (0x%llx@0x%llx) failed\n",
+		pr_err("request mem region (%s 0x%llx@0x%llx) failed\n",
+			label ?: "ramoops",
 			(unsigned long long)size, (unsigned long long)start);
 		return NULL;
 	}
-- 
2.17.1

[PATCH 2/8] pstore: Do not use crash buffer for decompression

[Kees Cook]

The pre-allocated compression buffer used for crash dumping was also
being used for decompression. This isn't technically safe, since it's
possible the kernel may attempt a crashdump while pstore is populating the
pstore filesystem (and performing decompression). Instead, just allocate
a separate buffer for decompression. Correctness is preferred over
performance here.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/platform.c | 56 ++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 31 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index b821054ca3ed..8b6028948cf3 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -258,20 +258,6 @@ static int pstore_compress(const void *in, void *out,
 	return outlen;
 }
 
-static int pstore_decompress(void *in, void *out,
-			     unsigned int inlen, unsigned int outlen)
-{
-	int ret;
-
-	ret = crypto_comp_decompress(tfm, in, inlen, out, &outlen);
-	if (ret) {
-		pr_err("crypto_comp_decompress failed, ret = %d!\n", ret);
-		return ret;
-	}
-
-	return outlen;
-}
-
 static void allocate_buf_for_compression(void)
 {
 	struct crypto_comp *ctx;
@@ -656,8 +642,9 @@ EXPORT_SYMBOL_GPL(pstore_unregister);
 
 static void decompress_record(struct pstore_record *record)
 {
+	int ret;
 	int unzipped_len;
-	char *decompressed;
+	char *unzipped, *workspace;
 
 	if (!record->compressed)
 		return;
@@ -668,35 +655,42 @@ static void decompress_record(struct pstore_record *record)
 		return;
 	}
 
-	/* No compression method has created the common buffer. */
+	/* Missing compression buffer means compression was not initialized. */
 	if (!big_oops_buf) {
-		pr_warn("no decompression buffer allocated\n");
+		pr_warn("no decompression method initialized!\n");
 		return;
 	}
 
-	unzipped_len = pstore_decompress(record->buf, big_oops_buf,
-					 record->size, big_oops_buf_sz);
-	if (unzipped_len <= 0) {
-		pr_err("decompression failed: %d\n", unzipped_len);
+	/* Allocate enough space to hold max decompression and ECC. */
+	unzipped_len = big_oops_buf_sz;
+	workspace = kmalloc(unzipped_len + record->ecc_notice_size,
+			    GFP_KERNEL);
+	if (!workspace)
 		return;
-	}
 
-	/* Build new buffer for decompressed contents. */
-	decompressed = kmalloc(unzipped_len + record->ecc_notice_size,
-			       GFP_KERNEL);
-	if (!decompressed) {
-		pr_err("decompression ran out of memory\n");
+	/* After decompression "unzipped_len" is almost certainly smaller. */
+	ret = crypto_comp_decompress(tfm, record->buf, record->size,
+					  workspace, &unzipped_len);
+	if (ret) {
+		pr_err("crypto_comp_decompress failed, ret = %d!\n", ret);
+		kfree(workspace);
 		return;
 	}
-	memcpy(decompressed, big_oops_buf, unzipped_len);
 
 	/* Append ECC notice to decompressed buffer. */
-	memcpy(decompressed + unzipped_len, record->buf + record->size,
+	memcpy(workspace + unzipped_len, record->buf + record->size,
 	       record->ecc_notice_size);
 
-	/* Swap out compresed contents with decompressed contents. */
+	/* Copy decompressed contents into an minimum-sized allocation. */
+	unzipped = kmemdup(workspace, unzipped_len + record->ecc_notice_size,
+			   GFP_KERNEL);
+	kfree(workspace);
+	if (!unzipped)
+		return;
+
+	/* Swap out compressed contents with decompressed contents. */
 	kfree(record->buf);
-	record->buf = decompressed;
+	record->buf = unzipped;
 	record->size = unzipped_len;
 	record->compressed = false;
 }
-- 
2.17.1

Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression

[Joel Fernandes]

On Thu, Nov 01, 2018 at 04:51:54PM -0700, Kees Cook wrote:
> The pre-allocated compression buffer used for crash dumping was also
> being used for decompression. This isn't technically safe, since it's
> possible the kernel may attempt a crashdump while pstore is populating the
> pstore filesystem (and performing decompression). Instead, just allocate

Yeah, that would be bad if it happened ;)

> a separate buffer for decompression. Correctness is preferred over
> performance here.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  fs/pstore/platform.c | 56 ++++++++++++++++++++------------------------
>  1 file changed, 25 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index b821054ca3ed..8b6028948cf3 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -258,20 +258,6 @@ static int pstore_compress(const void *in, void *out,
>  	return outlen;
>  }
>  
> -static int pstore_decompress(void *in, void *out,
> -			     unsigned int inlen, unsigned int outlen)
> -{
> -	int ret;
> -
> -	ret = crypto_comp_decompress(tfm, in, inlen, out, &outlen);
> -	if (ret) {
> -		pr_err("crypto_comp_decompress failed, ret = %d!\n", ret);
> -		return ret;
> -	}
> -
> -	return outlen;
> -}
> -
>  static void allocate_buf_for_compression(void)
>  {
>  	struct crypto_comp *ctx;
> @@ -656,8 +642,9 @@ EXPORT_SYMBOL_GPL(pstore_unregister);
>  
>  static void decompress_record(struct pstore_record *record)
>  {
> +	int ret;
>  	int unzipped_len;

nit: We could get rid of the unzipped_len variable now I think.

> -	char *decompressed;
> +	char *unzipped, *workspace;
>  
>  	if (!record->compressed)
>  		return;
> @@ -668,35 +655,42 @@ static void decompress_record(struct pstore_record *record)
>  		return;
>  	}
>  
> -	/* No compression method has created the common buffer. */
> +	/* Missing compression buffer means compression was not initialized. */
>  	if (!big_oops_buf) {
> -		pr_warn("no decompression buffer allocated\n");
> +		pr_warn("no decompression method initialized!\n");
>  		return;
>  	}
>  
> -	unzipped_len = pstore_decompress(record->buf, big_oops_buf,
> -					 record->size, big_oops_buf_sz);
> -	if (unzipped_len <= 0) {
> -		pr_err("decompression failed: %d\n", unzipped_len);
> +	/* Allocate enough space to hold max decompression and ECC. */
> +	unzipped_len = big_oops_buf_sz;
> +	workspace = kmalloc(unzipped_len + record->ecc_notice_size,

Should tihs be unzipped_len + record->ecc_notice_size + 1. The extra byte
being for the NULL character of the ecc notice?

This occurred to me when I saw the + 1 in ram.c. It could be better to just
abstract the size as a macro.

> +			    GFP_KERNEL);
> +	if (!workspace)
>  		return;
> -	}
>  
> -	/* Build new buffer for decompressed contents. */
> -	decompressed = kmalloc(unzipped_len + record->ecc_notice_size,
> -			       GFP_KERNEL);
> -	if (!decompressed) {
> -		pr_err("decompression ran out of memory\n");
> +	/* After decompression "unzipped_len" is almost certainly smaller. */
> +	ret = crypto_comp_decompress(tfm, record->buf, record->size,
> +					  workspace, &unzipped_len);
> +	if (ret) {
> +		pr_err("crypto_comp_decompress failed, ret = %d!\n", ret);
> +		kfree(workspace);
>  		return;
>  	}
> -	memcpy(decompressed, big_oops_buf, unzipped_len);
>  
>  	/* Append ECC notice to decompressed buffer. */
> -	memcpy(decompressed + unzipped_len, record->buf + record->size,
> +	memcpy(workspace + unzipped_len, record->buf + record->size,
>  	       record->ecc_notice_size);
>  
> -	/* Swap out compresed contents with decompressed contents. */
> +	/* Copy decompressed contents into an minimum-sized allocation. */
> +	unzipped = kmemdup(workspace, unzipped_len + record->ecc_notice_size,
> +			   GFP_KERNEL);
> +	kfree(workspace);
> +	if (!unzipped)
> +		return;
> +
> +	/* Swap out compressed contents with decompressed contents. */
>  	kfree(record->buf);
> -	record->buf = decompressed;
> +	record->buf = unzipped;

Rest of it LGTM, thanks!

 - Joel

Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression

[Kees Cook]

On Fri, Nov 2, 2018 at 1:24 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> On Thu, Nov 01, 2018 at 04:51:54PM -0700, Kees Cook wrote:
>>  static void decompress_record(struct pstore_record *record)
>>  {
>> +     int ret;
>>       int unzipped_len;
>
> nit: We could get rid of the unzipped_len variable now I think.

I didn't follow this -- it gets used quite a bit. I don't see a clean
way to remove it?

>> +     workspace = kmalloc(unzipped_len + record->ecc_notice_size,
>
> Should tihs be unzipped_len + record->ecc_notice_size + 1. The extra byte
> being for the NULL character of the ecc notice?
>
> This occurred to me when I saw the + 1 in ram.c. It could be better to just
> abstract the size as a macro.

Ooh, yes, good catch. I'll get this fixed.

Thanks for the review!

-- 
Kees Cook

Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression

[Joel Fernandes]

On Wed, Nov 14, 2018 at 01:56:09AM -0600, Kees Cook wrote:
> On Fri, Nov 2, 2018 at 1:24 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> > On Thu, Nov 01, 2018 at 04:51:54PM -0700, Kees Cook wrote:
> >>  static void decompress_record(struct pstore_record *record)
> >>  {
> >> +     int ret;
> >>       int unzipped_len;
> >
> > nit: We could get rid of the unzipped_len variable now I think.
> 
> I didn't follow this -- it gets used quite a bit. I don't see a clean
> way to remove it?

You are right. Sorry I missed that crpyto_comp_decompress actually uses it.

thanks,

 - Joel

Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression

[Kees Cook]

On Tue, Nov 13, 2018 at 11:56 PM Kees Cook <keescook@chromium.org> wrote:
> On Fri, Nov 2, 2018 at 1:24 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> > On Thu, Nov 01, 2018 at 04:51:54PM -0700, Kees Cook wrote:
> >> +     workspace = kmalloc(unzipped_len + record->ecc_notice_size,
> >
> > Should tihs be unzipped_len + record->ecc_notice_size + 1. The extra byte
> > being for the NULL character of the ecc notice?
> >
> > This occurred to me when I saw the + 1 in ram.c. It could be better to just
> > abstract the size as a macro.
>
> Ooh, yes, good catch. I'll get this fixed.

I spent more time looking at this, and it seems that only the initial
creation of this string needs the +1, since all other operations are
byte-based not NUL-terminated string based. It's a big odd, and I
might try to clean it up differently, but as it stands, this is okay.
(See inode.c which doesn't include the trailing NUL byte.)

-Kees

-- 
Kees Cook

Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression

[Joel Fernandes]

On Thu, Nov 29, 2018 at 02:06:39PM -0800, Kees Cook wrote:
> On Tue, Nov 13, 2018 at 11:56 PM Kees Cook <keescook@chromium.org> wrote:
> > On Fri, Nov 2, 2018 at 1:24 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> > > On Thu, Nov 01, 2018 at 04:51:54PM -0700, Kees Cook wrote:
> > >> +     workspace = kmalloc(unzipped_len + record->ecc_notice_size,
> > >
> > > Should tihs be unzipped_len + record->ecc_notice_size + 1. The extra byte
> > > being for the NULL character of the ecc notice?
> > >
> > > This occurred to me when I saw the + 1 in ram.c. It could be better to just
> > > abstract the size as a macro.
> >
> > Ooh, yes, good catch. I'll get this fixed.
> 
> I spent more time looking at this, and it seems that only the initial
> creation of this string needs the +1, since all other operations are
> byte-based not NUL-terminated string based. It's a big odd, and I
> might try to clean it up differently, but as it stands, this is okay.
> (See inode.c which doesn't include the trailing NUL byte.)

Ok. Yes it does seem a bit inconsistent but I agree its not an issue for this
particular patch. Sorry to waste your time, thanks!

 - Joel

[PATCH 3/8] pstore/ram: Report backend assignments with finer granularity

[Kees Cook]

In order to more easily perform automated regression testing, this
adds pr_debug() calls to report each prz allocation which can then be
verified against persistent storage. Specifically, seeing the dividing
line between header, data, any ECC bytes. (And the general assignment
output is updated to remove the bogus ECC blocksize which isn't actually
recorded outside the prz instance.)

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/ram.c      | 4 ++--
 fs/pstore/ram_core.c | 6 ++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index b51901f97dc2..25bede911809 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -856,9 +856,9 @@ static int ramoops_probe(struct platform_device *pdev)
 	ramoops_pmsg_size = pdata->pmsg_size;
 	ramoops_ftrace_size = pdata->ftrace_size;
 
-	pr_info("attached 0x%lx@0x%llx, ecc: %d/%d\n",
+	pr_info("using 0x%lx@0x%llx, ecc: %d\n",
 		cxt->size, (unsigned long long)cxt->phys_addr,
-		cxt->ecc_info.ecc_size, cxt->ecc_info.block_size);
+		cxt->ecc_info.ecc_size);
 
 	return 0;
 
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index f5d0173901aa..d5bf9be82545 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -576,6 +576,12 @@ struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
 	if (ret)
 		goto err;
 
+	pr_debug("attached %s 0x%lx@0x%llx: %lu header, %lu data, %lu ecc (%d/%d)\n",
+		prz->label, prz->size, (unsigned long long)prz->paddr,
+		sizeof(*prz->buffer), prz->buffer_size,
+		prz->size - sizeof(*prz->buffer) - prz->buffer_size,
+		prz->ecc_info.ecc_size, prz->ecc_info.block_size);
+
 	return prz;
 err:
 	persistent_ram_free(prz);
-- 
2.17.1

[PATCH 4/8] pstore/ram: Add kern-doc for struct persistent_ram_zone

[Kees Cook]

The struct persistent_ram_zone wasn't well documented. This adds kern-doc
for it.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/ram_core.c       | 10 +++++++++
 include/linux/pstore_ram.h | 46 +++++++++++++++++++++++++++++++++++---
 2 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index d5bf9be82545..1cda5922b4b4 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -29,6 +29,16 @@
 #include <linux/vmalloc.h>
 #include <asm/page.h>
 
+/**
+ * struct persistent_ram_buffer - persistent circular RAM buffer
+ *
+ * @sig:
+ *	signature to indicate header (PERSISTENT_RAM_SIG xor PRZ-type value)
+ * @start:
+ *	offset into @data where the beginning of the stored bytes begin
+ * @size:
+ *	number of valid bytes stored in @data
+ */
 struct persistent_ram_buffer {
 	uint32_t    sig;
 	atomic_t    start;
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 6e94980357d2..5d10ad51c1c4 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -30,6 +30,10 @@
  * 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;
@@ -43,17 +47,53 @@ 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
+ * @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;
-	struct persistent_ram_buffer *buffer;
-	size_t buffer_size;
 	u32 flags;
+
 	raw_spinlock_t buffer_lock;
+	struct persistent_ram_buffer *buffer;
+	size_t buffer_size;
 
-	/* ECC correction */
 	char *par_buffer;
 	char *par_header;
 	struct rs_control *rs_decoder;
-- 
2.17.1

[PATCH 5/8] pstore: Improve and update some comments and status output

[Kees Cook]

This improves and updates some comments:
 - dump handler comment out of sync from calling convention
 - fix kern-doc typo

and improves status output:
 - reminder that only kernel crash dumps are compressed
 - do not be silent about ECC infrastructure failures

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/platform.c   | 7 +++----
 fs/pstore/ram_core.c   | 4 +++-
 include/linux/pstore.h | 2 +-
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 8b6028948cf3..a956c7bc3f67 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -304,7 +304,7 @@ static void allocate_buf_for_compression(void)
 	big_oops_buf_sz = size;
 	big_oops_buf = buf;
 
-	pr_info("Using compression: %s\n", zbackend->name);
+	pr_info("Using crash dump compression: %s\n", zbackend->name);
 }
 
 static void free_buf_for_compression(void)
@@ -354,9 +354,8 @@ void pstore_record_init(struct pstore_record *record,
 }
 
 /*
- * callback from kmsg_dump. (s2,l2) has the most recently
- * written bytes, older bytes are in (s1,l1). Save as much
- * as we can from the end of the buffer.
+ * callback from kmsg_dump. Save as much as we can (up to kmsg_bytes) from the
+ * end of the buffer.
  */
 static void pstore_dump(struct kmsg_dumper *dumper,
 			enum kmsg_dump_reason reason)
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 1cda5922b4b4..e859e02f67a8 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -503,8 +503,10 @@ static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig,
 	bool zap = !!(prz->flags & PRZ_FLAG_ZAP_OLD);
 
 	ret = persistent_ram_init_ecc(prz, ecc_info);
-	if (ret)
+	if (ret) {
+		pr_warn("ECC failed %s\n", prz->label);
 		return ret;
+	}
 
 	sig ^= PERSISTENT_RAM_SIG;
 
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index a15bc4d48752..877ed81de346 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -85,7 +85,7 @@ struct pstore_record {
 /**
  * struct pstore_info - backend pstore driver structure
  *
- * @owner:	module which is repsonsible for this backend driver
+ * @owner:	module which is responsible for this backend driver
  * @name:	name of the backend driver
  *
  * @buf_lock:	spinlock to serialize access to @buf
-- 
2.17.1

[PATCH 6/8] pstore: Replace open-coded << with BIT()

[Kees Cook]

Minor clean-up to use BIT() (as already done in pstore_ram.h).

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

diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 877ed81de346..3549f2ba865c 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -189,10 +189,10 @@ struct pstore_info {
 };
 
 /* Supported frontends */
-#define PSTORE_FLAGS_DMESG	(1 << 0)
-#define PSTORE_FLAGS_CONSOLE	(1 << 1)
-#define PSTORE_FLAGS_FTRACE	(1 << 2)
-#define PSTORE_FLAGS_PMSG	(1 << 3)
+#define PSTORE_FLAGS_DMESG	BIT(0)
+#define PSTORE_FLAGS_CONSOLE	BIT(1)
+#define PSTORE_FLAGS_FTRACE	BIT(2)
+#define PSTORE_FLAGS_PMSG	BIT(3)
 
 extern int pstore_register(struct pstore_info *);
 extern void pstore_unregister(struct pstore_info *);
-- 
2.17.1

[PATCH 7/8] pstore: Remove needless lock during console writes

[Kees Cook]

Since commit 70ad35db3321 ("pstore: Convert console write to use
->write_buf"), the console writer does not use the preallocated crash
dump buffer any more, so there is no reason to perform locking around it.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/platform.c | 29 ++++++-----------------------
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index a956c7bc3f67..32340e7dd6a5 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -461,31 +461,14 @@ static void pstore_unregister_kmsg(void)
 #ifdef CONFIG_PSTORE_CONSOLE
 static void pstore_console_write(struct console *con, const char *s, unsigned c)
 {
-	const char *e = s + c;
+	struct pstore_record record;
 
-	while (s < e) {
-		struct pstore_record record;
-		unsigned long flags;
-
-		pstore_record_init(&record, psinfo);
-		record.type = PSTORE_TYPE_CONSOLE;
-
-		if (c > psinfo->bufsize)
-			c = psinfo->bufsize;
+	pstore_record_init(&record, psinfo);
+	record.type = PSTORE_TYPE_CONSOLE;
 
-		if (oops_in_progress) {
-			if (!spin_trylock_irqsave(&psinfo->buf_lock, flags))
-				break;
-		} else {
-			spin_lock_irqsave(&psinfo->buf_lock, flags);
-		}
-		record.buf = (char *)s;
-		record.size = c;
-		psinfo->write(&record);
-		spin_unlock_irqrestore(&psinfo->buf_lock, flags);
-		s += c;
-		c = e - s;
-	}
+	record.buf = (char *)s;
+	record.size = c;
+	psinfo->write(&record);
 }
 
 static struct console pstore_console = {
-- 
2.17.1

Re: [PATCH 7/8] pstore: Remove needless lock during console writes

[Joel Fernandes]

On Thu, Nov 01, 2018 at 04:51:59PM -0700, Kees Cook wrote:
> Since commit 70ad35db3321 ("pstore: Convert console write to use
> ->write_buf"), the console writer does not use the preallocated crash
> dump buffer any more, so there is no reason to perform locking around it.

Out of curiosity, what was the reason for having this preallocated crash
buffer in the first place? I thought the 'console' type only did regular
kernel console logging, not crash dumps.

I looked at all the patches and had some minor nits, with the nits addressed
(if you agree with them), feel free to add my Reviewed-by on future respins:

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Also I wonder if Namhyung is still poking around that virtio pstore driver he
mentioned in the commit mentioned above. :)

thanks,

- Joel

> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  fs/pstore/platform.c | 29 ++++++-----------------------
>  1 file changed, 6 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index a956c7bc3f67..32340e7dd6a5 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -461,31 +461,14 @@ static void pstore_unregister_kmsg(void)
>  #ifdef CONFIG_PSTORE_CONSOLE
>  static void pstore_console_write(struct console *con, const char *s, unsigned c)
>  {
> -	const char *e = s + c;
> +	struct pstore_record record;
>  
> -	while (s < e) {
> -		struct pstore_record record;
> -		unsigned long flags;
> -
> -		pstore_record_init(&record, psinfo);
> -		record.type = PSTORE_TYPE_CONSOLE;
> -
> -		if (c > psinfo->bufsize)
> -			c = psinfo->bufsize;
> +	pstore_record_init(&record, psinfo);
> +	record.type = PSTORE_TYPE_CONSOLE;
>  
> -		if (oops_in_progress) {
> -			if (!spin_trylock_irqsave(&psinfo->buf_lock, flags))
> -				break;
> -		} else {
> -			spin_lock_irqsave(&psinfo->buf_lock, flags);
> -		}
> -		record.buf = (char *)s;
> -		record.size = c;
> -		psinfo->write(&record);
> -		spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> -		s += c;
> -		c = e - s;
> -	}
> +	record.buf = (char *)s;
> +	record.size = c;
> +	psinfo->write(&record);
>  }
>  
>  static struct console pstore_console = {
> -- 
> 2.17.1
> 

Re: [PATCH 7/8] pstore: Remove needless lock during console writes

[Kees Cook]

On Fri, Nov 2, 2018 at 11:32 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
> On Thu, Nov 01, 2018 at 04:51:59PM -0700, Kees Cook wrote:
>> Since commit 70ad35db3321 ("pstore: Convert console write to use
>> ->write_buf"), the console writer does not use the preallocated crash
>> dump buffer any more, so there is no reason to perform locking around it.
>
> Out of curiosity, what was the reason for having this preallocated crash
> buffer in the first place? I thought the 'console' type only did regular
> kernel console logging, not crash dumps.

The primary reason is that the dumper needs to write to somewhere and
we don't know the state of the system (memory allocation may not work
for example).

The other frontends tend to run at "sane" locations in the kernel. The
dumper, however, is quite fragile.

> I looked at all the patches and had some minor nits, with the nits addressed
> (if you agree with them), feel free to add my Reviewed-by on future respins:
>
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Thanks!

> Also I wonder if Namhyung is still poking around that virtio pstore driver he
> mentioned in the commit mentioned above. :)

Did that never land? I thought it mostly had to happen at the qemu end?

With nvdimm emulation, we can just use ramoops. :)

-Kees

-- 
Kees Cook

Re: [PATCH 7/8] pstore: Remove needless lock during console writes

[Joel Fernandes]

On Fri, Nov 02, 2018 at 01:40:06PM -0700, Kees Cook wrote:
> On Fri, Nov 2, 2018 at 11:32 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
> > On Thu, Nov 01, 2018 at 04:51:59PM -0700, Kees Cook wrote:
> >> Since commit 70ad35db3321 ("pstore: Convert console write to use
> >> ->write_buf"), the console writer does not use the preallocated crash
> >> dump buffer any more, so there is no reason to perform locking around it.
> >
> > Out of curiosity, what was the reason for having this preallocated crash
> > buffer in the first place? I thought the 'console' type only did regular
> > kernel console logging, not crash dumps.
> 
> The primary reason is that the dumper needs to write to somewhere and
> we don't know the state of the system (memory allocation may not work
> for example).
> 
> The other frontends tend to run at "sane" locations in the kernel. The
> dumper, however, is quite fragile.

Makes sense. thanks.

> > Also I wonder if Namhyung is still poking around that virtio pstore driver he
> > mentioned in the commit mentioned above. :)
> 
> Did that never land? I thought it mostly had to happen at the qemu end?
> 
> With nvdimm emulation, we can just use ramoops. :)
> 

Yes it seems like it never landed: https://lwn.net/Articles/694742/

One of the nice thing about his virtio set though, vs the nvdimm way is that
he actually gets a directory on the host instead of a backing memory file.
Then he can just list this directory and see all the pstore files as he shows
in his example.

I guess it should not be too hard to write a tool to post-process the nvdimm
images and convert it to files anyway :)

thanks,

 - Joel

[PATCH 8/8] pstore/ram: Correctly calculate usable PRZ bytes

[Kees Cook]

The actual number of bytes stored in a PRZ is smaller than the
bytes requested by platform data, since there is a header on each
PRZ. Additionally, if ECC is enabled, there are trailing bytes used
as well. Normally this mismatch doesn't matter since PRZs are circular
buffers and the leading "overflow" bytes are just thrown away. However, in
the case of a compressed record, this rather badly corrupts the results.

This corruption was visible with "ramoops.mem_size=204800 ramoops.ecc=1".
Any stored crashes would not be uncompressable (producing a pstorefs
"dmesg-*.enc.z" file), and triggering errors at boot:

  [    2.790759] pstore: crypto_comp_decompress failed, ret = -22!

Reported-by: Joel Fernandes <joel@joelfernandes.org>
Fixes: b0aad7a99c1d ("pstore: Add compression support to pstore")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/ram.c        | 15 ++++++---------
 include/linux/pstore.h |  5 ++++-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 25bede911809..10ac4d23c423 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -814,17 +814,14 @@ static int ramoops_probe(struct platform_device *pdev)
 
 	cxt->pstore.data = cxt;
 	/*
-	 * Console can handle any buffer size, so prefer LOG_LINE_MAX. If we
-	 * have to handle dumps, we must have at least record_size buffer. And
-	 * for ftrace, bufsize is irrelevant (if bufsize is 0, buf will be
-	 * ZERO_SIZE_PTR).
+	 * Since bufsize is only used for dmesg crash dumps, it
+	 * must match the size of the dprz record (after PRZ header
+	 * and ECC bytes have been accounted for).
 	 */
-	if (cxt->console_size)
-		cxt->pstore.bufsize = 1024; /* LOG_LINE_MAX */
-	cxt->pstore.bufsize = max(cxt->record_size, cxt->pstore.bufsize);
-	cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL);
+	cxt->pstore.bufsize = cxt->dprzs[0]->buffer_size;
+	cxt->pstore.buf = kzalloc(cxt->pstore.bufsize, GFP_KERNEL);
 	if (!cxt->pstore.buf) {
-		pr_err("cannot allocate pstore buffer\n");
+		pr_err("cannot allocate pstore crash dump buffer\n");
 		err = -ENOMEM;
 		goto fail_clear;
 	}
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 3549f2ba865c..f46e5df76b58 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -90,7 +90,10 @@ struct pstore_record {
  *
  * @buf_lock:	spinlock to serialize access to @buf
  * @buf:	preallocated crash dump buffer
- * @bufsize:	size of @buf available for crash dump writes
+ * @bufsize:	size of @buf available for crash dump bytes (must match
+ *		smallest number of bytes available for writing to a
+ *		backend entry, since compressed bytes don't take kindly
+ *		to being truncated)
  *
  * @read_mutex:	serializes @open, @read, @close, and @erase callbacks
  * @flags:	bitfield of frontends the backend can accept writes for
-- 
2.17.1

Re: [PATCH 8/8] pstore/ram: Correctly calculate usable PRZ bytes

[Joel Fernandes]

On Thu, Nov 01, 2018 at 04:52:00PM -0700, Kees Cook wrote:
> The actual number of bytes stored in a PRZ is smaller than the
> bytes requested by platform data, since there is a header on each
> PRZ. Additionally, if ECC is enabled, there are trailing bytes used
> as well. Normally this mismatch doesn't matter since PRZs are circular
> buffers and the leading "overflow" bytes are just thrown away. However, in
> the case of a compressed record, this rather badly corrupts the results.

Actually this would also mean some data loss for non-compressed records were
also there before, but is now fixed?

> This corruption was visible with "ramoops.mem_size=204800 ramoops.ecc=1".
> Any stored crashes would not be uncompressable (producing a pstorefs
> "dmesg-*.enc.z" file), and triggering errors at boot:
> 
>   [    2.790759] pstore: crypto_comp_decompress failed, ret = -22!
> 
> Reported-by: Joel Fernandes <joel@joelfernandes.org>
> Fixes: b0aad7a99c1d ("pstore: Add compression support to pstore")
> Signed-off-by: Kees Cook <keescook@chromium.org>

Thanks!
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Also should this be fixed for other backends or are those good? AFAIR, I saw
this for EFI too.

- Joel



> ---
>  fs/pstore/ram.c        | 15 ++++++---------
>  include/linux/pstore.h |  5 ++++-
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 25bede911809..10ac4d23c423 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -814,17 +814,14 @@ static int ramoops_probe(struct platform_device *pdev)
>  
>  	cxt->pstore.data = cxt;
>  	/*
> -	 * Console can handle any buffer size, so prefer LOG_LINE_MAX. If we
> -	 * have to handle dumps, we must have at least record_size buffer. And
> -	 * for ftrace, bufsize is irrelevant (if bufsize is 0, buf will be
> -	 * ZERO_SIZE_PTR).
> +	 * Since bufsize is only used for dmesg crash dumps, it
> +	 * must match the size of the dprz record (after PRZ header
> +	 * and ECC bytes have been accounted for).
>  	 */
> -	if (cxt->console_size)
> -		cxt->pstore.bufsize = 1024; /* LOG_LINE_MAX */
> -	cxt->pstore.bufsize = max(cxt->record_size, cxt->pstore.bufsize);
> -	cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL);
> +	cxt->pstore.bufsize = cxt->dprzs[0]->buffer_size;
> +	cxt->pstore.buf = kzalloc(cxt->pstore.bufsize, GFP_KERNEL);
>  	if (!cxt->pstore.buf) {
> -		pr_err("cannot allocate pstore buffer\n");
> +		pr_err("cannot allocate pstore crash dump buffer\n");
>  		err = -ENOMEM;
>  		goto fail_clear;
>  	}
> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> index 3549f2ba865c..f46e5df76b58 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -90,7 +90,10 @@ struct pstore_record {
>   *
>   * @buf_lock:	spinlock to serialize access to @buf
>   * @buf:	preallocated crash dump buffer
> - * @bufsize:	size of @buf available for crash dump writes
> + * @bufsize:	size of @buf available for crash dump bytes (must match
> + *		smallest number of bytes available for writing to a
> + *		backend entry, since compressed bytes don't take kindly
> + *		to being truncated)
>   *
>   * @read_mutex:	serializes @open, @read, @close, and @erase callbacks
>   * @flags:	bitfield of frontends the backend can accept writes for
> -- 
> 2.17.1
> 

Re: [PATCH 8/8] pstore/ram: Correctly calculate usable PRZ bytes

[Kees Cook]

On Fri, Nov 2, 2018 at 11:01 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
> On Thu, Nov 01, 2018 at 04:52:00PM -0700, Kees Cook wrote:
>> The actual number of bytes stored in a PRZ is smaller than the
>> bytes requested by platform data, since there is a header on each
>> PRZ. Additionally, if ECC is enabled, there are trailing bytes used
>> as well. Normally this mismatch doesn't matter since PRZs are circular
>> buffers and the leading "overflow" bytes are just thrown away. However, in
>> the case of a compressed record, this rather badly corrupts the results.
>
> Actually this would also mean some data loss for non-compressed records were
> also there before, but is now fixed?

No, it's what I mentioned in the commit log: only the "tail" of any
data was getting stored, which is consistent with the configuration
given. The main problem is that ECC bytes weren't part of the
calculation the ram backend provided to pstore for pstore to pick the
correct amount of bytes to compress.

>> This corruption was visible with "ramoops.mem_size=204800 ramoops.ecc=1".
>> Any stored crashes would not be uncompressable (producing a pstorefs
>> "dmesg-*.enc.z" file), and triggering errors at boot:
>>
>>   [    2.790759] pstore: crypto_comp_decompress failed, ret = -22!
>>
>> Reported-by: Joel Fernandes <joel@joelfernandes.org>
>> Fixes: b0aad7a99c1d ("pstore: Add compression support to pstore")
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Thanks!
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Thanks!

> Also should this be fixed for other backends or are those good? AFAIR, I saw
> this for EFI too.

It seemed like the other backends were doing it correctly (e.g. erst
removes the header from calculation, etc). I did see that EFI
allocates more memory than needed?

        efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
        if (!efi_pstore_info.buf)
                return -ENOMEM;

        efi_pstore_info.bufsize = 1024;

efi_pstore_write() does:

        ret = efivar_entry_set_safe(efi_name, vendor, PSTORE_EFI_ATTRIBUTES,
                              !pstore_cannot_block_path(record->reason),
                              record->size, record->psi->buf);

and efivar_entry_set_safe() says:

 * Returns 0 on success, -ENOSPC if the firmware does not have enough
 * space for set_variable() to succeed, or a converted EFI status code
 * if set_variable() fails.

So I don't see how this could get truncated. (I'm not saying it
didn't... just that I can't see it in an obvious place.)

-Kees

-- 
Kees Cook

Re: [PATCH 8/8] pstore/ram: Correctly calculate usable PRZ bytes

[Joel Fernandes]

Hi Kees,

On Fri, Nov 02, 2018 at 01:00:08PM -0700, Kees Cook wrote:
[..] 
> >> This corruption was visible with "ramoops.mem_size=204800 ramoops.ecc=1".
> >> Any stored crashes would not be uncompressable (producing a pstorefs
> >> "dmesg-*.enc.z" file), and triggering errors at boot:
> >>
> >>   [    2.790759] pstore: crypto_comp_decompress failed, ret = -22!
> >>
> >> Reported-by: Joel Fernandes <joel@joelfernandes.org>
> >> Fixes: b0aad7a99c1d ("pstore: Add compression support to pstore")
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >
> > Thanks!
> > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> 
> Thanks!
> 
> > Also should this be fixed for other backends or are those good? AFAIR, I saw
> > this for EFI too.
> 
> It seemed like the other backends were doing it correctly (e.g. erst
> removes the header from calculation, etc). I did see that EFI
> allocates more memory than needed?
> 
>         efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
>         if (!efi_pstore_info.buf)
>                 return -ENOMEM;
> 
>         efi_pstore_info.bufsize = 1024;
> 
> efi_pstore_write() does:
> 
>         ret = efivar_entry_set_safe(efi_name, vendor, PSTORE_EFI_ATTRIBUTES,
>                               !pstore_cannot_block_path(record->reason),
>                               record->size, record->psi->buf);
> 
> and efivar_entry_set_safe() says:
> 
>  * Returns 0 on success, -ENOSPC if the firmware does not have enough
>  * space for set_variable() to succeed, or a converted EFI status code
>  * if set_variable() fails.
> 
> So I don't see how this could get truncated. (I'm not saying it
> didn't... just that I can't see it in an obvious place.)


So I *think* the issue is that the pstore had old compressed dmesg dumps in
EFI on my laptop, after the crypto layer in the kernel probably changed
enough to make the data non-decompressable, if that makes any sense. So older
code did compression in certain way, and newer code is doing the decompress,
or something like that.

I did some sysrq crashes on my laptop and the deflate decompress is working
fine with pstore+EFI. Its interesting I see some .enc.z files which fail to
decompress (which are older ones), and others which are decompressed fine
(the newer ones) ;-)

Dumping the magic bytes of the non decompressable .enc.z files, I get this
which shows a valid zlib compressed header:

Something like:
48 89 85 54 4d 6f 1a 31

The 0b1000 in the first byte means it is "deflate". The file tool indeed
successfully shows "zlib compressed data" and I did the math for the header
and it is indeed valid. So I don't think the data is insane. The buffer has
enough room because even the very small dumps are not decompressable.

At this point we can park this issue I guess, but a scenario that is still
broken is:
Say someone crashes the system on compress algo X and then recompiles with
compress algo Y, then the decompress would fail no?

One way to fix that is to store the comrpession method in buffer as well,
then initialize all algorithms at boot and choose the right one in the
buffer ideally. Otherwise atleast we should print a message saying "buffer is
encoded with algo X but compression selected is Y" or something. But I agree
its a very low priority "doctor it hurts if I do this" kind of issue :)

Anyway, let me know what you think :)

thanks,

- Joel

Re: [PATCH 8/8] pstore/ram: Correctly calculate usable PRZ bytes

[Kees Cook]

On Sun, Nov 4, 2018 at 8:42 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> Dumping the magic bytes of the non decompressable .enc.z files, I get this
> which shows a valid zlib compressed header:
>
> Something like:
> 48 89 85 54 4d 6f 1a 31
>
> The 0b1000 in the first byte means it is "deflate". The file tool indeed
> successfully shows "zlib compressed data" and I did the math for the header
> and it is indeed valid. So I don't think the data is insane. The buffer has
> enough room because even the very small dumps are not decompressable.

Interesting. So the kernel wouldn't decompress it even though it's the
right algo and untruncated? That seems worth fixing.

> At this point we can park this issue I guess, but a scenario that is still
> broken is:
> Say someone crashes the system on compress algo X and then recompiles with
> compress algo Y, then the decompress would fail no?
>
> One way to fix that is to store the comrpession method in buffer as well,
> then initialize all algorithms at boot and choose the right one in the
> buffer ideally. Otherwise atleast we should print a message saying "buffer is
> encoded with algo X but compression selected is Y" or something. But I agree
> its a very low priority "doctor it hurts if I do this" kind of issue :)

Right, this is fine: if algos change across a kernel version, I'm fine
with it failing. pstore isn't expected to work sanely outside of a
pretty narrow set of use-cases.

-Kees

-- 
Kees Cook

Re: [PATCH 8/8] pstore/ram: Correctly calculate usable PRZ bytes

[Joel Fernandes]

On Mon, Nov 05, 2018 at 09:04:13AM -0800, Kees Cook wrote:
> On Sun, Nov 4, 2018 at 8:42 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> > Dumping the magic bytes of the non decompressable .enc.z files, I get this
> > which shows a valid zlib compressed header:
> >
> > Something like:
> > 48 89 85 54 4d 6f 1a 31
> >
> > The 0b1000 in the first byte means it is "deflate". The file tool indeed
> > successfully shows "zlib compressed data" and I did the math for the header
> > and it is indeed valid. So I don't think the data is insane. The buffer has
> > enough room because even the very small dumps are not decompressable.
> 
> Interesting. So the kernel wouldn't decompress it even though it's the
> right algo and untruncated? That seems worth fixing.

I just can't reproduce the issue though :) I am wondering if it was something
to do with compression being done by an older version of the algorithm or a
buggy algorithm, and the decompressor is newer. Or something.

Not sure if its worth spending more time on, I'll park it for now unless you
want me to dig deeper into it.

- Joel