RE: [PATCH v2 4/5] ramoops: support multiple pmsg instances

[åæäæ / IWAMATSUïNOBUHIRO]

Hi,

> -----Original Message-----
> From: keescook@xxxxxxxxxx [mailto:keescook@xxxxxxxxxx] On Behalf Of Kees
> Cook
> Sent: Friday, September 09, 2016 6:53 AM
> To: åæäæ / IWAMATSUïNOBUHIRO
> Cc: Anton Vorontsov; Colin Cross; Tony Luck; LKML; Hiraku Toyooka; Mark
> Salyzyn; éåèå / AGUCHIïSEIJI
> Subject: Re: [PATCH v2 4/5] ramoops: support multiple pmsg instances
> 
> On Sun, Jul 24, 2016 at 8:56 PM, Nobuhiro Iwamatsu
> <nobuhiro.iwamatsu.kw@xxxxxxxxxxx> wrote:
> > From: Hiraku Toyooka <hiraku.toyooka.gu@xxxxxxxxxxx>
> >
> > This enables ramoops to deal with multiple pmsg instances.
> > A User can configure the size of each buffers by its module parameter
> > as follows.
> >
> >   pmsg_size=0x1000,0x2000,...
> >
> > Then, the ramoops allocates multiple buffers and tells the number of
> > buffers to pstore to create multiple /dev/pmsg[ID].
> >
> > Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@xxxxxxxxxxx>
> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@xxxxxxxxxxx>
> > Cc: Mark Salyzyn <salyzyn@xxxxxxxxxxx>
> > Cc: Seiji Aguchi <seiji.aguchi.tr@xxxxxxxxxxx>
> > ---
> >  Documentation/ramoops.txt  |  22 +++++++
> >  fs/pstore/ram.c            | 146
> ++++++++++++++++++++++++++++++++++++++-------
> >  include/linux/pstore_ram.h |   8 ++-
> >  3 files changed, 153 insertions(+), 23 deletions(-)
> >
> > diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
> > index 5d86756..cff6ac7 100644
> > --- a/Documentation/ramoops.txt
> > +++ b/Documentation/ramoops.txt
> > @@ -126,3 +126,25 @@ file. Here is an example of usage:
> >   0 ffffffff811d9c54  ffffffff8101a7a0  __const_udelay <-
> native_machine_emergency_restart+0x110/0x1e0
> >   0 ffffffff811d9c34  ffffffff811d9c80  __delay <-
> __const_udelay+0x30/0x40
> >   0 ffffffff811d9d14  ffffffff811d9c3f  delay_tsc <- __delay+0xf/0x20
> > +
> > +6. Pmsg support
> > +
> > +Ramoops supports pmsg - logging userspace mesages in persistent
> > +store. By default, one pmsg area becomes available in ramoops. You
> > +can write data into /dev/pmsg0, e.g.:
> > +
> > + # echo foo > /dev/pmsg0
> > +
> > +After reboot, the stored data can be read from pmsg-ramoops-0 on the
> > +pstore filesystem.
> > +
> > +You can specify size of the pmsg area by additional kernel command line,
> e.g.:
> > +
> > + "ramoops.pmsg_size=0x1000"
> > +
> > +You can also use multiple pmsg areas, e.g.:
> > +
> > + "ramoops.pmsg_size=0x1000,0x2000,..."
> > +
> > +Then, pmsg0, pmsg1, ... will appear on /dev. This is useful to
> > +control individual content aging or priority.
> 
> The documentation for the DT bindings will need updating too, in:
> Documentation/devicetree/bindings/reserved-memory/ramoops.txt

OK, I will add DT bindings to documentation.

