diff mbox series

[POC,RFC,2/2] pstore/ramoops: Add ramoops.mem_name= command line option

Message ID 20240409211351.234897475@goodmis.org (mailing list archive)
State Rejected
Headers show
Series pstore/mm/x86: Add wildcard memmap to map pstore consistently | expand

Commit Message

Steven Rostedt April 9, 2024, 9:02 p.m. UTC
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Add a method to find a region specified by memmap=nn*align:name for
ramoops. Adding a kernel command line parameter:

  memmap=12M*4096:oops ramoops.mem_name=oops

Will use the size and location defined by the memmap parameter where it
finds the memory and labels it "oops". The "oops" in the ramoops option
is used to search for it.

This allows for arbitrary RAM to be used for ramoops if it is known that
the memory is not cleared on kernel crashes or soft reboots.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 fs/pstore/ram.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Kees Cook April 9, 2024, 10:18 p.m. UTC | #1
On Tue, Apr 09, 2024 at 05:02:56PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Add a method to find a region specified by memmap=nn*align:name for
> ramoops. Adding a kernel command line parameter:
> 
>   memmap=12M*4096:oops ramoops.mem_name=oops
> 
> Will use the size and location defined by the memmap parameter where it
> finds the memory and labels it "oops". The "oops" in the ramoops option
> is used to search for it.
> 
> This allows for arbitrary RAM to be used for ramoops if it is known that
> the memory is not cleared on kernel crashes or soft reboots.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  fs/pstore/ram.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index b1a455f42e93..c200388399fb 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -50,6 +50,11 @@ module_param_hw(mem_address, ullong, other, 0400);
>  MODULE_PARM_DESC(mem_address,
>  		"start of reserved RAM used to store oops/panic logs");
>  
> +static char *mem_name;
> +module_param_named(mem_name, mem_name, charp, 0400);
> +MODULE_PARM_DESC(mem_name,
> +		"name of kernel param that holds addr (builtin only)");
> +
>  static ulong mem_size;
>  module_param(mem_size, ulong, 0400);
>  MODULE_PARM_DESC(mem_size,
> @@ -914,6 +919,19 @@ static void __init ramoops_register_dummy(void)
>  {
>  	struct ramoops_platform_data pdata;
>  
> +#ifndef MODULE
> +	/* Only allowed when builtin */

Why only when builtin?

> +	if (mem_name) {
> +		u64 start;
> +		u64 size;
> +
> +		if (memmap_named(mem_name, &start, &size)) {
> +			mem_address = start;
> +			mem_size = size;
> +		}
> +	}
> +#endif

Otherwise this looks good, though I'd prefer some comments about what's
happening here.

(And in retrospect, separately, I probably need to rename "dummy" to
"commandline" or something, since it's gathering valid settings here...)
Steven Rostedt April 9, 2024, 11:14 p.m. UTC | #2
On Tue, 9 Apr 2024 15:18:45 -0700
Kees Cook <keescook@chromium.org> wrote:

> > @@ -914,6 +919,19 @@ static void __init ramoops_register_dummy(void)
> >  {
> >  	struct ramoops_platform_data pdata;
> >  
> > +#ifndef MODULE
> > +	/* Only allowed when builtin */  
> 
> Why only when builtin?

Well, because the memory table that maps the found physical memory to a
lable is marked as __initdata, and will not be available after boot. If you
wanted it for a module, you would need some builtin code to find it.

> 
> > +	if (mem_name) {
> > +		u64 start;
> > +		u64 size;
> > +
> > +		if (memmap_named(mem_name, &start, &size)) {
> > +			mem_address = start;
> > +			mem_size = size;
> > +		}
> > +	}
> > +#endif  
> 
> Otherwise this looks good, though I'd prefer some comments about what's
> happening here.
> 
> (And in retrospect, separately, I probably need to rename "dummy" to
> "commandline" or something, since it's gathering valid settings here...)

Yeah, that was a bit confusing. I kept thinking "is this function stable?".

-- Steve
diff mbox series

Patch

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index b1a455f42e93..c200388399fb 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -50,6 +50,11 @@  module_param_hw(mem_address, ullong, other, 0400);
 MODULE_PARM_DESC(mem_address,
 		"start of reserved RAM used to store oops/panic logs");
 
+static char *mem_name;
+module_param_named(mem_name, mem_name, charp, 0400);
+MODULE_PARM_DESC(mem_name,
+		"name of kernel param that holds addr (builtin only)");
+
 static ulong mem_size;
 module_param(mem_size, ulong, 0400);
 MODULE_PARM_DESC(mem_size,
@@ -914,6 +919,19 @@  static void __init ramoops_register_dummy(void)
 {
 	struct ramoops_platform_data pdata;
 
+#ifndef MODULE
+	/* Only allowed when builtin */
+	if (mem_name) {
+		u64 start;
+		u64 size;
+
+		if (memmap_named(mem_name, &start, &size)) {
+			mem_address = start;
+			mem_size = size;
+		}
+	}
+#endif
+
 	/*
 	 * Prepare a dummy platform data structure to carry the module
 	 * parameters. If mem_size isn't set, then there are no module