pstore: Remove duplicate invoking of persistent_ram_zap()

PATCH In review Peng15 Wang 王鹏 2 participants 4 messages Oct 26, 2018 – Oct 28, 2018 Pstore filesystem

lore

Share & tools

lore

Peng15 Wang 王鹏

author

2018-10-26 03:45 UTC #0 lore raw

PATCH 1/1 pstore: Remove duplicate invoking of persistent_ram_zap()

From: Peng Wang <[email protected]>

When initialing przs with invalid data in buffer(no PERSISTENT_RAM_SIG), function call path is like this:

ramoops_init_prz ->

|
|-> persistent_ram_new -> persistent_ram_post_init -> persistent_ram_zap
|
|--> persistent_ram_zap

As we can see, persistent_ram_zap() is called twice. We can avoid this by removing it in ramoops_init_prz(), and only call it in persistent_ram_post_init().

Signed-off-by Peng Wang <[email protected]>

---
 fs/pstore/ram.c      | 2 --
 fs/pstore/ram_core.c | 3 +--
 2 files changed, 1 insertion(+), 4 deletions(-)

fs/pstore/ram.c +0 -2

index ffcff6516e89..3f57074a4fab 100644
@@ -649,8 +649,6 @@ static int ramoops_init_prz(const char *name,
 		return err;
 	}
 
-	persistent_ram_zap(*prz);
-
 	*paddr += sz;
 
 	return 0;

fs/pstore/ram_core.c +1 -2

index 12e21f789194..3ef0fd5a7cd5 100644
@@ -505,15 +505,14 @@ static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig,
 			pr_debug("found existing buffer, size %zu, start %zu\n",
 				 buffer_size(prz), buffer_start(prz));
 			persistent_ram_save_old(prz);
-			return 0;
 		}
 	} else {
 		pr_debug("no valid data in buffer (sig = 0x%08x)\n",
 			 prz->buffer->sig);
+		prz->buffer->sig = sig;
 	}
 
 	/* Rewind missing or invalid memory area. */
-	prz->buffer->sig = sig;
 	persistent_ram_zap(prz);
 
 	return 0;

Signature · 1 line

2.19.1

Kees Cook

2018-10-26 09:44 UTC #1 lore raw

Quoted 10 lines · On Fri, Oct 26, 2018 at 4:41 AM, Peng15 Wang 王鹏 <[email protected]> wrote:

From: Peng Wang <[email protected]>
When initialing przs with invalid data in buffer(no PERSISTENT_RAM_SIG), function call path is like this:
ramoops_init_prz ->
|
|-> persistent_ram_new -> persistent_ram_post_init -> persistent_ram_zap
|
|--> persistent_ram_zap

There does appear to be a duplicate call to persistent_ram_zap() in this case.

Quoted 3 lines

As we can see, persistent_ram_zap() is called twice. We can avoid this by removing it in ramoops_init_prz(), and only call it in persistent_ram_post_init().

However, I think the proposed fix doesn't work the way it should.

There are two prz init paths: ramoops_init_prz() (a single prz) and ramoops_init_przs (multiple przs). The "dump" and "ftrace" cases use the latter. In those, there is no call to persistent_ram_zap() if the buffer is valid.

In other words:

ramoops_init_prz() unconditionally calls persistent_ram_zap(). (And may call it twice if there is a mismatch of the magic header.)

ramoops_init_przs() only calls persistent_ram_zap() when the magic header is wrong.

The proposed patch unconditionally zaps all regions, which means we'd lose "dump" and "ftrace" across the next reboot.

Perhaps we could make it an option to persistent_ram_new()?

Signature · 1 line

Kees Cook

Peng15 Wang 王鹏

author

2018-10-27 08:53 UTC #2 lore raw

________________________________________

Quoted 39 lines

