public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation
@ 2023-06-27 20:25 Yuxiao Zhang
  2023-06-28  5:30 ` Greg KH
  2023-06-28 17:55 ` Kees Cook
  0 siblings, 2 replies; 13+ messages in thread
From: Yuxiao Zhang @ 2023-06-27 20:25 UTC (permalink / raw)
  To: Kees Cook, Tony Luck, 'Guilherme G . Piccoli', Greg KH,
	linux-hardening
  Cc: linux-kernel, wak, Yuxiao Zhang

Current pmsg implementation is using kmalloc for pmsg record buffer,
which has max size limits based on page size. Currently even we
allocate enough space with pmsg-size, pmsg will still fail if the
file size is larger than what kmalloc allowed.

Since we don't need physical contiguous memory for pmsg buffer
, we can use kvmalloc to avoid such limitation.

Signed-off-by: Yuxiao Zhang <yuxiaozhang@google.com>
---
 fs/pstore/inode.c    | 2 +-
 fs/pstore/platform.c | 9 +++++----
 fs/pstore/ram.c      | 5 +++--
 fs/pstore/ram_core.c | 3 ++-
 4 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index ffbadb8b3032..df7fb2ad4599 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -54,7 +54,7 @@ static void free_pstore_private(struct pstore_private *private)
 	if (!private)
 		return;
 	if (private->record) {
-		kfree(private->record->buf);
+		kvfree(private->record->buf);
 		kfree(private->record->priv);
 		kfree(private->record);
 	}
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index cbc0b468c1ab..f51e9460ac9d 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -32,6 +32,7 @@
 #include <linux/uaccess.h>
 #include <linux/jiffies.h>
 #include <linux/workqueue.h>
+#include <linux/mm.h>
 
 #include "internal.h"
 