> 
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c index 288c5d0..2bf893f
> > 100644
> > --- a/fs/pstore/ram.c
> > +++ b/fs/pstore/ram.c
> > @@ -51,8 +51,8 @@ static ulong ramoops_ftrace_size = MIN_MEM_SIZE;
> > module_param_named(ftrace_size, ramoops_ftrace_size, ulong, 0400);
> > MODULE_PARM_DESC(ftrace_size, "size of ftrace log");
> >
> > -static ulong ramoops_pmsg_size = MIN_MEM_SIZE;
> > -module_param_named(pmsg_size, ramoops_pmsg_size, ulong, 0400);
> > +static char *ramoops_pmsg_size_str;
> > +module_param_named(pmsg_size, ramoops_pmsg_size_str, charp, 0400);
> >  MODULE_PARM_DESC(pmsg_size, "size of user space message log");
> >
> >  static unsigned long long mem_address; @@ -86,14 +86,14 @@ struct
> > ramoops_context {
> >         struct persistent_ram_zone **dprzs;
> >         struct persistent_ram_zone *cprz;
> >         struct persistent_ram_zone *fprz;
> > -       struct persistent_ram_zone *mprz;
> > +       struct persistent_ram_zone **mprzs;
> >         phys_addr_t phys_addr;
> >         unsigned long size;
> >         unsigned int memtype;
> >         size_t record_size;
> >         size_t console_size;
> >         size_t ftrace_size;
> > -       size_t pmsg_size;
> > +       size_t *pmsg_size;
> >         int dump_oops;
> >         struct persistent_ram_ecc_info ecc_info;
> >         unsigned int max_dump_cnt;
> > @@ -103,6 +103,7 @@ struct ramoops_context {
> >         unsigned int console_read_cnt;
> >         unsigned int ftrace_read_cnt;
> >         unsigned int pmsg_read_cnt;
> > +       unsigned int num_pmsg;
> >         struct pstore_info pstore;
> >  };
> >
> > @@ -220,9 +221,10 @@ static ssize_t ramoops_pstore_read(u64 *id, enum
> pstore_type_id *type,
> >         if (!prz_ok(prz))
> >                 prz = ramoops_get_next_prz(&cxt->fprz,
> &cxt->ftrace_read_cnt,
> >                                            1, id, type,
> PSTORE_TYPE_FTRACE, 0);
> > -       if (!prz_ok(prz))
> > -               prz = ramoops_get_next_prz(&cxt->mprz,
> &cxt->pmsg_read_cnt,
> > -                                          1, id, type,
> PSTORE_TYPE_PMSG, 0);
> > +       while (cxt->pmsg_read_cnt < cxt->num_pmsg && !prz)
> > +               prz = ramoops_get_next_prz(cxt->mprzs,
> &cxt->pmsg_read_cnt,
> > +                                          cxt->num_pmsg, id, type,
> > +                                          PSTORE_TYPE_PMSG, 0);
> >         if (!prz_ok(prz))
> >                 return 0;
> >
> > @@ -286,9 +288,11 @@ static int notrace ramoops_pstore_write_buf(enum
> pstore_type_id type,
> >                 persistent_ram_write(cxt->fprz, buf, size);
> >                 return 0;
> >         } else if (type == PSTORE_TYPE_PMSG) {
> > -               if (!cxt->mprz)
> > +               if (part >= cxt->num_pmsg)
> > +                       return -EINVAL;
> > +               if (!cxt->mprzs || !cxt->mprzs[part])
> >                         return -ENOMEM;
> > -               persistent_ram_write(cxt->mprz, buf, size);
> > +               persistent_ram_write(cxt->mprzs[part], buf, size);
> >                 return 0;
> >         }
> >
> > @@ -348,7 +352,9 @@ static int ramoops_pstore_erase(enum pstore_type_id
> type, u64 id, int count,
> >                 prz = cxt->fprz;
> >                 break;
> >         case PSTORE_TYPE_PMSG:
> > -               prz = cxt->mprz;
> > +               if (id >= cxt->num_pmsg)
> > +                       return -EINVAL;
> > +               prz = cxt->mprzs[id];
> >                 break;
> >         default:
> >                 return -EINVAL;
> > @@ -426,6 +432,37 @@ fail_prz:
> >         return err;
> >  }
> >
> > +static int ramoops_init_mprzs(struct device *dev, struct
> ramoops_context *cxt,
> > +                             phys_addr_t *paddr, unsigned long total)
> > +{
> > +       int err = -ENOMEM;
> > +       int i;
> > +
> > +       if (!total)
> > +               return 0;
> > +
> > +       if (*paddr + total - cxt->phys_addr > cxt->size) {
> > +               dev_err(dev, "no room for pmsg\n");
> > +               return -ENOMEM;
> > +       }
> > +
> > +       cxt->mprzs = kcalloc(cxt->num_pmsg, sizeof(*cxt->mprzs),
> GFP_KERNEL);
> > +       if (!cxt->mprzs)
> > +               goto fail_prz;
> > +
> > +       for (i = 0; i < cxt->num_pmsg; i++) {
> > +               err = __ramoops_init_prz(dev, cxt, &cxt->mprzs[i],
> paddr,
> > +                                        cxt->pmsg_size[i], 0, true);
> > +               if (err)
> > +                       goto fail_prz;
> > +       }
> > +
> > +       return 0;
> > +fail_prz:
> > +       ramoops_free_przs(cxt->mprzs);
> > +       return err;
> > +}
> > +
> >  static int ramoops_init_prz(struct device *dev, struct ramoops_context
> *cxt,
> >                             struct persistent_ram_zone **prz,
> >                             phys_addr_t *paddr, size_t sz, u32 sig) @@
> > -472,6 +509,8 @@ static int ramoops_probe(struct platform_device *pdev)
> >         size_t dump_mem_sz;
> >         phys_addr_t paddr;
> >         int err = -EINVAL;
> > +       unsigned long pmsg_size_total = 0;
> > +       unsigned int num_pmsg = 0;
> >
> >         /* Only a single ramoops area allowed at a time, so fail extra
> >          * probes.
> > @@ -492,8 +531,18 @@ static int ramoops_probe(struct platform_device
> *pdev)
> >                 pdata->console_size =
> rounddown_pow_of_two(pdata->console_size);
> >         if (pdata->ftrace_size && !is_power_of_2(pdata->ftrace_size))
> >                 pdata->ftrace_size =
> rounddown_pow_of_two(pdata->ftrace_size);
> > -       if (pdata->pmsg_size && !is_power_of_2(pdata->pmsg_size))
> > -               pdata->pmsg_size =
> rounddown_pow_of_two(pdata->pmsg_size);
> > +       if (pdata->pmsg_size) {
> > +               for (;; num_pmsg++) {
> > +                       unsigned long size =
> > + pdata->pmsg_size[num_pmsg];
> > +
> > +                       if (!size)
> > +                               break;
> > +                       if (!is_power_of_2(size))
> > +                               pdata->pmsg_size[num_pmsg]
> > +                                       = rounddown_pow_of_two(size);
> > +                       pmsg_size_total += size;
> > +               }
> > +       }
> >
> >         cxt->size = pdata->mem_size;
> >         cxt->phys_addr = pdata->mem_address; @@ -501,17 +550,26 @@
> > static int ramoops_probe(struct platform_device *pdev)
> >         cxt->record_size = pdata->record_size;
> >         cxt->console_size = pdata->console_size;
> >         cxt->ftrace_size = pdata->ftrace_size;
> > -       cxt->pmsg_size = pdata->pmsg_size;
> >         cxt->dump_oops = pdata->dump_oops;
> >         cxt->ecc_info = pdata->ecc_info;
> > +       cxt->num_pmsg = num_pmsg;
> > +       if (num_pmsg) {
> > +               cxt->pmsg_size = kcalloc(num_pmsg, sizeof(size_t),
> GFP_KERNEL);
> > +               if (!cxt->pmsg_size) {
> > +                       err = -ENOMEM;
> > +                       goto fail_out;
> > +               }
> > +               memcpy(cxt->pmsg_size, pdata->pmsg_size,
> > +                      sizeof(size_t) * num_pmsg);
> > +       }
> >
> >         paddr = cxt->phys_addr;
> >
> >         dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size
> > -                       - cxt->pmsg_size;
> > +                       - pmsg_size_total;
> >         err = ramoops_init_dprzs(dev, cxt, &paddr, dump_mem_sz);
> >         if (err)
> > -               goto fail_out;
> > +               goto fail_init_dprzs;
> >
> >         err = ramoops_init_prz(dev, cxt, &cxt->cprz, &paddr,
> >                                cxt->console_size, 0); @@ -523,11
> > +581,12 @@ static int ramoops_probe(struct platform_device *pdev)
> >         if (err)
> >                 goto fail_init_fprz;
> >
> > -       err = ramoops_init_prz(dev, cxt, &cxt->mprz, &paddr,
> cxt->pmsg_size, 0);
> > +       err = ramoops_init_mprzs(dev, cxt, &paddr, pmsg_size_total);
> >         if (err)
> > -               goto fail_init_mprz;
> > +               goto fail_init_mprzs;
> >
> >         cxt->pstore.data = cxt;
> > +       cxt->pstore.num_pmsg = num_pmsg;
> >         /*
> >          * Console can handle any buffer size, so prefer LOG_LINE_MAX.
> If we
> >          * have to handle dumps, we must have at least record_size
> > buffer. And @@ -560,7 +619,6 @@ static int ramoops_probe(struct
> platform_device *pdev)
> >         record_size = pdata->record_size;
> >         dump_oops = pdata->dump_oops;
> >         ramoops_console_size = pdata->console_size;
> > -       ramoops_pmsg_size = pdata->pmsg_size;
> 
> Doesn't this mean the module parameters for the pmsg_size list won't be
> visible to sysfs?

I see. I will think other solution.
> 
> >         ramoops_ftrace_size = pdata->ftrace_size;
> >
> >         pr_info("attached 0x%lx@0x%llx, ecc: %d/%d\n", @@ -573,14
> > +631,16 @@ fail_buf:
> >         kfree(cxt->pstore.buf);
> >  fail_clear:
> >         cxt->pstore.bufsize = 0;
> > -       persistent_ram_free(cxt->mprz);
> > -fail_init_mprz:
> > +       ramoops_free_przs(cxt->mprzs);
> > +fail_init_mprzs:
> >         persistent_ram_free(cxt->fprz);
> >  fail_init_fprz:
> >         persistent_ram_free(cxt->cprz);
> >  fail_init_cprz:
> >         cxt->max_dump_cnt = 0;
> >         ramoops_free_przs(cxt->dprzs);
> > +fail_init_dprzs:
> > +       kfree(cxt->pmsg_size);
> >  fail_out:
> >         return err;
> >  }
> > @@ -594,8 +654,9 @@ static int ramoops_remove(struct platform_device
> > *pdev)
> >
> >         kfree(cxt->pstore.buf);
> >         cxt->pstore.bufsize = 0;
> > +       kfree(cxt->pmsg_size);
> >
> > -       persistent_ram_free(cxt->mprz);
> > +       ramoops_free_przs(cxt->mprzs);
> >         persistent_ram_free(cxt->fprz);
> >         persistent_ram_free(cxt->cprz);
> >         ramoops_free_przs(cxt->dprzs); @@ -611,6 +672,38 @@ static
> > struct platform_driver ramoops_driver = {
> >         },
> >  };
> >
> > +static unsigned long *parse_size_str(char *size_str) {
> > +       int i, ret;
> > +       unsigned long *size_array, count = 1;
> > +
> > +       if (size_str) {
> > +               char *s = size_str;
> > +               /* Necessary array size is the number of commas + 1 */
> > +               for (; (s = strchr(s, ',')); s++)
> > +                       count++;
> > +       }
> > +
> > +       size_array = kcalloc(count + 1, sizeof(unsigned long),
> GFP_KERNEL);
> > +       if (!size_array)
> > +               goto out;
> > +
> > +       if (size_str) {
> > +               for (i = 0; i < count; i++) {
> > +                       ret = get_option(&size_str, (int
> *)&size_array[i]);
> > +                       if (ret == 1)
> > +                               break;
> > +                       else if (ret != 2) {
> > +                               size_array[i] = 0;
> > +                               break;
> > +                       }
> > +               }
> > +       } else
> > +               size_array[0] = MIN_MEM_SIZE;
> > +out:
> > +       return size_array;
> > +}
> > +
> >  static void ramoops_register_dummy(void)  {
> >         if (!mem_size)
> > @@ -630,7 +723,9 @@ static void ramoops_register_dummy(void)
> >         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->pmsg_size = parse_size_str(ramoops_pmsg_size_str);
> 
> I think this function is wrong, since it looks like the num_pmsg count a
> bit above expects to find a NULL-terminated list. I think
> parse_size_str() needs to include an extra empty array allocation to make
> sure the list is always NULL-terminated.


OK, I will fix this.

> 
> > +       if (!dummy_data->pmsg_size)
> > +               goto fail_pmsg_size;
> >         dummy_data->dump_oops = dump_oops;
> >         /*
> >          * For backwards compatibility ramoops.ecc=1 means 16 bytes
> > ECC @@ -643,7 +738,13 @@ static void ramoops_register_dummy(void)
> >         if (IS_ERR(dummy)) {
> >                 pr_info("could not create platform device: %ld\n",
> >                         PTR_ERR(dummy));
> > +               goto fail_pdev;
> >         }
> > +       return;
> > +fail_pdev:
> > +       kfree(dummy_data->pmsg_size);
> > +fail_pmsg_size:
> > +       kfree(dummy_data);
> >  }
> >
> >  static int __init ramoops_init(void)
> > @@ -657,6 +758,7 @@ static void __exit ramoops_exit(void)  {
> >         platform_driver_unregister(&ramoops_driver);
> >         platform_device_unregister(dummy);
> > +       kfree(dummy_data->pmsg_size);
> >         kfree(dummy_data);
> >  }
> >  module_exit(ramoops_exit);
> > diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
> > index 4660aaa..af7bbdd 100644
> > --- a/include/linux/pstore_ram.h
> > +++ b/include/linux/pstore_ram.h
> > @@ -72,6 +72,12 @@ ssize_t persistent_ram_ecc_string(struct
> persistent_ram_zone *prz,
> >   * Ramoops platform data
> >   * @mem_size   memory size for ramoops
> >   * @mem_address        physical memory address to contain ramoops
> > + * @pmsg_size  array containing size of each pmsg area. 0 value in the
> array
> > + *             indicates the end of the content. For example, if the
> following
> > + *             array is given,
> > + *                 .pmsg_size = { 0x1000, 0x2000, 0x0, 0x3000, };
> > + *             then, pmsg areas are allocated for the first two size
> values
> > + *             and '0x3000' is just ignored.
> >   */
> >
> >  struct ramoops_platform_data {
> > @@ -81,7 +87,7 @@ struct ramoops_platform_data {
> >         unsigned long   record_size;
> >         unsigned long   console_size;
> >         unsigned long   ftrace_size;
> > -       unsigned long   pmsg_size;
> > +       unsigned long   *pmsg_size;
> >         int             dump_oops;
> >         struct persistent_ram_ecc_info ecc_info;  };
> > --
> > 2.8.1
> >
> >
> 
> -Kees
> 
> --
> Kees Cook
> Nexus Security

Best regards,
  Nobuhiro