From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kees Cook Subject: Re: Crash in msm serial on dragonboard with ftrace bootargs Date: Wed, 17 Oct 2018 11:25:34 -0700 Message-ID: References: <1cae8f10-55f5-20ce-9105-30af6f88bd6e@codeaurora.org> <20181016112928.4b52afb5@gandalf.local.home> <20181017101355.GA230639@joelaf.mtv.corp.google.com> <2a23cd74-7364-0fb7-3c7b-7be79a881073@codeaurora.org> <69d2f43d-dc96-9348-7f70-5db88e8f5c39@codeaurora.org> <20181017175611.GB107185@joelaf.mtv.corp.google.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <20181017175611.GB107185@joelaf.mtv.corp.google.com> Sender: linux-kernel-owner@vger.kernel.org To: Joel Fernandes Cc: Sai Prakash Ranjan , Steven Rostedt , Stephen Boyd , Bjorn Andersson , Andy Gross , David Brown , Jiri Slaby , "Ivan T. Ivanov" , Geliang Tang , Greg Kroah-Hartman , Pramod Gurav , linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org, "open list:SERIAL DRIVERS" , LKML , Rajendra Nayak , Vivek Gautam , Sibi Sankar List-Id: linux-arm-msm@vger.kernel.org On Wed, Oct 17, 2018 at 10:56 AM, Joel Fernandes wrote: > On Wed, Oct 17, 2018 at 08:19:41PM +0530, Sai Prakash Ranjan wrote: >> On 10/17/2018 5:08 PM, Sai Prakash Ranjan wrote: >> > > >> > > What do you think about the (untested) patch below? It seems to me >> > > that it >> > > should solve the issue of missing early crash dumps, but I have not >> > > tested it >> > > yet. Sai, would you mind trying it out and let me know if you can see the >> > > early crash dumps properly now? >> > > >> > > ----8<--- >> > > From: "Joel Fernandes (Google)" >> > > Subject: [RFC] pstore: allocate compression during late_initcall >> > > >> > > ramoop's pstore registration (using pstore_register) has to run during >> > > late_initcall because crypto backend may not be ready during >> > > postcore_initcall. This causes missing of dmesg crash dumps which could >> > > have been caught by pstore. >> > > >> > > Instead, lets allow ramoops pstore registration earlier, and once crypto >> > > is ready we can initialize the compression. >> > > >> > > Reported-by: Sai Prakash Ranjan >> > > Signed-off-by: Joel Fernandes (Google) >> > > --- >> > > fs/pstore/platform.c | 13 +++++++++++++ >> > > fs/pstore/ram.c | 2 +- >> > > 2 files changed, 14 insertions(+), 1 deletion(-) >> > > >> > > diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c >> > > index 15e99d5a681d..f09066db2d4d 100644 >> > > --- a/fs/pstore/platform.c >> > > +++ b/fs/pstore/platform.c >> > > @@ -780,6 +780,19 @@ void __init pstore_choose_compression(void) >> > > } >> > > } >> > > +static int __init pstore_compression_late_init(void) >> > > +{ >> > > + /* >> > > + * Check if any pstore backends registered earlier but did not >> > > allocate >> > > + * for compression because crypto was not ready, if so then >> > > initialize >> > > + * compression. >> > > + */ >> > > + if (psinfo && !tfm) >> > > + allocate_buf_for_compression(); >> > > + return 0; >> > > +} >> > > +late_initcall(pstore_compression_late_init); >> > > + >> > > module_param(compress, charp, 0444); >> > > MODULE_PARM_DESC(compress, "Pstore compression to use"); >> > > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c >> > > index bbd1e357c23d..98e48d1a9776 100644 >> > > --- a/fs/pstore/ram.c >> > > +++ b/fs/pstore/ram.c >> > > @@ -940,7 +940,7 @@ static int __init ramoops_init(void) >> > > ramoops_register_dummy(); >> > > return platform_driver_register(&ramoops_driver); >> > > } >> > > -late_initcall(ramoops_init); >> > > +postcore_initcall(ramoops_init); >> > > static void __exit ramoops_exit(void) >> > > { >> > > >> > >> > Yes I could see the early crash dump. Also I tested with different >> > compression (LZO) instead of deflate just to be sure and it works fine, >> > thanks :) >> > >> > Tested-by: Sai Prakash Ranjan >> > > > Thanks. > >> I just noticed that allocate_buf_for_compression() is also called from >> pstore_register(). Shouldn't that call be removed now that ramoops_init is >> moved to postcore_initcall and allocate_buf_for_compression() will just >> return doing nothing when called from pstore_register()? > > Yes, that is the point. If crypto is not ready then my thought is > allocate_buf_for_compression() called from pstore_register() should do > nothing and pstore will work uncompressed and the kdump infrastructure will > only cause uncompressed writes. But say if the kdump triggered *after* crypto > was ready, then the dump write out would be compressed since pstore is ready > for compression. > > The idea is if a future pstore backend calls pstore_register late, then it > may as well do the allocate_buf_for_compression as well at that time when it > runs. In that cause pstore_compression_late_init would do nothing. > > So this approach is both dynamic and future proof. Yeah, thanks! I think this looks correct, but I'll spend some more time testing it too. -Kees -- Kees Cook Pixel Security