Messages in this thread | | | From | Kees Cook <> | Date | Sun, 28 Oct 2018 15:46:15 +0000 | Subject | Re: [PATCH] pstore: Remove duplicate invoking of persistent_ram_zap() |
| |
On Sat, Oct 27, 2018 at 9:52 AM, Peng15 Wang 王鹏 <wangpeng15@xiaomi.com> wrote: > > > ________________________________________ >>From: Kees Cook <keescook@chromium.org> >>Sent: Friday, October 26, 2018 17:44 >>To: Peng15 Wang 王鹏 >>Cc: anton@enomsg.org; ccross@android.com; tony.luck@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 王鹏 <wangpeng15@xiaomi.com> wrote: >>> From: Peng Wang <wangpeng15@xiaomi.com> >>> >>> 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.
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. :)
> 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
-- Kees Cook
| |