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

From: Kees Cook
Date: Thu Sep 08 2016 - 17:53:13 EST

Next message: Kees Cook: "Re: [PATCH v2 5/5] selftests/pstore: add testcases for multiple pmsg instances"
Previous message: Josh Poimboeuf: "[PATCH v2] x86/dumpstack: allow preemption in show_stack_log_lvl() and dump_trace()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

> 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?

> 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.

> + 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

Next message: Kees Cook: "Re: [PATCH v2 5/5] selftests/pstore: add testcases for multiple pmsg instances"
Previous message: Josh Poimboeuf: "[PATCH v2] x86/dumpstack: allow preemption in show_stack_log_lvl() and dump_trace()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]