@@ -549,7 +550,7 @@ static int pstore_write_user_compat(struct pstore_record *record,
 	if (record->buf)
 		return -EINVAL;
 
-	record->buf = memdup_user(buf, record->size);
+	record->buf = vmemdup_user(buf, record->size);
 	if (IS_ERR(record->buf)) {
 		ret = PTR_ERR(record->buf);
 		goto out;
@@ -557,7 +558,7 @@ static int pstore_write_user_compat(struct pstore_record *record,
 
 	ret = record->psi->write(record);
 
-	kfree(record->buf);
+	kvfree(record->buf);
 out:
 	record->buf = NULL;
 
@@ -730,7 +731,7 @@ static void decompress_record(struct pstore_record *record)
 		return;
 
 	/* Swap out compressed contents with decompressed contents. */
-	kfree(record->buf);
+	kvfree(record->buf);
 	record->buf = unzipped;
 	record->size = unzipped_len;
 	record->compressed = false;
@@ -783,7 +784,7 @@ void pstore_get_backend_records(struct pstore_info *psi,
 		rc = pstore_mkfile(root, record);
 		if (rc) {
 			/* pstore_mkfile() did not take record, so free it. */
-			kfree(record->buf);
+			kvfree(record->buf);
 			kfree(record->priv);
 			kfree(record);
 			if (rc != -EEXIST || !quiet)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ade66dbe5f39..296465b14fa9 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -20,6 +20,7 @@
 #include <linux/compiler.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/mm.h>
 
 #include "internal.h"
 #include "ram_internal.h"
@@ -268,7 +269,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
 	/* ECC correction notice */
 	record->ecc_notice_size = persistent_ram_ecc_string(prz, NULL, 0);
 
-	record->buf = kmalloc(size + record->ecc_notice_size + 1, GFP_KERNEL);
+	record->buf = kvmalloc(size + record->ecc_notice_size + 1, GFP_KERNEL);
 	if (record->buf == NULL) {
 		size = -ENOMEM;
 		goto out;
@@ -282,7 +283,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
 
 out:
 	if (free_prz) {
-		kfree(prz->old_log);
+		kvfree(prz->old_log);
 		kfree(prz);
 	}
 
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 966191d3a5ba..3453d493ec27 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/vmalloc.h>
+#include <linux/mm.h>
 #include <asm/page.h>
 
 #include "ram_internal.h"
@@ -385,7 +386,7 @@ void *persistent_ram_old(struct persistent_ram_zone *prz)
 
 void persistent_ram_free_old(struct persistent_ram_zone *prz)
 {
-	kfree(prz->old_log);
+	kvfree(prz->old_log);
 	prz->old_log = NULL;
 	prz->old_log_size = 0;
 }
-- 
2.41.0.162.gfafddb0af9-goog

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

* Re: [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation
  2023-06-27 20:25 [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation Yuxiao Zhang
@ 2023-06-28  5:30 ` Greg KH
  2023-06-28 17:10   ` Yuxiao Zhang
  2023-06-28 17:55 ` Kees Cook
  1 sibling, 1 reply; 13+ messages in thread
From: Greg KH @ 2023-06-28  5:30 UTC (permalink / raw)
  To: Yuxiao Zhang
  Cc: Kees Cook, Tony Luck, 'Guilherme G . Piccoli',
	linux-hardening, linux-kernel, wak

On Tue, Jun 27, 2023 at 01:25:41PM -0700, Yuxiao Zhang wrote:
> Current pmsg implementation is using kmalloc for pmsg record buffer,
> which has max size limits based on page size.

What is that max size?

> Currently even we
> allocate enough space with pmsg-size, pmsg will still fail if the
> file size is larger than what kmalloc allowed.
> 
> Since we don't need physical contiguous memory for pmsg buffer
> , we can use kvmalloc to avoid such limitation.

Odd placement of the ',' character :)

Anyway, thanks for getting this sent out.

But, what in-kernel user is hitting this in the pstore implementation?
How big of a buffer is it trying to create?  Is this a bug in older
kernels with the in-kernel drivers as well?  If so, should it go to
stable releases and how far back?

thanks,

greg k-h

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

* Re: Re: [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation
  2023-06-28  5:30 ` Greg KH
@ 2023-06-28 17:10   ` Yuxiao Zhang
  2023-06-28 18:12     ` Guilherme G. Piccoli
  0 siblings, 1 reply; 13+ messages in thread
From: Yuxiao Zhang @ 2023-06-28 17:10 UTC (permalink / raw)
  To: Greg KH
  Cc: Kees Cook, Tony Luck, 'Guilherme G . Piccoli',
	linux-hardening, linux-kernel, wak

Thanks for reviewing the patch.

On 28 Jun 2023 07:30:16 +0200, Greg KH wrote:
>What is that max size?

The max size is arch dependent, it should be 2^(PAGE_SIZE+MAX_ORDER). In our environment it is 4M.

>what in-kernel user is hitting this in the pstore implementation?

We are trying to use pmsg to hold a core dump file, so we have pmsg-size=32M and thus hit this issue.

Other than us, here is one I found that trying to save dmesg beyond kmalloc limitaton:
https://lore.kernel.org/lkml/b2d66d9f-15a6-415c-2485-44649027a1d5@igalia.com/T/

Thanks,
-Yuxiao Zhang

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

* Re: [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation
  2023-06-27 20:25 [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation Yuxiao Zhang
  2023-06-28  5:30 ` Greg KH
@ 2023-06-28 17:55 ` Kees Cook
  2023-06-30 20:50   ` Yuxiao Zhang
  2023-06-30 20:53   ` Yuxiao Zhang
  1 sibling, 2 replies; 13+ messages in thread
From: Kees Cook @ 2023-06-28 17:55 UTC (permalink / raw)
  To: Yuxiao Zhang
  Cc: Tony Luck, 'Guilherme G . Piccoli', Greg KH,
	linux-hardening, linux-kernel, wak

On Tue, Jun 27, 2023 at 01:25:41PM -0700, Yuxiao Zhang wrote:
> Current pmsg implementation is using kmalloc for pmsg record buffer,
> which has max size limits based on page size. Currently even we
> allocate enough space with pmsg-size, pmsg will still fail if the
> file size is larger than what kmalloc allowed.
> 
> Since we don't need physical contiguous memory for pmsg buffer
> , we can use kvmalloc to avoid such limitation.

Conceptually, I am fine with this change. I need a little time to trace
down the allocations. At first glance, I thought this patch only needed
to cover pstore_write_user_compat(), but I guess the read side needs to
be adjusted as well?

I'll double-check.

And yes, Greg's questions are all good -- fixing syntax and adding size
details in the commit log would be appreciated.

-Kees

-- 
Kees Cook

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

* Re: [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation
  2023-06-28 17:10   ` Yuxiao Zhang
@ 2023-06-28 18:12     ` Guilherme G. Piccoli
  2023-06-28 23:24       ` Kees Cook
  0 siblings, 1 reply; 13+ messages in thread
From: Guilherme G. Piccoli @ 2023-06-28 18:12 UTC (permalink / raw)
  To: Yuxiao Zhang
  Cc: Kees Cook, Greg KH, Tony Luck, linux-hardening, linux-kernel, wak

On 28/06/2023 19:10, Yuxiao Zhang wrote:
> Thanks for reviewing the patch.
> 
> On 28 Jun 2023 07:30:16 +0200, Greg KH wrote:
>> What is that max size?
> 
> The max size is arch dependent, it should be 2^(PAGE_SIZE+MAX_ORDER). In our environment it is 4M.
> 
>> what in-kernel user is hitting this in the pstore implementation?
> 
> We are trying to use pmsg to hold a core dump file, so we have pmsg-size=32M and thus hit this issue.
> 
> Other than us, here is one I found that trying to save dmesg beyond kmalloc limitaton:
> https://lore.kernel.org/lkml/b2d66d9f-15a6-415c-2485-44649027a1d5@igalia.com/T/

Hi Yuxiao Zhang, thanks for the improvement! And also, thanks for
mentioning the thread above - I tested your patch today and was soon to
write you this message heh

So, first of all, the patch works for the Steam Deck case - kernel 6.4
with or without your patch behaved the same, i.e., pstore/ramoops worked
and it was possible to collect the dmesg.

But when I tried to increase the record size in ramoops, I got this
error: https://termbin.com/b12e

This is the same error as mentioned in the thread above. And it happens
when I try to bump the record size to 4MB, the biggest working value is
still 2MB. Maybe a missing spot, which remained using kmalloc() instead
of the virtual variant?

Also - Kees certainly knows that way better, but - are we 100% sure that
the region for pstore can be non-contiguous? For some reason, I always
thought this was a requirement - grepping the code, I found this
(wrong?) comment:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/pstore/zone.c#n3

Cheers,


Guilherme

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

* Re: [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation
  2022-01-04 19:36 pstore/ramoops - why only collect a partial dmesg? Guilherme G. Piccoli
@ 2023-06-28 18:48 ` Yuxiao Zhang
  2023-06-28 19:05   ` Guilherme G. Piccoli
  0 siblings, 1 reply; 13+ messages in thread
From: Yuxiao Zhang @ 2023-06-28 18:48 UTC (permalink / raw)
  To: Kees Cook, 'Guilherme G . Piccoli'
  Cc: Tony Luck, Greg KH, linux-hardening, linux-kernel, wak

Thanks for the review Kees.

>And yes, Greg's questions are all good -- fixing syntax and adding size
details in the commit log would be appreciated.

Sure, will send another patch to address this.

Hi Guilherme, thanks for testing this patch.

>But when I tried to increase the record size in ramoops, I got this error: https://termbin.com/b12e

What record type are you testing? I should have mention that this patch is only for pmsg use case on ramoops (I mentioned it in the original thread but need to start a new one due to format issue).

>Also - Kees certainly knows that way better, but - are we 100% sure that
the region for pstore can be non-contiguous? For some reason, I always
thought this was a requirement

Right, that is why this patch only touches the pstore ram path, I am not sure how things will go if it is block device backed.

Thanks,
-Yuxiao Zhang

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

* Re: [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation
  2023-06-28 18:48 ` [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation Yuxiao Zhang
@ 2023-06-28 19:05   ` Guilherme G. Piccoli
  0 siblings, 0 replies; 13+ messages in thread
From: Guilherme G. Piccoli @ 2023-06-28 19:05 UTC (permalink / raw)
  To: Yuxiao Zhang, Kees Cook
  Cc: Tony Luck, Greg KH, linux-hardening, linux-kernel, wak

On 28/06/2023 20:48, Yuxiao Zhang wrote:
> [...]
> What record type are you testing? I should have mention that this patch is only for pmsg use case on ramoops (I mentioned it in the original thread but need to start a new one due to format issue).

Oh, my bad! I'm collecting oops log, so the change is not really related
to my case. It is in the patch title, so my fault not reading that
properly heheh


> 
> Right, that is why this patch only touches the pstore ram path, I am not sure how things will go if it is block device backed.

It makes sense!
Thanks,


Guilherme

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

* Re: [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation
  2023-06-28 18:12     ` Guilherme G. Piccoli
@ 2023-06-28 23:24       ` Kees Cook
  2023-06-29 19:22         ` Guilherme G. Piccoli
  0 siblings, 1 reply; 13+ messages in thread
From: Kees Cook @ 2023-06-28 23:24 UTC (permalink / raw)
  To: Guilherme G. Piccoli
  Cc: Yuxiao Zhang, Greg KH, Tony Luck, linux-hardening, linux-kernel,
	wak

On Wed, Jun 28, 2023 at 03:12:10PM -0300, Guilherme G. Piccoli wrote:
> Also - Kees certainly knows that way better, but - are we 100% sure that
> the region for pstore can be non-contiguous? For some reason, I always
> thought this was a requirement - grepping the code, I found this
> (wrong?) comment:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/pstore/zone.c#n3

The contiguous requirements are entirely for the RAM backend's storage,
so these intermediate memory regions can use non-contiguous physical
backing memory (i.e. vmalloc).

Even the special case of crash-dumping should be fine for the large
buffer used to hold the crash before doing compression.

There are effectively 4 types of allocations in pstore:

1- a physical -> virtual mapping for the RAM backend
2- the allocations (if any) for non-RAM backends to hold a copy of pstore
   records when extracted from the backend storage (e.g NVRAM, EFI vars,
   etc).
3- incoming allocations that are used temporarily to hand data to the
   backend (e.g. the write_compat used in this patch)
4- the allocation used for holding the pstorefs data contents (which I
   need to double-check, but is entirely defined by the backends)

Changes aren't needed for (1), it's fine on its own. This patch changes
allocations for (2) and (3) for pmsg and the RAM backend, if I'm reading
it correctly. I think (4) is included as part of (2), but I need to
check. As long as all paths use kvfree() for the record buffers,
everything should Just Work for RAM. Switching the other backends to
also use kvalloc() would allow for greater flexibility, though.

Anyway, it's on my list to review and test. I'm still working through
other things related to the merge window opening, so I may be a bit slow
for the next week. :)

-Kees

-- 
Kees Cook

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

* Re: [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation
  2023-06-28 23:24       ` Kees Cook
@ 2023-06-29 19:22         ` Guilherme G. Piccoli
  0 siblings, 0 replies; 13+ messages in thread
From: Guilherme G. Piccoli @ 2023-06-29 19:22 UTC (permalink / raw)
  To: Kees Cook
  Cc: Yuxiao Zhang, Greg KH, Tony Luck, linux-hardening, linux-kernel,
	wak

On 28/06/2023 20:24, Kees Cook wrote:
> On Wed, Jun 28, 2023 at 03:12:10PM -0300, Guilherme G. Piccoli wrote:
>> Also - Kees certainly knows that way better, but - are we 100% sure that
>> the region for pstore can be non-contiguous? For some reason, I always
>> thought this was a requirement - grepping the code, I found this
>> (wrong?) comment:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/pstore/zone.c#n3
> 
> The contiguous requirements are entirely for the RAM backend's storage,
> so these intermediate memory regions can use non-contiguous physical
> backing memory (i.e. vmalloc).
> 
> Even the special case of crash-dumping should be fine for the large
> buffer used to hold the crash before doing compression.
> 
> There are effectively 4 types of allocations in pstore:
> 
> 1- a physical -> virtual mapping for the RAM backend
> 2- the allocations (if any) for non-RAM backends to hold a copy of pstore
>    records when extracted from the backend storage (e.g NVRAM, EFI vars,
>    etc).
> 3- incoming allocations that are used temporarily to hand data to the
>    backend (e.g. the write_compat used in this patch)
> 4- the allocation used for holding the pstorefs data contents (which I
>    need to double-check, but is entirely defined by the backends)
> 
> Changes aren't needed for (1), it's fine on its own. This patch changes
> allocations for (2) and (3) for pmsg and the RAM backend, if I'm reading
> it correctly. I think (4) is included as part of (2), but I need to
> check. As long as all paths use kvfree() for the record buffers,
> everything should Just Work for RAM. Switching the other backends to
> also use kvalloc() would allow for greater flexibility, though.
> 
> Anyway, it's on my list to review and test. I'm still working through
> other things related to the merge window opening, so I may be a bit slow
> for the next week. :)
> 
> -Kees
> 

Thanks a bunch for the clarification Kees, much appreciated!
Now I understand it way better =)

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

* Re: [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation
  2023-06-28 17:55 ` Kees Cook
@ 2023-06-30 20:50   ` Yuxiao Zhang
  2023-06-30 20:53   ` Yuxiao Zhang
  1 sibling, 0 replies; 13+ messages in thread
From: Yuxiao Zhang @ 2023-06-30 20:50 UTC (permalink / raw)
  To: Kees Cook
  Cc: Tony Luck, 'Guilherme G . Piccoli', Greg KH,
	linux-hardening, linux-kernel, wak, Added, size, details, to,
	commit, message, and, fixed, the, format.See, new, Yuxiao Zhang

Thanks,
-Yuxiao


From cd3ec6155a3cf0e198afdf2d040c73ee146b696f Mon Sep 17 00:00:00 2001
From: Yuxiao Zhang <yuxiaozhang@google.com>
Date: Fri, 30 Jun 2023 10:45:21 -0700
Subject: [PATCH] pstore: ramoops: support pmsg size larger than kmalloc
 limitation

Current pmsg implementation is using kmalloc for pmsg record buffer,
which has max size limits of 2^(MAX_ORDER + PAGE_SHIFT). Currently even
we allocate enough space with pmsg-size, pmsg will still fail if the
file size is larger than what kmalloc allowed.

Since we don't need physical contiguous memory for pmsg buffer,
we can use kvmalloc to avoid such limitation.

Signed-off-by: Yuxiao Zhang <yuxiaozhang@google.com>
---
 fs/pstore/inode.c    | 2 +-
 fs/pstore/platform.c | 9 +++++----
 fs/pstore/ram.c      | 5 +++--
 fs/pstore/ram_core.c | 3 ++-
 4 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index ffbadb8b3032..df7fb2ad4599 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -54,7 +54,7 @@ static void free_pstore_private(struct pstore_private *private)
 	if (!private)
 		return;
 	if (private->record) {
-		kfree(private->record->buf);
+		kvfree(private->record->buf);
 		kfree(private->record->priv);
 		kfree(private->record);
 	}
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index cbc0b468c1ab..f51e9460ac9d 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -32,6 +32,7 @@
 #include <linux/uaccess.h>
 #include <linux/jiffies.h>
 #include <linux/workqueue.h>
+#include <linux/mm.h>
 
 #include "internal.h"
 
@@ -549,7 +550,7 @@ static int pstore_write_user_compat(struct pstore_record *record,
 	if (record->buf)
 		return -EINVAL;
 
-	record->buf = memdup_user(buf, record->size);
+	record->buf = vmemdup_user(buf, record->size);
 	if (IS_ERR(record->buf)) {
 		ret = PTR_ERR(record->buf);
 		goto out;
@@ -557,7 +558,7 @@ static int pstore_write_user_compat(struct pstore_record *record,
 
 	ret = record->psi->write(record);
 
-	kfree(record->buf);
+	kvfree(record->buf);
 out:
 	record->buf = NULL;
 
@@ -730,7 +731,7 @@ static void decompress_record(struct pstore_record *record)
 		return;
 
 	/* Swap out compressed contents with decompressed contents. */
-	kfree(record->buf);
+	kvfree(record->buf);
 	record->buf = unzipped;
 	record->size = unzipped_len;
 	record->compressed = false;
@@ -783,7 +784,7 @@ void pstore_get_backend_records(struct pstore_info *psi,
 		rc = pstore_mkfile(root, record);
 		if (rc) {
 			/* pstore_mkfile() did not take record, so free it. */
-			kfree(record->buf);
+			kvfree(record->buf);
 			kfree(record->priv);
 			kfree(record);
 			if (rc != -EEXIST || !quiet)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ade66dbe5f39..296465b14fa9 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -20,6 +20,7 @@
 #include <linux/compiler.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/mm.h>
 
 #include "internal.h"
 #include "ram_internal.h"
@@ -268,7 +269,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
 	/* ECC correction notice */
 	record->ecc_notice_size = persistent_ram_ecc_string(prz, NULL, 0);
 
-	record->buf = kmalloc(size + record->ecc_notice_size + 1, GFP_KERNEL);
+	record->buf = kvmalloc(size + record->ecc_notice_size + 1, GFP_KERNEL);
 	if (record->buf == NULL) {
 		size = -ENOMEM;
 		goto out;
@@ -282,7 +283,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
 
 out:
 	if (free_prz) {
-		kfree(prz->old_log);
+		kvfree(prz->old_log);
 		kfree(prz);
 	}
 
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 966191d3a5ba..3453d493ec27 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/vmalloc.h>
+#include <linux/mm.h>
 #include <asm/page.h>
 
 #include "ram_internal.h"
@@ -385,7 +386,7 @@ void *persistent_ram_old(struct persistent_ram_zone *prz)
 
 void persistent_ram_free_old(struct persistent_ram_zone *prz)
 {
-	kfree(prz->old_log);
+	kvfree(prz->old_log);
 	prz->old_log = NULL;
 	prz->old_log_size = 0;
 }
-- 
2.41.0.255.g8b1d071c50-goog


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

* Re: [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation
  2023-06-28 17:55 ` Kees Cook
  2023-06-30 20:50   ` Yuxiao Zhang
@ 2023-06-30 20:53   ` Yuxiao Zhang
  2023-07-18 20:23     ` Yuxiao Zhang
  1 sibling, 1 reply; 13+ messages in thread
From: Yuxiao Zhang @ 2023-06-30 20:53 UTC (permalink / raw)
  To: Kees Cook
  Cc: Tony Luck, 'Guilherme G . Piccoli', Greg KH,
	linux-hardening, linux-kernel, wak, Yuxiao Zhang

Sorry forgot to add subject header in msg which messed up email client,
resending it again

Added size details to commit message and fixed the format. See the new
patch below.

Thanks,
-Yuxiao


From cd3ec6155a3cf0e198afdf2d040c73ee146b696f Mon Sep 17 00:00:00 2001
From: Yuxiao Zhang <yuxiaozhang@google.com>
Date: Fri, 30 Jun 2023 10:45:21 -0700
Subject: [PATCH] pstore: ramoops: support pmsg size larger than kmalloc
 limitation

Current pmsg implementation is using kmalloc for pmsg record buffer,
which has max size limits of 2^(MAX_ORDER + PAGE_SHIFT). Currently even
we allocate enough space with pmsg-size, pmsg will still fail if the
file size is larger than what kmalloc allowed.

Since we don't need physical contiguous memory for pmsg buffer,
we can use kvmalloc to avoid such limitation.

Signed-off-by: Yuxiao Zhang <yuxiaozhang@google.com>
---
 fs/pstore/inode.c    | 2 +-
 fs/pstore/platform.c | 9 +++++----
 fs/pstore/ram.c      | 5 +++--
 fs/pstore/ram_core.c | 3 ++-
 4 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index ffbadb8b3032..df7fb2ad4599 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -54,7 +54,7 @@ static void free_pstore_private(struct pstore_private *private)
 	if (!private)
 		return;
 	if (private->record) {
-		kfree(private->record->buf);
+		kvfree(private->record->buf);
 		kfree(private->record->priv);
 		kfree(private->record);
 	}
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index cbc0b468c1ab..f51e9460ac9d 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -32,6 +32,7 @@
 #include <linux/uaccess.h>
 #include <linux/jiffies.h>
 #include <linux/workqueue.h>
+#include <linux/mm.h>
 
 #include "internal.h"
 
@@ -549,7 +550,7 @@ static int pstore_write_user_compat(struct pstore_record *record,
 	if (record->buf)
 		return -EINVAL;
 
-	record->buf = memdup_user(buf, record->size);
+	record->buf = vmemdup_user(buf, record->size);
 	if (IS_ERR(record->buf)) {
 		ret = PTR_ERR(record->buf);
 		goto out;
@@ -557,7 +558,7 @@ static int pstore_write_user_compat(struct pstore_record *record,
 
 	ret = record->psi->write(record);
 
-	kfree(record->buf);
+	kvfree(record->buf);
 out:
 	record->buf = NULL;
 
@@ -730,7 +731,7 @@ static void decompress_record(struct pstore_record *record)
 		return;
 
 	/* Swap out compressed contents with decompressed contents. */
-	kfree(record->buf);
+	kvfree(record->buf);
 	record->buf = unzipped;
 	record->size = unzipped_len;
 	record->compressed = false;
@@ -783,7 +784,7 @@ void pstore_get_backend_records(struct pstore_info *psi,
 		rc = pstore_mkfile(root, record);
 		if (rc) {
 			/* pstore_mkfile() did not take record, so free it. */
-			kfree(record->buf);
+			kvfree(record->buf);
 			kfree(record->priv);
 			kfree(record);
 			if (rc != -EEXIST || !quiet)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ade66dbe5f39..296465b14fa9 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -20,6 +20,7 @@
 #include <linux/compiler.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/mm.h>
 
 #include "internal.h"
 #include "ram_internal.h"
@@ -268,7 +269,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
 	/* ECC correction notice */
 	record->ecc_notice_size = persistent_ram_ecc_string(prz, NULL, 0);
 
-	record->buf = kmalloc(size + record->ecc_notice_size + 1, GFP_KERNEL);
+	record->buf = kvmalloc(size + record->ecc_notice_size + 1, GFP_KERNEL);
 	if (record->buf == NULL) {
 		size = -ENOMEM;
 		goto out;
@@ -282,7 +283,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
 
 out:
 	if (free_prz) {
-		kfree(prz->old_log);
+		kvfree(prz->old_log);
 		kfree(prz);
 	}
 
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 966191d3a5ba..3453d493ec27 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/vmalloc.h>
+#include <linux/mm.h>
 #include <asm/page.h>
 
 #include "ram_internal.h"
@@ -385,7 +386,7 @@ void *persistent_ram_old(struct persistent_ram_zone *prz)
 
 void persistent_ram_free_old(struct persistent_ram_zone *prz)
 {
-	kfree(prz->old_log);
+	kvfree(prz->old_log);
 	prz->old_log = NULL;
 	prz->old_log_size = 0;
 }
-- 
2.41.0.255.g8b1d071c50-goog


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

* Re: [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation
  2023-06-30 20:53   ` Yuxiao Zhang
@ 2023-07-18 20:23     ` Yuxiao Zhang
  2023-08-17 23:40       ` Kees Cook
  0 siblings, 1 reply; 13+ messages in thread
From: Yuxiao Zhang @ 2023-07-18 20:23 UTC (permalink / raw)
  To: Kees Cook
  Cc: Tony Luck, 'Guilherme G . Piccoli', Greg KH,
	linux-hardening, linux-kernel, wak

Friendly ping, any update on this?

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

* Re: [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation
  2023-07-18 20:23     ` Yuxiao Zhang
@ 2023-08-17 23:40       ` Kees Cook
  0 siblings, 0 replies; 13+ messages in thread
From: Kees Cook @ 2023-08-17 23:40 UTC (permalink / raw)
  To: Yuxiao Zhang
  Cc: Tony Luck, 'Guilherme G . Piccoli', Greg KH,
	linux-hardening, linux-kernel, wak

On Tue, Jul 18, 2023 at 01:23:47PM -0700, Yuxiao Zhang wrote:
> Friendly ping, any update on this?

Hi! I finally got a chance to look this over. I added a few more
kvzalloc() uses to generalize this for all records (not just pmsg), and
it's testing well. Here's the resulting commit:

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/pstore&id=104fd0b5e948157f8e8ac88a20b46ba8641d4e95

-- 
Kees Cook

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

end of thread, other threads:[~2023-08-17 23:41 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-27 20:25 [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation Yuxiao Zhang
2023-06-28  5:30 ` Greg KH
2023-06-28 17:10   ` Yuxiao Zhang
2023-06-28 18:12     ` Guilherme G. Piccoli
2023-06-28 23:24       ` Kees Cook
2023-06-29 19:22         ` Guilherme G. Piccoli
2023-06-28 17:55 ` Kees Cook
2023-06-30 20:50   ` Yuxiao Zhang
2023-06-30 20:53   ` Yuxiao Zhang
2023-07-18 20:23     ` Yuxiao Zhang
2023-08-17 23:40       ` Kees Cook
  -- strict thread matches above, loose matches on Subject: below --
2022-01-04 19:36 pstore/ramoops - why only collect a partial dmesg? Guilherme G. Piccoli
2023-06-28 18:48 ` [PATCH] pstore: ramoops: support pmsg size larger than kmalloc limitation Yuxiao Zhang
2023-06-28 19:05   ` 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