From: Kees Cook <[email protected]>
Sent: Friday, October 26, 2018 17:44
To: Peng15 Wang 王鹏
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] pstore: Remove duplicate invoking of persistent_ram_zap()
Quoted 10 lines · On Fri, Oct 26, 2018 at 4:41 AM, Peng15 Wang 王鹏 <[email protected]> wrote:
From: Peng Wang <[email protected]>
When initialing przs with invalid data in buffer(no PERSISTENT_RAM_SIG), function call path is like this:
ramoops_init_prz ->
|
|-> persistent_ram_new -> persistent_ram_post_init -> persistent_ram_zap
|
|--> persistent_ram_zap
There does appear to be a duplicate call to persistent_ram_zap() in this case.
Quoted 3 lines
As we can see, persistent_ram_zap() is called twice. We can avoid this by removing it in ramoops_init_prz(), and only call it in persistent_ram_post_init().
However, I think the proposed fix doesn't work the way it should.
There are two prz init paths: ramoops_init_prz() (a single prz) and ramoops_init_przs (multiple przs). The "dump" and "ftrace" cases use the latter. In those, there is no call to persistent_ram_zap() if the buffer is valid.
In other words:
ramoops_init_prz() unconditionally calls persistent_ram_zap(). (And may call it twice if there is a mismatch of the magic header.)
ramoops_init_przs() only calls persistent_ram_zap() when the magic header is wrong.

Quoted 7 lines

The proposed patch unconditionally zaps all regions, which means we'd lose "dump" and "ftrace" across the next reboot.
Perhaps we could make it an option to persistent_ram_new()?
--
Kees Cook

Thanks for your reply.

You are right, this patch does zap regions unconditionally when it comes to "dump" and "ftrace". Sorry for the inconvenience owing to my previous mistake.

I have tried adding an option to persistent_ram_new() according to your advice and will send a V2 version patch later. Could you please kindly pay any attention to it? Thank you!

Kees Cook

2018-10-28 15:46 UTC #3 lore raw

Quoted 47 lines · On Sat, Oct 27, 2018 at 9:52 AM, Peng15 Wang 王鹏 <[email protected]> wrote:

________________________________________
Quoted 44 lines
From: Kees Cook <[email protected]>
Sent: Friday, October 26, 2018 17:44
To: Peng15 Wang 王鹏
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] pstore: Remove duplicate invoking of persistent_ram_zap()
Quoted 10 lines · On Fri, Oct 26, 2018 at 4:41 AM, Peng15 Wang 王鹏 <[email protected]> wrote:
From: Peng Wang <[email protected]>
When initialing przs with invalid data in buffer(no PERSISTENT_RAM_SIG), function call path is like this:
ramoops_init_prz ->
|
|-> persistent_ram_new -> persistent_ram_post_init -> persistent_ram_zap
|
|--> persistent_ram_zap
There does appear to be a duplicate call to persistent_ram_zap() in this case.
Quoted 3 lines
As we can see, persistent_ram_zap() is called twice. We can avoid this by removing it in ramoops_init_prz(), and only call it in persistent_ram_post_init().
However, I think the proposed fix doesn't work the way it should.
There are two prz init paths: ramoops_init_prz() (a single prz) and ramoops_init_przs (multiple przs). The "dump" and "ftrace" cases use the latter. In those, there is no call to persistent_ram_zap() if the buffer is valid.
In other words:
ramoops_init_prz() unconditionally calls persistent_ram_zap(). (And may call it twice if there is a mismatch of the magic header.)
ramoops_init_przs() only calls persistent_ram_zap() when the magic header is wrong.
The proposed patch unconditionally zaps all regions, which means we'd lose "dump" and "ftrace" across the next reboot.
Perhaps we could make it an option to persistent_ram_new()?

Quoted 7 lines

Quoted 2 lines
--
Kees Cook
Thanks for your reply.
You are right, this patch does zap regions unconditionally when it comes to "dump" and "ftrace". Sorry for the inconvenience owing to my previous mistake.

No problem at all! I'm glad you started this conversation, since it was a subtle aspect of the code that I didn't understand until I took a closer look at it. :)

Quoted 2 lines

I have tried adding an option to persistent_ram_new() according to your advice and will send a V2 version patch later. Could you please kindly pay any attention to it? Thank you!

Yes, thanks! I've added Joel to CC as well, since he was doing clean ups in a related area. I think we'll probably combine the efforts.

-kees

Signature · 1 line

Kees Cook