lkml.org 
[lkml]   [2019]   [Jan]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] pstore/ram: Replace dummy_data heap memory with stack memory
On Tue, Jan 22, 2019 at 3:47 PM Yue Hu <huyue2@yulong.com> wrote:
>
> On Tue, 22 Jan 2019 14:53:01 +1300
> Kees Cook <keescook@chromium.org> wrote:
>
> > On Tue, Jan 22, 2019 at 2:37 PM Yue Hu <huyue2@yulong.com> wrote:
> > >
> > > From e37cbd4d22eae55c034536817b22d429ba0ae27a Mon Sep 17 00:00:00
> > > 2001 From: Yue Hu <huyue2@yulong.com>
> > > Date: Mon, 21 Jan 2019 18:20:41 +0800
> > > Subject: [PATCH] pstore/ram: Replace dummy_data heap memory with
> > > stack memory
> > >
> > > In ramoops_register_dummy() dummy_data is allocated via kzalloc()
> > > then it will always occupy the heap space after register platform
> > > device via platform_device_register_data(), but it will not be
> > > used any more. So let's free it for system usage, replace it with
> > > stack memory is better due to small size.
> >
> > That's not true: dummy_data is registered with
> > platform_device_register_data() which stores the pointer. (And is used
> > in ramoops_probe().) When the probe runs, the stack frame will be
> > gone.
>
> hi Kees,
>
> Yes, platform_device_register_data() will store the pointer to
> platform_data, however the store process is using memory copy(kmemdup)
> internelly as below:
>
> ```c
> int platform_device_add_data(struct platform_device *pdev, const void
> *data, size_t size)
> {
> void *d = NULL;
>
> if (data) {
> d = kmemdup(data, size, GFP_KERNEL);
> if (!d)
> return -ENOMEM;
> }
>
> kfree(pdev->dev.platform_data);
> pdev->dev.platform_data = d;
> return 0;
> }
> ```

Ah! Yes, you're totally right. I misread
platform_device_register_resndata() and didn't dig far enough. :)

I'll get this added to the series. Thanks!

-Kees

>
> so no need to keep the memory for the parameter of platform specific
> data passed in platform_device_register_data().
>
> When using the change, probe is successful and it's working fine by
> test.
>
> >
> > That said, it would be possible to move the stack allocation into
> > ramoops_init() instead, since the frame will still be available during
> > the call to platform_driver_register() which will call
> > ramoops_probe(). If you made that change, then we could drop the
> > ramoops_unregister_dummy() call, and rename ramoops_register_dummy()
> > into ramoops_populate_dummy() or something.
> >
> > But even in this case, you'd need to remove the platform data after
> > the probe just to make sure the stack pointer didn't stick around.
> >
> > -Kees
> >
> > >
> > > Signed-off-by: Yue Hu <huyue2@yulong.com>
> > > ---
> > > fs/pstore/ram.c | 34 +++++++++++++---------------------
> > > 1 file changed, 13 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> > > index 96f7d32..8db1f7f 100644
> > > --- a/fs/pstore/ram.c
> > > +++ b/fs/pstore/ram.c
> > > @@ -110,7 +110,6 @@ struct ramoops_context {
> > > };
> > >
> > > static struct platform_device *dummy;
> > > -static struct ramoops_platform_data *dummy_data;
> > >
> > > static int ramoops_pstore_open(struct pstore_info *psi)
> > > {
> > > @@ -896,13 +895,12 @@ static inline void
> > > ramoops_unregister_dummy(void) {
> > > platform_device_unregister(dummy);
> > > dummy = NULL;
> > > -
> > > - kfree(dummy_data);
> > > - dummy_data = NULL;
> > > }
> > >
> > > static void __init ramoops_register_dummy(void)
> > > {
> > > + struct ramoops_platform_data pdata;
> > > +
> > > /*
> > > * Prepare a dummy platform data structure to carry the
> > > module
> > > * parameters. If mem_size isn't set, then there are no
> > > module @@ -913,30 +911,24 @@ static void __init
> > > ramoops_register_dummy(void)
> > >
> > > pr_info("using module parameters\n");
> > >
> > > - dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
> > > - if (!dummy_data) {
> > > - pr_info("could not allocate pdata\n");
> > > - return;
> > > - }
> > > -
> > > - dummy_data->mem_size = mem_size;
> > > - dummy_data->mem_address = mem_address;
> > > - dummy_data->mem_type = mem_type;
> > > - dummy_data->record_size = record_size;
> > > - dummy_data->console_size = ramoops_console_size;
> > > - dummy_data->ftrace_size = ramoops_ftrace_size;
> > > - dummy_data->pmsg_size = ramoops_pmsg_size;
> > > - dummy_data->dump_oops = dump_oops;
> > > - dummy_data->flags = RAMOOPS_FLAG_FTRACE_PER_CPU;
> > > + pdata.mem_size = mem_size;
> > > + pdata.mem_address = mem_address;
> > > + pdata.mem_type = mem_type;
> > > + pdata.record_size = record_size;
> > > + pdata.console_size = ramoops_console_size;
> > > + pdata.ftrace_size = ramoops_ftrace_size;
> > > + pdata.pmsg_size = ramoops_pmsg_size;
> > > + pdata.dump_oops = dump_oops;
> > > + pdata.flags = RAMOOPS_FLAG_FTRACE_PER_CPU;
> > >
> > > /*
> > > * For backwards compatibility ramoops.ecc=1 means 16 bytes
> > > ECC
> > > * (using 1 byte for ECC isn't much of use anyway).
> > > */
> > > - dummy_data->ecc_info.ecc_size = ramoops_ecc == 1 ? 16 :
> > > ramoops_ecc;
> > > + pdata.ecc_info.ecc_size = ramoops_ecc == 1 ? 16 :
> > > ramoops_ecc;
> > >
> > > dummy = platform_device_register_data(NULL, "ramoops", -1,
> > > - dummy_data, sizeof(struct
> > > ramoops_platform_data));
> > > + &pdata, sizeof(struct
> > > ramoops_platform_data)); if (IS_ERR(dummy)) {
> > > pr_info("could not create platform device: %ld\n",
> > > PTR_ERR(dummy));
> > > --
> > > 1.9.1
> > >
> > >
> > >
> > >
> >
> >
>
>
>


--
Kees Cook

\
 
 \ /
  Last update: 2019-01-22 03:59    [W:0.041 / U:0.260 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site