Messages in this thread Patch in this message | | | Date | Mon, 29 Oct 2018 21:22:08 -0700 | From | Joel Fernandes <> | Subject | Re: [PATCH v2] pstore: Avoid duplicate call of persistent_ram_zap() |
| |
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
| |