Re: [PATCH v4] pstore: Avoid duplicate call of persistent_ram_zap()
>From: Joel Fernandes
>Sent: Wednesday, October 31, 2018 6:16
>To: Kees Cook
>Cc: Peng15 Wang 王鹏; Anton Vorontsov; Colin Cross; Tony Luck; LKML;
>vipwangerx...@gmail.com
>Subject: Re: [PATCH v4] pstore: Avoid duplicate call of persistent_ram_zap()
>
>On Tue, Oct 30, 2018 at 02:52:43PM -0700, Kees Cook wrote:
>> On Tue, Oct 30, 2018 at 2:38 PM, Joel Fernandes
>> wrote:
>> > On Tue, Oct 30, 2018 at 03:52:34PM +0800, Peng Wang wrote:
>> >> When initialing prz 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 adding an option to persistent_ram_new(), and
>> >> only call persistent_ram_zap() when it is needed.
>> >>
>> >> Signed-off-by: Peng Wang
>> >> ---
>> >> fs/pstore/ram.c| 4 +---
>> >> fs/pstore/ram_core.c | 5 +++--
>> >> include/linux/pstore_ram.h | 1 +
>> >> 3 files changed, 5 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> >> index ffcff6516e89..b51901f97dc2 100644
>> >> --- a/fs/pstore/ram.c
>> >> +++ b/fs/pstore/ram.c
>> >> @@ -640,7 +640,7 @@ static int ramoops_init_prz(const char *name,
>> >>
>> >> label = kasprintf(GFP_KERNEL, "ramoops:%s", name);
>> >> *prz = persistent_ram_new(*paddr, sz, sig, >ecc_info,
>> >> - cxt->memtype, 0, label);
>> >> + cxt->memtype, PRZ_FLAG_ZAP_OLD, label);
>> >> if (IS_ERR(*prz)) {
>> >> int err = PTR_ERR(*prz);
>> >
>> > Looks good to me except the minor comment below:
>> >
>> >>
>> >> @@ -649,8 +649,6 @@ static int ramoops_init_prz(const char *name,
>> >> return err;
>> >> }
>> >>
>> >> - persistent_ram_zap(*prz);
>> >> -
>> >> *paddr += sz;
>> >>
>> >> return 0;
>> >> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
>> >> index 12e21f789194..2ededd1ea1c2 100644
>> >> --- a/fs/pstore/ram_core.c
>> >> +++ b/fs/pstore/ram_core.c
>> >> @@ -505,15 +505,16 @@ 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;
>> >> + if (!(prz->flags & PRZ_FLAG_ZAP_OLD))
>> >> + return 0;
>> >
>> > This could be written differently.
>> >
>> > We could just do:
>> >
>> > if (prz->flags & PRZ_FLAG_ZAP_OLD)
>> > persistent_ram_zap(prz);
>> >
>> > And remove the zap from below below.
>>
>> I actually rearranged things a little to avoid additional round-trips
>> on the mailing list. :)
>>
>> > Since Kees already took this patch, I can just patch this in my series if
>> > Kees and you are Ok with this suggestion.
>>
>> I've put it up here:
>> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=pstore/devel=ac564e023248e3f4d87917b91d12376ddfca5000
>
>Cool, it LGTM :)
>
>- Joel
>
Thank you all for these warm help.
This is my first time to submit a patch to community. Feel great!
Re: [PATCH v4] pstore: Avoid duplicate call of persistent_ram_zap()
>From: Joel Fernandes
>Sent: Wednesday, October 31, 2018 6:16
>To: Kees Cook
>Cc: Peng15 Wang 王鹏; Anton Vorontsov; Colin Cross; Tony Luck; LKML;
>vipwangerx...@gmail.com
>Subject: Re: [PATCH v4] pstore: Avoid duplicate call of persistent_ram_zap()
>
>On Tue, Oct 30, 2018 at 02:52:43PM -0700, Kees Cook wrote:
>> On Tue, Oct 30, 2018 at 2:38 PM, Joel Fernandes
>> wrote:
>> > On Tue, Oct 30, 2018 at 03:52:34PM +0800, Peng Wang wrote:
>> >> When initialing prz 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 adding an option to persistent_ram_new(), and
>> >> only call persistent_ram_zap() when it is needed.
>> >>
>> >> Signed-off-by: Peng Wang
>> >> ---
>> >> fs/pstore/ram.c| 4 +---
>> >> fs/pstore/ram_core.c | 5 +++--
>> >> include/linux/pstore_ram.h | 1 +
>> >> 3 files changed, 5 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> >> index ffcff6516e89..b51901f97dc2 100644
>> >> --- a/fs/pstore/ram.c
>> >> +++ b/fs/pstore/ram.c
>> >> @@ -640,7 +640,7 @@ static int ramoops_init_prz(const char *name,
>> >>
>> >> label = kasprintf(GFP_KERNEL, "ramoops:%s", name);
>> >> *prz = persistent_ram_new(*paddr, sz, sig, >ecc_info,
>> >> - cxt->memtype, 0, label);
>> >> + cxt->memtype, PRZ_FLAG_ZAP_OLD, label);
>> >> if (IS_ERR(*prz)) {
>> >> int err = PTR_ERR(*prz);
>> >
>> > Looks good to me except the minor comment below:
>> >
>> >>
>> >> @@ -649,8 +649,6 @@ static int ramoops_init_prz(const char *name,
>> >> return err;
>> >> }
>> >>
>> >> - persistent_ram_zap(*prz);
>> >> -
>> >> *paddr += sz;
>> >>
>> >> return 0;
>> >> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
>> >> index 12e21f789194..2ededd1ea1c2 100644
>> >> --- a/fs/pstore/ram_core.c
>> >> +++ b/fs/pstore/ram_core.c
>> >> @@ -505,15 +505,16 @@ 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;
>> >> + if (!(prz->flags & PRZ_FLAG_ZAP_OLD))
>> >> + return 0;
>> >
>> > This could be written differently.
>> >
>> > We could just do:
>> >
>> > if (prz->flags & PRZ_FLAG_ZAP_OLD)
>> > persistent_ram_zap(prz);
>> >
>> > And remove the zap from below below.
>>
>> I actually rearranged things a little to avoid additional round-trips
>> on the mailing list. :)
>>
>> > Since Kees already took this patch, I can just patch this in my series if
>> > Kees and you are Ok with this suggestion.
>>
>> I've put it up here:
>> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=pstore/devel=ac564e023248e3f4d87917b91d12376ddfca5000
>
>Cool, it LGTM :)
>
>- Joel
>
Thank you all for these warm help.
This is my first time to submit a patch to community. Feel great!
Re: [PATCH] pstore: Remove duplicate invoking of persistent_ram_zap()
>From: Kees Cook >Sent: Friday, October 26, 2018 17:44 >To: Peng15 Wang 王鹏 >Cc: an...@enomsg.org; ccr...@android.com; tony.l...@intel.com; >linux-kernel@vger.kernel.org >Subject: Re: [PATCH] pstore: Remove duplicate invoking of persistent_ram_zap() >On Fri, Oct 26, 2018 at 4:41 AM, Peng15 Wang 王鹏 wrote: >> From: Peng Wang >> >> 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. > >> 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()? > >-- >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!
Re: [PATCH] pstore: Remove duplicate invoking of persistent_ram_zap()
>From: Kees Cook >Sent: Friday, October 26, 2018 17:44 >To: Peng15 Wang 王鹏 >Cc: an...@enomsg.org; ccr...@android.com; tony.l...@intel.com; >linux-kernel@vger.kernel.org >Subject: Re: [PATCH] pstore: Remove duplicate invoking of persistent_ram_zap() >On Fri, Oct 26, 2018 at 4:41 AM, Peng15 Wang 王鹏 wrote: >> From: Peng Wang >> >> 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. > >> 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()? > >-- >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!
[PATCH] pstore: Remove duplicate invoking of persistent_ram_zap()
From: Peng Wang
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
---
fs/pstore/ram.c | 2 --
fs/pstore/ram_core.c | 3 +--
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ffcff6516e89..3f57074a4fab 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -649,8 +649,6 @@ static int ramoops_init_prz(const char *name,
return err;
}
- persistent_ram_zap(*prz);
-
*paddr += sz;
return 0;
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 12e21f789194..3ef0fd5a7cd5 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -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;
--
2.19.1
[PATCH] pstore: Remove duplicate invoking of persistent_ram_zap()
From: Peng Wang
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
---
fs/pstore/ram.c | 2 --
fs/pstore/ram_core.c | 3 +--
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ffcff6516e89..3f57074a4fab 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -649,8 +649,6 @@ static int ramoops_init_prz(const char *name,
return err;
}
- persistent_ram_zap(*prz);
-
*paddr += sz;
return 0;
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 12e21f789194..3ef0fd5a7cd5 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -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;
--
2.19.1
