[PATCH 0/1] pstore/ram: Ensure stable pmsg address with per-CPU ftrace buffers

[PATCH 0/1] pstore/ram: Ensure stable pmsg address with per-CPU ftrace buffers

[pso]

From: Paramjit Oberoi <pso@chromium.org>


Hi pstore maintainers,

Resending a little patch from May: without this change, the start
address of pmsg varies with the number of CPUs in the system. This is a
problem for tools that want to manipulate pstore outside a VM (we are
doing this for crash reporting).



Paramjit Oberoi (1):
  pstore/ram: Ensure stable pmsg address with per-CPU ftrace buffers

 fs/pstore/ram.c | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.38.0.rc1.362.ged0d419d3c-goog

[PATCH 1/1] pstore/ram: Ensure stable pmsg address with per-CPU ftrace buffers

[pso]

From: Paramjit Oberoi <psoberoi@google.com>

When allocating ftrace pstore zones, there may be space left over at the
end of the region. The paddr pointer needs to be advanced to account for
this so that the next region (pmsg) ends up at the correct location.

Signed-off-by: Paramjit Oberoi <pso@chromium.org>
Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Paramjit Oberoi <psoberoi@google.com>
---

 fs/pstore/ram.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index fefe3d391d3af..3bca6cd34c02a 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -554,10 +554,12 @@ static int ramoops_init_przs(const char *name,
 			goto fail;
 		}
 		*paddr += zone_sz;
+		mem_sz -= zone_sz;
 		prz_ar[i]->type = pstore_name_to_type(name);
 	}
 
 	*przs = prz_ar;
+	*paddr += mem_sz;
 	return 0;
 
 fail:
-- 
2.38.0.rc1.362.ged0d419d3c-goog

Re: [PATCH 1/1] pstore/ram: Ensure stable pmsg address with per-CPU ftrace buffers

[Kees Cook]

On Tue, Oct 11, 2022 at 11:36:31AM -0700, pso@chromium.org wrote:
> From: Paramjit Oberoi <psoberoi@google.com>
> 
> When allocating ftrace pstore zones, there may be space left over at the
> end of the region. The paddr pointer needs to be advanced to account for
> this so that the next region (pmsg) ends up at the correct location.
> 
> Signed-off-by: Paramjit Oberoi <pso@chromium.org>
> Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
> Signed-off-by: Paramjit Oberoi <psoberoi@google.com>

Hm, interesting point. Since only ftrace is dynamically sized in this
fashion, how about just moving the pmsg allocation before ftrace, and
adding a comment that for now ftrace should be allocated last?

i.e. something like:

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 650f89c8ae36..9e11d3e7dffe 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -788,6 +788,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;
@@ -799,11 +804,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.

(Note that this won't apply to the current tree, where I've started some
other refactoring.)

-- 
Kees Cook

Re: [PATCH 1/1] pstore/ram: Ensure stable pmsg address with per-CPU ftrace buffers

[Kees Cook]

On Tue, Oct 11, 2022 at 12:59:50PM -0700, Paramjit Oberoi wrote:
> > Hm, interesting point. Since only ftrace is dynamically sized in this
> > fashion, how about just moving the pmsg allocation before ftrace, and
> > adding a comment that for now ftrace should be allocated last?
> 
> That is a good idea, and it would solve the problem.
> 
> The only downside is it would break some code that works today because it
> ran in contexts where the pmsg address was stable (no per-cpu ftrace
> buffers, or power-of-two CPUs).

I don't follow? And actually, I wonder about the original patch now --
nothing should care about the actual addresses. Everything should be
coming out of the pstore filesystem.

-- 
Kees Cook

Re: [PATCH 1/1] pstore/ram: Ensure stable pmsg address with per-CPU ftrace buffers

[Kees Cook]

On Tue, Oct 11, 2022 at 01:44:54PM -0700, Paramjit Oberoi wrote:
> > > The only downside is it would break some code that works today because
> it
> > > ran in contexts where the pmsg address was stable (no per-cpu ftrace
> > > buffers, or power-of-two CPUs).
> >
> > I don't follow? And actually, I wonder about the original patch now --
> > nothing should care about the actual addresses. Everything should be
> > coming out of the pstore filesystem.
> 
> We are running VMs with the pstore RAM mapped to a file, and using some
> tools outside the VM to read/manipulate the pstore after VM shutdown.

Ah-ha! Interesting. Well, I think it will be more stable this way even
for that. :)

-- 
Kees Cook