* Re: [PATCH v2] pstore: Avoid duplicate call of persistent_ram_zap()
[not found] <1540645734426.61104@xiaomi.com>
@ 2018-10-28 16:03 ` Kees Cook
[not found] ` <1540795068068.62079@xiaomi.com>
0 siblings, 1 reply; 2+ messages in thread
From: Kees Cook @ 2018-10-28 16:03 UTC (permalink / raw)
To: Peng15 Wang 王鹏
Cc: anton@enomsg.org, ccross@android.com, tony.luck@intel.com,
linux-kernel@vger.kernel.org, Joel Fernandes
On Sat, Oct 27, 2018 at 2:08 PM, Peng15 Wang 王鹏 <wangpeng15@xiaomi.com> 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 <wangpeng15@xiaomi.com>
> ---
> fs/pstore/ram.c | 5 +++--
> fs/pstore/ram_core.c | 11 +++++++----
> include/linux/pstore_ram.h | 3 ++-
> 3 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index ffcff6516e89..3044274de2f0 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -596,7 +596,8 @@ static int ramoops_init_przs(const char *name,
> name, i, *cnt - 1);
> prz_ar[i] = persistent_ram_new(*paddr, zone_sz, sig,
> &cxt->ecc_info,
> - cxt->memtype, flags, label);
> + cxt->memtype, flags,
> + label, true);
> if (IS_ERR(prz_ar[i])) {
> err = PTR_ERR(prz_ar[i]);
> dev_err(dev, "failed to request %s mem region (0x%zx@0x%llx): %d\n",
> @@ -640,7 +641,7 @@ static int ramoops_init_prz(const char *name,
>
> label = kasprintf(GFP_KERNEL, "ramoops:%s", name);
> *prz = persistent_ram_new(*paddr, sz, sig, &cxt->ecc_info,
> - cxt->memtype, 0, label);
> + cxt->memtype, 0, label, false);
> if (IS_ERR(*prz)) {
> int err = PTR_ERR(*prz);
>
> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
> index 12e21f789194..d8a520c8741c 100644
> --- a/fs/pstore/ram_core.c
> +++ b/fs/pstore/ram_core.c
> @@ -486,7 +486,8 @@ static int persistent_ram_buffer_map(phys_addr_t start, phys_addr_t size,
> }
>
> static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig,
> - struct persistent_ram_ecc_info *ecc_info)
> + struct persistent_ram_ecc_info *ecc_info,
> + bool zap_option)
> {
> int ret;
>
> @@ -514,7 +515,8 @@ static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig,
>
> /* Rewind missing or invalid memory area. */
> prz->buffer->sig = sig;
> - persistent_ram_zap(prz);
> + if (zap_option)
> + persistent_ram_zap(prz);
This part of persistent_ram_post_init() handles the "invalid buffer"
case, which should always zap. The question is whether or not to zap
in the case of a valid buffer (the "return 0" earlier in the
function). I think you v2 patch needs similar changes found in your
v1: the v2 patch also needs to remove the "return 0" and replace it
with "zap_option = true;" and to remove the zap call from
ramoops_init_prz(). Then I think all the paths will be consolidated.
-Kees
>
> return 0;
> }
> @@ -548,7 +550,8 @@ void persistent_ram_free(struct persistent_ram_zone *prz)
>
> struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
> u32 sig, struct persistent_ram_ecc_info *ecc_info,
> - unsigned int memtype, u32 flags, char *label)
> + unsigned int memtype, u32 flags, char *label,
> + bool zap_option)
> {
> struct persistent_ram_zone *prz;
> int ret = -ENOMEM;
> @@ -568,7 +571,7 @@ struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
> if (ret)
> goto err;
>
> - ret = persistent_ram_post_init(prz, sig, ecc_info);
> + ret = persistent_ram_post_init(prz, sig, ecc_info, zap_option);
> if (ret)
> goto err;
>
> diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
> index 602d64725222..5d4acad9fe56 100644
> --- a/include/linux/pstore_ram.h
> +++ b/include/linux/pstore_ram.h
> @@ -66,7 +66,8 @@ struct persistent_ram_zone {
>
> struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
> u32 sig, struct persistent_ram_ecc_info *ecc_info,
> - unsigned int memtype, u32 flags, char *label);
> + unsigned int memtype, u32 flags, char *label,
> + bool zap_option);
> void persistent_ram_free(struct persistent_ram_zone *prz);
> void persistent_ram_zap(struct persistent_ram_zone *prz);
>
> --
> 2.19.1
--
Kees Cook
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH v2] pstore: Avoid duplicate call of persistent_ram_zap()
[not found] ` <1540795068068.62079@xiaomi.com>
@ 2018-10-30 4:22 ` Joel Fernandes
0 siblings, 0 replies; 2+ messages in thread
From: Joel Fernandes @ 2018-10-30 4:22 UTC (permalink / raw)
To: Peng15 Wang 王鹏
Cc: Kees Cook, anton@enomsg.org, ccross@android.com,
tony.luck@intel.com, linux-kernel@vger.kernel.org
On Mon, Oct 29, 2018 at 06:37:53AM +0000, Peng15 Wang 王鹏 wrote:
>
> ________________________________________
> >From: Kees Cook <keescook@chromium.org>
> >Sent: Monday, October 29, 2018 0:03
> >To: Peng15 Wang 王鹏
> >Cc: anton@enomsg.org; ccross@android.com; tony.luck@intel.com; linux-kernel@vger.kernel.org; Joel Fernandes
> >Subject: Re: [PATCH v2] pstore: Avoid duplicate call of persistent_ram_zap()
> >
> >On Sat, Oct 27, 2018 at 2:08 PM, Peng15 Wang 王鹏 <wangpeng15@xiaomi.com> 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 <wangpeng15@xiaomi.com>
> >> ---
> >> fs/pstore/ram.c | 5 +++--
> >> fs/pstore/ram_core.c | 11 +++++++----
> >> include/linux/pstore_ram.h | 3 ++-
> >> 3 files changed, 12 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> >> index ffcff6516e89..3044274de2f0 100644
> >> --- a/fs/pstore/ram.c
> >> +++ b/fs/pstore/ram.c
> >> @@ -596,7 +596,8 @@ static int ramoops_init_przs(const char *name,
> >> name, i, *cnt - 1);
> >> prz_ar[i] = persistent_ram_new(*paddr, zone_sz, sig,
> >> &cxt->ecc_info,
> >> - cxt->memtype, flags, label);
> >> + cxt->memtype, flags,
> >> + label, true);
> >> if (IS_ERR(prz_ar[i])) {
> >> err = PTR_ERR(prz_ar[i]);
> >> dev_err(dev, "failed to request %s mem region (0x%zx@0x%llx): %d\n",
> >> @@ -640,7 +641,7 @@ static int ramoops_init_prz(const char *name,
> >>
> >> label = kasprintf(GFP_KERNEL, "ramoops:%s", name);
> >> *prz = persistent_ram_new(*paddr, sz, sig, &cxt->ecc_info,
> >> - cxt->memtype, 0, label);
> >> + cxt->memtype, 0, label, false);
> >> if (IS_ERR(*prz)) {
> >> int err = PTR_ERR(*prz);
> >>
> >> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
> >> index 12e21f789194..d8a520c8741c 100644
> >> --- a/fs/pstore/ram_core.c
> >> +++ b/fs/pstore/ram_core.c
> >> @@ -486,7 +486,8 @@ static int persistent_ram_buffer_map(phys_addr_t start, phys_addr_t size,
> >> }
> >>
> >> static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig,
> >> - struct persistent_ram_ecc_info *ecc_info)
> >> + struct persistent_ram_ecc_info *ecc_info,
> >> + bool zap_option)
> >> {
> >> int ret;
> >>
> >> @@ -514,7 +515,8 @@ static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 >sig,
> >>
> >> /* Rewind missing or invalid memory area. */
> >> prz->buffer->sig = sig;
> >> - persistent_ram_zap(prz);
> >> + if (zap_option)
> >> + persistent_ram_zap(prz);
> >
> >This part of persistent_ram_post_init() handles the "invalid buffer"
> >case, which should always zap. The question is whether or not to zap
> >in the case of a valid buffer (the "return 0" earlier in the
> >function). I think you v2 patch needs similar changes found in your
> >v1: the v2 patch also needs to remove the "return 0" and replace it
> >with "zap_option = true;" and to remove the zap call from
> >ramoops_init_prz(). Then I think all the paths will be consolidated.
>
> Thank you so much for the tips!
>
> Furthermore, we can make "zap_option" stand for whether its caller want to zap in case of
> a valid buffer. So ramoops_init_przs() would say "false", and ramoops_init_prz() would
> say "true".
>
> In persistent_ram_post_init(), if zap_option says "false", we return immediately after
> persistent_ram_save_old(), otherwise persistent_ram_zap would be called at the end.
Can you not just add it to the flags, something like PRZ_ZAP_NEW, and set
that flag before calling ramoops_init_prz*, then check the flag in
persistent_ram_new? We are already passing flags to persistent_ram_new.
That way no new function arguments are needed and its simple.
- Joel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-10-30 4:22 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1540645734426.61104@xiaomi.com>
2018-10-28 16:03 ` [PATCH v2] pstore: Avoid duplicate call of persistent_ram_zap() Kees Cook
[not found] ` <1540795068068.62079@xiaomi.com>
2018-10-30 4:22 ` Joel Fernandes
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox