Messages in this thread | | | From | Kees Cook <> | Date | Tue, 22 Jan 2019 15:58:27 +1300 | Subject | Re: [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
| |