From: "Peng15 Wang 王鹏" <wangpeng15@xiaomi.com>
To: Kees Cook <keescook@chromium.org>
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>
Subject: Re: [PATCH] pstore: Remove duplicate invoking of persistent_ram_zap()
Date: Sat, 27 Oct 2018 08:52:58 +0000 [thread overview]
Message-ID: <1540630374669.30217@xiaomi.com> (raw)
In-Reply-To: <CAGXu5jJtwLgrMVvXR+C8oSKPus+iVdYsYn4Q2+8FjRbgpL+jNA@mail.gmail.com>
________________________________________
>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.
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!
next prev parent reply other threads:[~2018-10-27 8:53 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 王鹏 [this message]
2018-10-28 15:46 ` Kees Cook
-- 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=1540630374669.30217@xiaomi.com \
--to=wangpeng15@xiaomi.com \
--cc=anton@enomsg.org \
--cc=ccross@android.com \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tony.luck@intel.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