From: Kees Cook <keescook@chromium.org>
To: "Peng15 Wang 王鹏" <wangpeng15@xiaomi.com>
Cc: "anton@enomsg.org" <anton@enomsg.org>,
"ccross@android.com" <ccross@android.com>,
"tony.luck@intel.com" <tony.luck@intel.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Joel Fernandes <joel@joelfernandes.org>
Subject: Re: [PATCH] pstore: Remove duplicate invoking of persistent_ram_zap()
Date: Sun, 28 Oct 2018 15:46:15 +0000 [thread overview]
Message-ID: <CAGXu5jJcPh7sSEXNAZpeatqLnvsuySPyYpQvDeP6AXnsxCr_cg@mail.gmail.com> (raw)
In-Reply-To: <1540630374669.30217@xiaomi.com>
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
next prev parent reply other threads:[~2018-10-28 15:46 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-26 3:41 [PATCH] pstore: Remove duplicate invoking of persistent_ram_zap() Peng15 Wang 王鹏
2018-10-26 9:44 ` Kees Cook
2018-10-27 8:52 ` Peng15 Wang 王鹏
2018-10-28 15:46 ` Kees Cook [this message]
-- strict thread matches above, loose matches on Subject: below --
2018-10-25 13:16 Peng Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAGXu5jJcPh7sSEXNAZpeatqLnvsuySPyYpQvDeP6AXnsxCr_cg@mail.gmail.com \
--to=keescook@chromium.org \
--cc=anton@enomsg.org \
--cc=ccross@android.com \
--cc=joel@joelfernandes.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tony.luck@intel.com \
--cc=wangpeng15@xiaomi.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).