* [PATCH v2 1/5] ramoops: use persistent_ram_free() instead of kfree() for freeing prz
2016-07-25 3:56 [PATCH v2 0/5] pstore: ramoops: support multiple pmsg instances Nobuhiro Iwamatsu
@ 2016-07-25 3:56 ` Nobuhiro Iwamatsu
2016-07-28 19:35 ` Kees Cook
2016-07-25 3:56 ` [PATCH v2 2/5] ramoops: introduce generic init/free functions for prz Nobuhiro Iwamatsu
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Nobuhiro Iwamatsu @ 2016-07-25 3:56 UTC (permalink / raw)
To: Anton Vorontsov, Colin Cross, Kees Cook, Tony Luck
Cc: linux-kernel, Hiraku Toyooka, Nobuhiro Iwamatsu, Mark Salyzyn,
Seiji Aguchi
From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
persistent_ram_zone(=prz) structures are allocated by persistent_ram_new(),
which includes vmap() or ioremap(). But they are currently freed by
kfree(). This uses persistent_ram_free() for correct this asymmetry usage.
Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
Cc: Mark Salyzyn <salyzyn@android.com>
Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
---
fs/pstore/ram.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index bd9812e..22416c0 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -567,11 +567,11 @@ fail_buf:
kfree(cxt->pstore.buf);
fail_clear:
cxt->pstore.bufsize = 0;
- kfree(cxt->mprz);
+ persistent_ram_free(cxt->mprz);
fail_init_mprz:
- kfree(cxt->fprz);
+ persistent_ram_free(cxt->fprz);
fail_init_fprz:
- kfree(cxt->cprz);
+ persistent_ram_free(cxt->cprz);
fail_init_cprz:
ramoops_free_przs(cxt);
fail_out:
--
2.8.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 1/5] ramoops: use persistent_ram_free() instead of kfree() for freeing prz
2016-07-25 3:56 ` [PATCH v2 1/5] ramoops: use persistent_ram_free() instead of kfree() for freeing prz Nobuhiro Iwamatsu
@ 2016-07-28 19:35 ` Kees Cook
2016-07-29 5:58 ` 岩松信洋 / IWAMATSU，NOBUHIRO
0 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2016-07-28 19:35 UTC (permalink / raw)
To: Nobuhiro Iwamatsu
Cc: Anton Vorontsov, Colin Cross, Tony Luck, LKML, Hiraku Toyooka,
Mark Salyzyn, Seiji Aguchi
On Sun, Jul 24, 2016 at 8:56 PM, Nobuhiro Iwamatsu
<nobuhiro.iwamatsu.kw@hitachi.com> wrote:
> From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
>
> persistent_ram_zone(=prz) structures are allocated by persistent_ram_new(),
> which includes vmap() or ioremap(). But they are currently freed by
> kfree(). This uses persistent_ram_free() for correct this asymmetry usage.
>
> Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
> Cc: Mark Salyzyn <salyzyn@android.com>
> Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
Oh, yes, oops. I may extract this patch and get it into v4.8, since
this is an explicit bug fix. Thanks!
-Kees
> ---
> fs/pstore/ram.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index bd9812e..22416c0 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -567,11 +567,11 @@ fail_buf:
> kfree(cxt->pstore.buf);
> fail_clear:
> cxt->pstore.bufsize = 0;
> - kfree(cxt->mprz);
> + persistent_ram_free(cxt->mprz);
> fail_init_mprz:
> - kfree(cxt->fprz);
> + persistent_ram_free(cxt->fprz);
> fail_init_fprz:
> - kfree(cxt->cprz);
> + persistent_ram_free(cxt->cprz);
> fail_init_cprz:
> ramoops_free_przs(cxt);
> fail_out:
> --
> 2.8.1
>
>
--
Kees Cook
Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2 1/5] ramoops: use persistent_ram_free() instead of kfree() for freeing prz
2016-07-28 19:35 ` Kees Cook
@ 2016-07-29 5:58 ` 岩松信洋 / IWAMATSU，NOBUHIRO
2016-08-03 19:04 ` Kees Cook
0 siblings, 1 reply; 17+ messages in thread
From: 岩松信洋 / IWAMATSU，NOBUHIRO @ 2016-07-29 5:58 UTC (permalink / raw)
To: Kees Cook
Cc: Anton Vorontsov, Colin Cross, Tony Luck, LKML, Mark Salyzyn,
阿口誠司 / AGUCHI，SEIJI,
ltc-kernel,
森谷真寿美 /
MORITANI，MASUMI, ltc-kernel
Hi,
> -----Original Message-----
> From: keescook@google.com [mailto:keescook@google.com] On Behalf Of Kees
> Cook
> Sent: Friday, July 29, 2016 4:35 AM
> To: 岩松信洋 / IWAMATSU，NOBUHIRO
> Cc: Anton Vorontsov; Colin Cross; Tony Luck; LKML; Hiraku Toyooka; Mark
> Salyzyn; 阿口誠司 / AGUCHI，SEIJI
> Subject: Re: [PATCH v2 1/5] ramoops: use persistent_ram_free() instead of
> kfree() for freeing prz
>
> On Sun, Jul 24, 2016 at 8:56 PM, Nobuhiro Iwamatsu
> <nobuhiro.iwamatsu.kw@hitachi.com> wrote:
> > From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> >
> > persistent_ram_zone(=prz) structures are allocated by
> > persistent_ram_new(), which includes vmap() or ioremap(). But they are
> > currently freed by kfree(). This uses persistent_ram_free() for correct
> this asymmetry usage.
> >
> > Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
> > Cc: Mark Salyzyn <salyzyn@android.com>
> > Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
>
> Oh, yes, oops. I may extract this patch and get it into v4.8, since this
> is an explicit bug fix. Thanks!
>
> -Kees
Thanks. Please pickup this commit as bug fix.
And if you have a time, please review other patches in this patch series.
Best regards,
Nobuhiro
>
> > ---
> > fs/pstore/ram.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c index bd9812e..22416c0
> > 100644
> > --- a/fs/pstore/ram.c
> > +++ b/fs/pstore/ram.c
> > @@ -567,11 +567,11 @@ fail_buf:
> > kfree(cxt->pstore.buf);
> > fail_clear:
> > cxt->pstore.bufsize = 0;
> > - kfree(cxt->mprz);
> > + persistent_ram_free(cxt->mprz);
> > fail_init_mprz:
> > - kfree(cxt->fprz);
> > + persistent_ram_free(cxt->fprz);
> > fail_init_fprz:
> > - kfree(cxt->cprz);
> > + persistent_ram_free(cxt->cprz);
> > fail_init_cprz:
> > ramoops_free_przs(cxt);
> > fail_out:
> > --
> > 2.8.1
> >
> >
>
>
>
> --
> Kees Cook
> Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/5] ramoops: use persistent_ram_free() instead of kfree() for freeing prz
2016-07-29 5:58 ` 岩松信洋 / IWAMATSU，NOBUHIRO
@ 2016-08-03 19:04 ` Kees Cook
0 siblings, 0 replies; 17+ messages in thread
From: Kees Cook @ 2016-08-03 19:04 UTC (permalink / raw)
To: 岩松信洋 / IWAMATSU，NOBUHIRO
Cc: Anton Vorontsov, Colin Cross, Tony Luck, LKML, Mark Salyzyn,
阿口誠司 / AGUCHI，SEIJI,
ltc-kernel,
森谷真寿美 /
MORITANI，MASUMI
On Thu, Jul 28, 2016 at 10:58 PM, 岩松信洋 / IWAMATSU，NOBUHIRO
<nobuhiro.iwamatsu.kw@hitachi.com> wrote:
> Hi,
>
>> -----Original Message-----
>> From: keescook@google.com [mailto:keescook@google.com] On Behalf Of Kees
>> Cook
>> Sent: Friday, July 29, 2016 4:35 AM
>> To: 岩松信洋 / IWAMATSU，NOBUHIRO
>> Cc: Anton Vorontsov; Colin Cross; Tony Luck; LKML; Hiraku Toyooka; Mark
>> Salyzyn; 阿口誠司 / AGUCHI，SEIJI
>> Subject: Re: [PATCH v2 1/5] ramoops: use persistent_ram_free() instead of
>> kfree() for freeing prz
>>
>> On Sun, Jul 24, 2016 at 8:56 PM, Nobuhiro Iwamatsu
>> <nobuhiro.iwamatsu.kw@hitachi.com> wrote:
>> > From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
>> >
>> > persistent_ram_zone(=prz) structures are allocated by
>> > persistent_ram_new(), which includes vmap() or ioremap(). But they are
>> > currently freed by kfree(). This uses persistent_ram_free() for correct
>> this asymmetry usage.
>> >
>> > Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
>> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
>> > Cc: Mark Salyzyn <salyzyn@android.com>
>> > Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
>>
>> Oh, yes, oops. I may extract this patch and get it into v4.8, since this
>> is an explicit bug fix. Thanks!
>>
>> -Kees
>
> Thanks. Please pickup this commit as bug fix.
Applied, this should get picked up soon.
> And if you have a time, please review other patches in this patch series.
For sure; I'm busy with the 4.8 merge window being open right now, but
after that I'll get them reviewed.
Thanks!
-Kees
>
> Best regards,
> Nobuhiro
>
>>
>> > ---
>> > fs/pstore/ram.c | 6 +++---
>> > 1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c index bd9812e..22416c0
>> > 100644
>> > --- a/fs/pstore/ram.c
>> > +++ b/fs/pstore/ram.c
>> > @@ -567,11 +567,11 @@ fail_buf:
>> > kfree(cxt->pstore.buf);
>> > fail_clear:
>> > cxt->pstore.bufsize = 0;
>> > - kfree(cxt->mprz);
>> > + persistent_ram_free(cxt->mprz);
>> > fail_init_mprz:
>> > - kfree(cxt->fprz);
>> > + persistent_ram_free(cxt->fprz);
>> > fail_init_fprz:
>> > - kfree(cxt->cprz);
>> > + persistent_ram_free(cxt->cprz);
>> > fail_init_cprz:
>> > ramoops_free_przs(cxt);
>> > fail_out:
>> > --
>> > 2.8.1
>> >
>> >
>>
>>
>>
>> --
>> Kees Cook
>> Chrome OS & Brillo Security
--
Kees Cook
Brillo & Chrome OS Security
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 2/5] ramoops: introduce generic init/free functions for prz
2016-07-25 3:56 [PATCH v2 0/5] pstore: ramoops: support multiple pmsg instances Nobuhiro Iwamatsu
2016-07-25 3:56 ` [PATCH v2 1/5] ramoops: use persistent_ram_free() instead of kfree() for freeing prz Nobuhiro Iwamatsu
@ 2016-07-25 3:56 ` Nobuhiro Iwamatsu
2016-09-08 21:22 ` Kees Cook
2016-07-25 3:56 ` [PATCH v2 3/5] pstore: support multiple pmsg instances Nobuhiro Iwamatsu
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Nobuhiro Iwamatsu @ 2016-07-25 3:56 UTC (permalink / raw)
To: Anton Vorontsov, Colin Cross, Kees Cook, Tony Luck
Cc: linux-kernel, Hiraku Toyooka, Nobuhiro Iwamatsu, Mark Salyzyn,
Seiji Aguchi
From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
We modifies initialization and freeing code for prz for generic usage.
This change
* add generic function __ramoops_init_prz() to reduce redundancy
between ramoops_init_prz() and ramoops_init_przs().
* rename 'przs' member in struct ramoops_context to 'dprzs' so that
it stands for 'dump przs'.
* rename ramoops_init_prz() to ramoops_init_dprzs().
* change parameter of ramoops_free_przs() from struct ramoops_context *
into struct persistent_ram_zone * in order to make it available for
all prz array.
Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
Cc: Mark Salyzyn <salyzyn@android.com>
Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
---
fs/pstore/ram.c | 65 ++++++++++++++++++++++++++++++++-------------------------
1 file changed, 36 insertions(+), 29 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 22416c0..288c5d0 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -83,7 +83,7 @@ MODULE_PARM_DESC(ramoops_ecc,
"bytes ECC)");
struct ramoops_context {
- struct persistent_ram_zone **przs;
+ struct persistent_ram_zone **dprzs;
struct persistent_ram_zone *cprz;
struct persistent_ram_zone *fprz;
struct persistent_ram_zone *mprz;
@@ -199,7 +199,7 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
/* Find the next valid persistent_ram_zone for DMESG */
while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
- prz = ramoops_get_next_prz(cxt->przs, &cxt->dump_read_cnt,
+ prz = ramoops_get_next_prz(cxt->dprzs, &cxt->dump_read_cnt,
cxt->max_dump_cnt, id, type,
PSTORE_TYPE_DMESG, 1);
if (!prz_ok(prz))
@@ -314,10 +314,10 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
if (part != 1)
return -ENOSPC;
- if (!cxt->przs)
+ if (!cxt->dprzs)
return -ENOSPC;
- prz = cxt->przs[cxt->dump_write_cnt];
+ prz = cxt->dprzs[cxt->dump_write_cnt];
hlen = ramoops_write_kmsg_hdr(prz, compressed);
if (size + hlen > prz->buffer_size)
@@ -339,7 +339,7 @@ static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, int count,
case PSTORE_TYPE_DMESG:
if (id >= cxt->max_dump_cnt)
return -EINVAL;
- prz = cxt->przs[id];
+ prz = cxt->dprzs[id];
break;
case PSTORE_TYPE_CONSOLE:
prz = cxt->cprz;
@@ -371,21 +371,24 @@ static struct ramoops_context oops_cxt = {
},
};
-static void ramoops_free_przs(struct ramoops_context *cxt)
+static void ramoops_free_przs(struct persistent_ram_zone **przs)
{
int i;
- cxt->max_dump_cnt = 0;
- if (!cxt->przs)
+ if (!przs)
return;
- for (i = 0; !IS_ERR_OR_NULL(cxt->przs[i]); i++)
- persistent_ram_free(cxt->przs[i]);
- kfree(cxt->przs);
+ for (i = 0; i < !IS_ERR_OR_NULL(przs[i]); i++)
+ persistent_ram_free(przs[i]);
+ kfree(przs);
}
-static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
- phys_addr_t *paddr, size_t dump_mem_sz)
+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, bool zap);
+
+static int ramoops_init_dprzs(struct device *dev, struct ramoops_context *cxt,
+ phys_addr_t *paddr, size_t dump_mem_sz)
{
int err = -ENOMEM;
int i;
@@ -402,29 +405,24 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
if (!cxt->max_dump_cnt)
return -ENOMEM;
- cxt->przs = kzalloc(sizeof(*cxt->przs) * cxt->max_dump_cnt,
+ cxt->dprzs = kcalloc(cxt->max_dump_cnt, sizeof(*cxt->dprzs),
GFP_KERNEL);
- if (!cxt->przs) {
+ if (!cxt->dprzs) {
dev_err(dev, "failed to initialize a prz array for dumps\n");
goto fail_prz;
}
for (i = 0; i < cxt->max_dump_cnt; i++) {
- cxt->przs[i] = persistent_ram_new(*paddr, cxt->record_size, 0,
- &cxt->ecc_info,
- cxt->memtype);
- if (IS_ERR(cxt->przs[i])) {
- err = PTR_ERR(cxt->przs[i]);
- dev_err(dev, "failed to request mem region (0x%zx@0x%llx): %d\n",
- cxt->record_size, (unsigned long long)*paddr, err);
+ err = __ramoops_init_prz(dev, cxt, &cxt->dprzs[i], paddr,
+ cxt->record_size, 0, false);
+ if (err)
goto fail_prz;
- }
- *paddr += cxt->record_size;
}
return 0;
fail_prz:
- ramoops_free_przs(cxt);
+ cxt->max_dump_cnt = 0;
+ ramoops_free_przs(cxt->dprzs);
return err;
}
@@ -432,6 +430,13 @@ 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)
{
+ return __ramoops_init_prz(dev, cxt, prz, paddr, sz, sig, true);
+}
+
+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, bool zap)
+{
if (!sz)
return 0;
@@ -451,7 +456,8 @@ static int ramoops_init_prz(struct device *dev, struct ramoops_context *cxt,
return err;
}
- persistent_ram_zap(*prz);
+ if (zap)
+ persistent_ram_zap(*prz);
*paddr += sz;
@@ -503,7 +509,7 @@ static int ramoops_probe(struct platform_device *pdev)
dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size
- cxt->pmsg_size;
- err = ramoops_init_przs(dev, cxt, &paddr, dump_mem_sz);
+ err = ramoops_init_dprzs(dev, cxt, &paddr, dump_mem_sz);
if (err)
goto fail_out;
@@ -573,7 +579,8 @@ fail_init_mprz:
fail_init_fprz:
persistent_ram_free(cxt->cprz);
fail_init_cprz:
- ramoops_free_przs(cxt);
+ cxt->max_dump_cnt = 0;
+ ramoops_free_przs(cxt->dprzs);
fail_out:
return err;
}
@@ -591,7 +598,7 @@ static int ramoops_remove(struct platform_device *pdev)
persistent_ram_free(cxt->mprz);
persistent_ram_free(cxt->fprz);
persistent_ram_free(cxt->cprz);
- ramoops_free_przs(cxt);
+ ramoops_free_przs(cxt->dprzs);
return 0;
}
--
2.8.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 2/5] ramoops: introduce generic init/free functions for prz
2016-07-25 3:56 ` [PATCH v2 2/5] ramoops: introduce generic init/free functions for prz Nobuhiro Iwamatsu
@ 2016-09-08 21:22 ` Kees Cook
2016-10-05 4:06 ` 岩松信洋 / IWAMATSU，NOBUHIRO
0 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2016-09-08 21:22 UTC (permalink / raw)
To: Nobuhiro Iwamatsu
Cc: Anton Vorontsov, Colin Cross, Tony Luck, LKML, Hiraku Toyooka,
Mark Salyzyn, Seiji Aguchi
On Sun, Jul 24, 2016 at 8:56 PM, Nobuhiro Iwamatsu
<nobuhiro.iwamatsu.kw@hitachi.com> wrote:
> From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
>
> We modifies initialization and freeing code for prz for generic usage.
Sorry for the delay in getting to this review, I've been catching up
on pstore finally. :)
> This change
>
> * add generic function __ramoops_init_prz() to reduce redundancy
> between ramoops_init_prz() and ramoops_init_przs().
Can you split this into a separate patch?
> * rename 'przs' member in struct ramoops_context to 'dprzs' so that
> it stands for 'dump przs'.
> * rename ramoops_init_prz() to ramoops_init_dprzs().
And also these two into a separate patch, since it's just a renaming.
And could you add comments for all the przs, it's getting harder to
read these since they're just single-letter names. :)
> * change parameter of ramoops_free_przs() from struct ramoops_context *
> into struct persistent_ram_zone * in order to make it available for
> all prz array.
I *think* this should be with the first change, so splitting this
email's patch into two patches would make review easier (i.e. first do
renamings, then make functional changes).
> Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
> Cc: Mark Salyzyn <salyzyn@android.com>
> Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
> ---
> fs/pstore/ram.c | 65 ++++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 36 insertions(+), 29 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 22416c0..288c5d0 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -83,7 +83,7 @@ MODULE_PARM_DESC(ramoops_ecc,
> "bytes ECC)");
>
> struct ramoops_context {
> - struct persistent_ram_zone **przs;
> + struct persistent_ram_zone **dprzs;
> struct persistent_ram_zone *cprz;
> struct persistent_ram_zone *fprz;
> struct persistent_ram_zone *mprz;
> @@ -199,7 +199,7 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
>
> /* Find the next valid persistent_ram_zone for DMESG */
> while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
> - prz = ramoops_get_next_prz(cxt->przs, &cxt->dump_read_cnt,
> + prz = ramoops_get_next_prz(cxt->dprzs, &cxt->dump_read_cnt,
> cxt->max_dump_cnt, id, type,
> PSTORE_TYPE_DMESG, 1);
> if (!prz_ok(prz))
> @@ -314,10 +314,10 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
> if (part != 1)
> return -ENOSPC;
>
> - if (!cxt->przs)
> + if (!cxt->dprzs)
> return -ENOSPC;
>
> - prz = cxt->przs[cxt->dump_write_cnt];
> + prz = cxt->dprzs[cxt->dump_write_cnt];
>
> hlen = ramoops_write_kmsg_hdr(prz, compressed);
> if (size + hlen > prz->buffer_size)
> @@ -339,7 +339,7 @@ static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, int count,
> case PSTORE_TYPE_DMESG:
> if (id >= cxt->max_dump_cnt)
> return -EINVAL;
> - prz = cxt->przs[id];
> + prz = cxt->dprzs[id];
> break;
> case PSTORE_TYPE_CONSOLE:
> prz = cxt->cprz;
> @@ -371,21 +371,24 @@ static struct ramoops_context oops_cxt = {
> },
> };
>
> -static void ramoops_free_przs(struct ramoops_context *cxt)
> +static void ramoops_free_przs(struct persistent_ram_zone **przs)
> {
> int i;
>
> - cxt->max_dump_cnt = 0;
> - if (!cxt->przs)
> + if (!przs)
> return;
>
> - for (i = 0; !IS_ERR_OR_NULL(cxt->przs[i]); i++)
> - persistent_ram_free(cxt->przs[i]);
> - kfree(cxt->przs);
> + for (i = 0; i < !IS_ERR_OR_NULL(przs[i]); i++)
> + persistent_ram_free(przs[i]);
> + kfree(przs);
> }
>
> -static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
> - phys_addr_t *paddr, size_t dump_mem_sz)
> +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, bool zap);
> +
> +static int ramoops_init_dprzs(struct device *dev, struct ramoops_context *cxt,
> + phys_addr_t *paddr, size_t dump_mem_sz)
> {
> int err = -ENOMEM;
> int i;
> @@ -402,29 +405,24 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
> if (!cxt->max_dump_cnt)
> return -ENOMEM;
>
> - cxt->przs = kzalloc(sizeof(*cxt->przs) * cxt->max_dump_cnt,
> + cxt->dprzs = kcalloc(cxt->max_dump_cnt, sizeof(*cxt->dprzs),
> GFP_KERNEL);
> - if (!cxt->przs) {
> + if (!cxt->dprzs) {
> dev_err(dev, "failed to initialize a prz array for dumps\n");
> goto fail_prz;
> }
>
> for (i = 0; i < cxt->max_dump_cnt; i++) {
> - cxt->przs[i] = persistent_ram_new(*paddr, cxt->record_size, 0,
> - &cxt->ecc_info,
> - cxt->memtype);
> - if (IS_ERR(cxt->przs[i])) {
> - err = PTR_ERR(cxt->przs[i]);
> - dev_err(dev, "failed to request mem region (0x%zx@0x%llx): %d\n",
> - cxt->record_size, (unsigned long long)*paddr, err);
> + err = __ramoops_init_prz(dev, cxt, &cxt->dprzs[i], paddr,
> + cxt->record_size, 0, false);
> + if (err)
> goto fail_prz;
> - }
> - *paddr += cxt->record_size;
> }
>
> return 0;
> fail_prz:
> - ramoops_free_przs(cxt);
> + cxt->max_dump_cnt = 0;
> + ramoops_free_przs(cxt->dprzs);
And this will need rebasing on the next -next (since the "free" path
has been fixed up to do the right thing):
http://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/commit/?h=for-next/pstore&id=a4dd738c8e0503457902ffb2b742a07c9acbc98b
> return err;
> }
>
> @@ -432,6 +430,13 @@ 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)
> {
> + return __ramoops_init_prz(dev, cxt, prz, paddr, sz, sig, true);
> +}
> +
> +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, bool zap)
> +{
> if (!sz)
> return 0;
>
> @@ -451,7 +456,8 @@ static int ramoops_init_prz(struct device *dev, struct ramoops_context *cxt,
> return err;
> }
>
> - persistent_ram_zap(*prz);
> + if (zap)
> + persistent_ram_zap(*prz);
>
> *paddr += sz;
>
> @@ -503,7 +509,7 @@ static int ramoops_probe(struct platform_device *pdev)
>
> dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size
> - cxt->pmsg_size;
> - err = ramoops_init_przs(dev, cxt, &paddr, dump_mem_sz);
> + err = ramoops_init_dprzs(dev, cxt, &paddr, dump_mem_sz);
> if (err)
> goto fail_out;
>
> @@ -573,7 +579,8 @@ fail_init_mprz:
> fail_init_fprz:
> persistent_ram_free(cxt->cprz);
> fail_init_cprz:
> - ramoops_free_przs(cxt);
> + cxt->max_dump_cnt = 0;
> + ramoops_free_przs(cxt->dprzs);
> fail_out:
> return err;
> }
> @@ -591,7 +598,7 @@ static int ramoops_remove(struct platform_device *pdev)
> persistent_ram_free(cxt->mprz);
> persistent_ram_free(cxt->fprz);
> persistent_ram_free(cxt->cprz);
> - ramoops_free_przs(cxt);
> + ramoops_free_przs(cxt->dprzs);
>
> return 0;
> }
> --
> 2.8.1
>
>
Thanks!
-Kees
--
Kees Cook
Nexus Security
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: Re: [PATCH v2 2/5] ramoops: introduce generic init/free functions for prz
2016-09-08 21:22 ` Kees Cook
@ 2016-10-05 4:06 ` 岩松信洋 / IWAMATSU，NOBUHIRO
0 siblings, 0 replies; 17+ messages in thread
From: 岩松信洋 / IWAMATSU，NOBUHIRO @ 2016-10-05 4:06 UTC (permalink / raw)
To: Kees Cook
Cc: Anton Vorontsov, Colin Cross, Tony Luck, LKML, Hiraku Toyooka,
Mark Salyzyn,
阿口誠司 / AGUCHI，SEIJI
Hi,
Thanks for youre review.
> -----Original Message-----
> From: keescook@google.com [mailto:keescook@google.com] On Behalf Of Kees
> Cook
> Sent: Friday, September 09, 2016 6:23 AM
> To: 岩松信洋 / IWAMATSU，NOBUHIRO
> Cc: Anton Vorontsov; Colin Cross; Tony Luck; LKML; Hiraku Toyooka; Mark
> Salyzyn; 阿口誠司 / AGUCHI，SEIJI
> Subject: [!]Re: [PATCH v2 2/5] ramoops: introduce generic init/free
> functions for prz
>
> On Sun, Jul 24, 2016 at 8:56 PM, Nobuhiro Iwamatsu
> <nobuhiro.iwamatsu.kw@hitachi.com> wrote:
> > From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> >
> > We modifies initialization and freeing code for prz for generic usage.
>
> Sorry for the delay in getting to this review, I've been catching up on
> pstore finally. :)
>
> > This change
> >
> > * add generic function __ramoops_init_prz() to reduce redundancy
> > between ramoops_init_prz() and ramoops_init_przs().
>
> Can you split this into a separate patch?
OK, I will do.
>
> > * rename 'przs' member in struct ramoops_context to 'dprzs' so that
> > it stands for 'dump przs'.
> > * rename ramoops_init_prz() to ramoops_init_dprzs().
>
> And also these two into a separate patch, since it's just a renaming.
> And could you add comments for all the przs, it's getting harder to read
> these since they're just single-letter names. :)
>
> > * change parameter of ramoops_free_przs() from struct ramoops_context
> *
> > into struct persistent_ram_zone * in order to make it available for
> > all prz array.
>
> I *think* this should be with the first change, so splitting this email's
> patch into two patches would make review easier (i.e. first do renamings,
> then make functional changes).
OK, I will do too.
>
> > Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
> > Cc: Mark Salyzyn <salyzyn@android.com>
> > Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
<snip>
>
> Thanks!
>
> -Kees
>
> --
> Kees Cook
> Nexus Security
Best regards,
Nobuhiro
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 3/5] pstore: support multiple pmsg instances
2016-07-25 3:56 [PATCH v2 0/5] pstore: ramoops: support multiple pmsg instances Nobuhiro Iwamatsu
2016-07-25 3:56 ` [PATCH v2 1/5] ramoops: use persistent_ram_free() instead of kfree() for freeing prz Nobuhiro Iwamatsu
2016-07-25 3:56 ` [PATCH v2 2/5] ramoops: introduce generic init/free functions for prz Nobuhiro Iwamatsu
@ 2016-07-25 3:56 ` Nobuhiro Iwamatsu
2016-09-08 21:33 ` Kees Cook
2016-07-25 3:56 ` [PATCH v2 4/5] ramoops: " Nobuhiro Iwamatsu
2016-07-25 3:56 ` [PATCH v2 5/5] selftests/pstore: add testcases for " Nobuhiro Iwamatsu
4 siblings, 1 reply; 17+ messages in thread
From: Nobuhiro Iwamatsu @ 2016-07-25 3:56 UTC (permalink / raw)
To: Anton Vorontsov, Colin Cross, Kees Cook, Tony Luck
Cc: linux-kernel, Hiraku Toyooka, Nobuhiro Iwamatsu, Mark Salyzyn,
Seiji Aguchi
From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
This enables pmsg to deal with multiple instances. One possible
use is content priority control on limited persistent store space. By
using different buffers, we can prevent important messages from being
overwritten by less important messages.
When pstore backend module specifies the number of instances (num_pmsg)
in pstore_info, multiple /dev/pmsg[ID] appear. (ID is an integer
starting at 0. It corresponds to minor number of the each char device.)
Writes to each /dev/pmsg[ID] are isolated each other. After reboot, the
contents are available in /sys/fs/pstore/pmsg-[backend]-[ID].
Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
Cc: Mark Salyzyn <salyzyn@android.com>
Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
---
fs/pstore/pmsg.c | 20 ++++++++++++++++++--
include/linux/pstore.h | 1 +
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
index 7de20cd..2e281ed 100644
--- a/fs/pstore/pmsg.c
+++ b/fs/pstore/pmsg.c
@@ -52,8 +52,8 @@ static ssize_t write_pmsg(struct file *file, const char __user *buf,
vfree(buffer);
return -EFAULT;
}
- psinfo->write_buf(PSTORE_TYPE_PMSG, 0, &id, 0, buffer, 0, c,
- psinfo);
+ psinfo->write_buf(PSTORE_TYPE_PMSG, 0, &id,
+ iminor(file->f_inode), buffer, 0, c, psinfo);
i += c;
}
@@ -85,6 +85,7 @@ static char *pmsg_devnode(struct device *dev, umode_t *mode)
void pstore_register_pmsg(void)
{
struct device *pmsg_device;
+ int i = 0;
pmsg_major = register_chrdev(0, PMSG_NAME, &pmsg_fops);
if (pmsg_major < 0) {
@@ -105,9 +106,24 @@ void pstore_register_pmsg(void)
pr_err("failed to create device\n");
goto err_device;
}
+
+ /* allocate additional /dev/pmsg[ID] */
+ for (i = 1; i < psinfo->num_pmsg; i++) {
+ struct device *pmsg_device;
+
+ pmsg_device = device_create(pmsg_class, NULL,
+ MKDEV(pmsg_major, i), NULL, "%s%d",
+ PMSG_NAME, i);
+ if (IS_ERR(pmsg_device)) {
+ pr_err("failed to create device\n");
+ goto err_device;
+ }
+ }
return;
err_device:
+ for (i--; i >= 0; i--)
+ device_destroy(pmsg_class, MKDEV(pmsg_major, i));
class_destroy(pmsg_class);
err_class:
unregister_chrdev(pmsg_major, PMSG_NAME);
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 831479f..b0c24cc 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -54,6 +54,7 @@ struct pstore_info {
size_t bufsize;
struct mutex read_mutex; /* serialize open/read/close */
int flags;
+ unsigned int num_pmsg;
int (*open)(struct pstore_info *psi);
int (*close)(struct pstore_info *psi);
ssize_t (*read)(u64 *id, enum pstore_type_id *type,
--
2.8.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 3/5] pstore: support multiple pmsg instances
2016-07-25 3:56 ` [PATCH v2 3/5] pstore: support multiple pmsg instances Nobuhiro Iwamatsu
@ 2016-09-08 21:33 ` Kees Cook
2016-10-05 4:09 ` 岩松信洋 / IWAMATSU，NOBUHIRO
0 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2016-09-08 21:33 UTC (permalink / raw)
To: Nobuhiro Iwamatsu
Cc: Anton Vorontsov, Colin Cross, Tony Luck, LKML, Hiraku Toyooka,
Mark Salyzyn, Seiji Aguchi
On Sun, Jul 24, 2016 at 8:56 PM, Nobuhiro Iwamatsu
<nobuhiro.iwamatsu.kw@hitachi.com> wrote:
> From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
>
> This enables pmsg to deal with multiple instances. One possible
> use is content priority control on limited persistent store space. By
> using different buffers, we can prevent important messages from being
> overwritten by less important messages.
>
> When pstore backend module specifies the number of instances (num_pmsg)
> in pstore_info, multiple /dev/pmsg[ID] appear. (ID is an integer
> starting at 0. It corresponds to minor number of the each char device.)
> Writes to each /dev/pmsg[ID] are isolated each other. After reboot, the
> contents are available in /sys/fs/pstore/pmsg-[backend]-[ID].
>
> Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
> Cc: Mark Salyzyn <salyzyn@android.com>
> Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
> ---
> fs/pstore/pmsg.c | 20 ++++++++++++++++++--
> include/linux/pstore.h | 1 +
> 2 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
> index 7de20cd..2e281ed 100644
> --- a/fs/pstore/pmsg.c
> +++ b/fs/pstore/pmsg.c
> @@ -52,8 +52,8 @@ static ssize_t write_pmsg(struct file *file, const char __user *buf,
> vfree(buffer);
> return -EFAULT;
> }
> - psinfo->write_buf(PSTORE_TYPE_PMSG, 0, &id, 0, buffer, 0, c,
> - psinfo);
> + psinfo->write_buf(PSTORE_TYPE_PMSG, 0, &id,
> + iminor(file->f_inode), buffer, 0, c, psinfo);
>
> i += c;
> }
> @@ -85,6 +85,7 @@ static char *pmsg_devnode(struct device *dev, umode_t *mode)
> void pstore_register_pmsg(void)
> {
> struct device *pmsg_device;
> + int i = 0;
>
> pmsg_major = register_chrdev(0, PMSG_NAME, &pmsg_fops);
> if (pmsg_major < 0) {
> @@ -105,9 +106,24 @@ void pstore_register_pmsg(void)
> pr_err("failed to create device\n");
> goto err_device;
> }
> +
> + /* allocate additional /dev/pmsg[ID] */
> + for (i = 1; i < psinfo->num_pmsg; i++) {
> + struct device *pmsg_device;
> +
> + pmsg_device = device_create(pmsg_class, NULL,
> + MKDEV(pmsg_major, i), NULL, "%s%d",
> + PMSG_NAME, i);
> + if (IS_ERR(pmsg_device)) {
> + pr_err("failed to create device\n");
> + goto err_device;
> + }
> + }
This should just be merged into the first device_create call (i.e.
drop the first one, keep your loop, but have "i" start at 0). There's
no reason that I can see to do it separately.
> return;
>
> err_device:
> + for (i--; i >= 0; i--)
> + device_destroy(pmsg_class, MKDEV(pmsg_major, i));
Was the device_destroy() for /dev/pmsg0 missing before? (i.e. does
class_destroy() or unregister_chrdev() already clean up the devices?)
> class_destroy(pmsg_class);
> err_class:
> unregister_chrdev(pmsg_major, PMSG_NAME);
> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> index 831479f..b0c24cc 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -54,6 +54,7 @@ struct pstore_info {
> size_t bufsize;
> struct mutex read_mutex; /* serialize open/read/close */
> int flags;
> + unsigned int num_pmsg;
> int (*open)(struct pstore_info *psi);
> int (*close)(struct pstore_info *psi);
> ssize_t (*read)(u64 *id, enum pstore_type_id *type,
> --
> 2.8.1
>
>
-Kees
--
Kees Cook
Nexus Security
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH v2 3/5] pstore: support multiple pmsg instances
2016-09-08 21:33 ` Kees Cook
@ 2016-10-05 4:09 ` 岩松信洋 / IWAMATSU，NOBUHIRO
0 siblings, 0 replies; 17+ messages in thread
From: 岩松信洋 / IWAMATSU，NOBUHIRO @ 2016-10-05 4:09 UTC (permalink / raw)
To: Kees Cook
Cc: Anton Vorontsov, Colin Cross, Tony Luck, LKML, Mark Salyzyn,
阿口誠司 / AGUCHI，SEIJI
Hi,
> -----Original Message-----
> From: keescook@google.com [mailto:keescook@google.com] On Behalf Of Kees
> Cook
> Sent: Friday, September 09, 2016 6:34 AM
> To: 岩松信洋 / IWAMATSU，NOBUHIRO
> Cc: Anton Vorontsov; Colin Cross; Tony Luck; LKML; Hiraku Toyooka; Mark
> Salyzyn; 阿口誠司 / AGUCHI，SEIJI
> Subject: Re: [PATCH v2 3/5] pstore: support multiple pmsg instances
>
> On Sun, Jul 24, 2016 at 8:56 PM, Nobuhiro Iwamatsu
> <nobuhiro.iwamatsu.kw@hitachi.com> wrote:
> > From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> >
> > This enables pmsg to deal with multiple instances. One possible use is
> > content priority control on limited persistent store space. By using
> > different buffers, we can prevent important messages from being
> > overwritten by less important messages.
> >
> > When pstore backend module specifies the number of instances
> > (num_pmsg) in pstore_info, multiple /dev/pmsg[ID] appear. (ID is an
> > integer starting at 0. It corresponds to minor number of the each char
> > device.) Writes to each /dev/pmsg[ID] are isolated each other. After
> > reboot, the contents are available in
> /sys/fs/pstore/pmsg-[backend]-[ID].
> >
> > Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
> > Cc: Mark Salyzyn <salyzyn@android.com>
> > Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
> > ---
> > fs/pstore/pmsg.c | 20 ++++++++++++++++++--
> > include/linux/pstore.h | 1 +
> > 2 files changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c index
> > 7de20cd..2e281ed 100644
> > --- a/fs/pstore/pmsg.c
> > +++ b/fs/pstore/pmsg.c
> > @@ -52,8 +52,8 @@ static ssize_t write_pmsg(struct file *file, const char
> __user *buf,
> > vfree(buffer);
> > return -EFAULT;
> > }
> > - psinfo->write_buf(PSTORE_TYPE_PMSG, 0, &id, 0, buffer,
> 0, c,
> > - psinfo);
> > + psinfo->write_buf(PSTORE_TYPE_PMSG, 0, &id,
> > + iminor(file->f_inode), buffer, 0, c,
> > + psinfo);
> >
> > i += c;
> > }
> > @@ -85,6 +85,7 @@ static char *pmsg_devnode(struct device *dev,
> > umode_t *mode) void pstore_register_pmsg(void) {
> > struct device *pmsg_device;
> > + int i = 0;
> >
> > pmsg_major = register_chrdev(0, PMSG_NAME, &pmsg_fops);
> > if (pmsg_major < 0) {
> > @@ -105,9 +106,24 @@ void pstore_register_pmsg(void)
> > pr_err("failed to create device\n");
> > goto err_device;
> > }
> > +
> > + /* allocate additional /dev/pmsg[ID] */
> > + for (i = 1; i < psinfo->num_pmsg; i++) {
> > + struct device *pmsg_device;
> > +
> > + pmsg_device = device_create(pmsg_class, NULL,
> > + MKDEV(pmsg_major, i), NULL,
> "%s%d",
> > + PMSG_NAME, i);
> > + if (IS_ERR(pmsg_device)) {
> > + pr_err("failed to create device\n");
> > + goto err_device;
> > + }
> > + }
>
> This should just be merged into the first device_create call (i.e.
> drop the first one, keep your loop, but have "i" start at 0). There's no
> reason that I can see to do it separately.
Indeed, I will fix.
>
> > return;
> >
> > err_device:
> > + for (i--; i >= 0; i--)
> > + device_destroy(pmsg_class, MKDEV(pmsg_major, i));
>
> Was the device_destroy() for /dev/pmsg0 missing before? (i.e. does
> class_destroy() or unregister_chrdev() already clean up the devices?)
Your are right.
I will fix as /dev/pmsg0 also treated with device_destroy().
>
> > class_destroy(pmsg_class);
> > err_class:
> > unregister_chrdev(pmsg_major, PMSG_NAME); diff --git
> > a/include/linux/pstore.h b/include/linux/pstore.h index
> > 831479f..b0c24cc 100644
> > --- a/include/linux/pstore.h
> > +++ b/include/linux/pstore.h
> > @@ -54,6 +54,7 @@ struct pstore_info {
> > size_t bufsize;
> > struct mutex read_mutex; /* serialize open/read/close
> */
> > int flags;
> > + unsigned int num_pmsg;
> > int (*open)(struct pstore_info *psi);
> > int (*close)(struct pstore_info *psi);
> > ssize_t (*read)(u64 *id, enum pstore_type_id *type,
> > --
> > 2.8.1
> >
> >
>
> -Kees
>
>
> --
> Kees Cook
> Nexus Security
Best regards,
Nobuhiro
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 4/5] ramoops: support multiple pmsg instances
2016-07-25 3:56 [PATCH v2 0/5] pstore: ramoops: support multiple pmsg instances Nobuhiro Iwamatsu
` (2 preceding siblings ...)
2016-07-25 3:56 ` [PATCH v2 3/5] pstore: support multiple pmsg instances Nobuhiro Iwamatsu
@ 2016-07-25 3:56 ` Nobuhiro Iwamatsu
2016-09-08 21:53 ` Kees Cook
2016-07-25 3:56 ` [PATCH v2 5/5] selftests/pstore: add testcases for " Nobuhiro Iwamatsu
4 siblings, 1 reply; 17+ messages in thread
From: Nobuhiro Iwamatsu @ 2016-07-25 3:56 UTC (permalink / raw)
To: Anton Vorontsov, Colin Cross, Kees Cook, Tony Luck
Cc: linux-kernel, Hiraku Toyooka, Nobuhiro Iwamatsu, Mark Salyzyn,
Seiji Aguchi
From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
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@hitachi.com>
Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
Cc: Mark Salyzyn <salyzyn@android.com>
Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
---
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.
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;
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);
+ 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 4/5] ramoops: support multiple pmsg instances
2016-07-25 3:56 ` [PATCH v2 4/5] ramoops: " Nobuhiro Iwamatsu
@ 2016-09-08 21:53 ` Kees Cook
2016-10-05 4:13 ` 岩松信洋 / IWAMATSU，NOBUHIRO
0 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2016-09-08 21:53 UTC (permalink / raw)
To: Nobuhiro Iwamatsu
Cc: Anton Vorontsov, Colin Cross, Tony Luck, LKML, Hiraku Toyooka,
Mark Salyzyn, Seiji Aguchi
On Sun, Jul 24, 2016 at 8:56 PM, Nobuhiro Iwamatsu
<nobuhiro.iwamatsu.kw@hitachi.com> wrote:
> From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
>
> 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@hitachi.com>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
> Cc: Mark Salyzyn <salyzyn@android.com>
> Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
> ---
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH v2 4/5] ramoops: support multiple pmsg instances
2016-09-08 21:53 ` Kees Cook
@ 2016-10-05 4:13 ` 岩松信洋 / IWAMATSU，NOBUHIRO
0 siblings, 0 replies; 17+ messages in thread
From: 岩松信洋 / IWAMATSU，NOBUHIRO @ 2016-10-05 4:13 UTC (permalink / raw)
To: Kees Cook
Cc: Anton Vorontsov, Colin Cross, Tony Luck, LKML, Mark Salyzyn,
阿口誠司 / AGUCHI，SEIJI
Hi,
> -----Original Message-----
> From: keescook@google.com [mailto:keescook@google.com] 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@hitachi.com> wrote:
> > From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> >
> > 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@hitachi.com>
> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
> > Cc: Mark Salyzyn <salyzyn@android.com>
> > Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
> > ---
> > 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 5/5] selftests/pstore: add testcases for multiple pmsg instances
2016-07-25 3:56 [PATCH v2 0/5] pstore: ramoops: support multiple pmsg instances Nobuhiro Iwamatsu
` (3 preceding siblings ...)
2016-07-25 3:56 ` [PATCH v2 4/5] ramoops: " Nobuhiro Iwamatsu
@ 2016-07-25 3:56 ` Nobuhiro Iwamatsu
2016-09-08 21:55 ` Kees Cook
4 siblings, 1 reply; 17+ messages in thread
From: Nobuhiro Iwamatsu @ 2016-07-25 3:56 UTC (permalink / raw)
To: Anton Vorontsov, Colin Cross, Kees Cook, Tony Luck
Cc: linux-kernel, Hiraku Toyooka, Nobuhiro Iwamatsu, Mark Salyzyn,
Seiji Aguchi, Shuah Khan
From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
To test multiple pmsg, we should check that /dev/pmsg[N] (N > 0) is
available. After reboot, we should check that pmsg-[backend]-[N] which
keeps content is detected even if pmsg-[backend]-[N-M] (0 < M <= N)
doesn't exist due to lack of contents.
So this adds the following testcases.
- pstore_tests
- Write unique string to the last /dev/pmsgN
- pstore_post_reboot_tests
- Check the last pmsg area is detected
Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
Cc: Mark Salyzyn <salyzyn@android.com>
Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
Cc: Shuah Khan <shuahkh@osg.samsung.com>
---
tools/testing/selftests/pstore/common_tests | 21 +++++++++++++++--
.../selftests/pstore/pstore_post_reboot_tests | 27 ++++++++++++----------
tools/testing/selftests/pstore/pstore_tests | 16 ++++++++++---
3 files changed, 47 insertions(+), 17 deletions(-)
diff --git a/tools/testing/selftests/pstore/common_tests b/tools/testing/selftests/pstore/common_tests
index 3ea64d7..566a25d 100755
--- a/tools/testing/selftests/pstore/common_tests
+++ b/tools/testing/selftests/pstore/common_tests
@@ -27,9 +27,9 @@ show_result() { # result_value
}
check_files_exist() { # type of pstorefs file
- if [ -e ${1}-${backend}-0 ]; then
+ if [ -e ${1}0 ]; then
prlog "ok"
- for f in `ls ${1}-${backend}-*`; do
+ for f in `ls ${1}*`; do
prlog -e "\t${f}"
done
else
@@ -53,6 +53,23 @@ operate_files() { # tested value, files, operation
fi
}
+check_pmsg_content() { # pmsg filename
+ prev_uuid=`cat $TOP_DIR/prev_uuid`
+ if [ $? -eq 0 ]; then
+ nr_matched=`grep -c "$TEST_STRING_PATTERN" $1`
+ if [ "$nr_matched" = "1" ]; then
+ grep -q "$TEST_STRING_PATTERN"$prev_uuid $1
+ show_result $?
+ else
+ prlog "FAIL"
+ rc=1
+ fi
+ else
+ prlog "FAIL"
+ rc=1
+ fi
+}
+
# Parameters
TEST_STRING_PATTERN="Testing pstore: uuid="
UUID=`cat /proc/sys/kernel/random/uuid`
diff --git a/tools/testing/selftests/pstore/pstore_post_reboot_tests b/tools/testing/selftests/pstore/pstore_post_reboot_tests
index 6ccb154..272498f 100755
--- a/tools/testing/selftests/pstore/pstore_post_reboot_tests
+++ b/tools/testing/selftests/pstore/pstore_post_reboot_tests
@@ -35,13 +35,13 @@ fi
cd ${mount_point}
prlog -n "Checking dmesg files exist in pstore filesystem ... "
-check_files_exist dmesg
+check_files_exist dmesg-${backend}-
prlog -n "Checking console files exist in pstore filesystem ... "
-check_files_exist console
+check_files_exist console-${backend}-
prlog -n "Checking pmsg files exist in pstore filesystem ... "
-check_files_exist pmsg
+check_files_exist pmsg-${backend}-
prlog -n "Checking dmesg files contain oops end marker"
grep_end_trace() {
@@ -54,16 +54,19 @@ prlog -n "Checking console file contains oops end marker ... "
grep -q "\---\[ end trace" console-${backend}-0
show_result $?
-prlog -n "Checking pmsg file properly keeps the content written before crash ... "
-prev_uuid=`cat $TOP_DIR/prev_uuid`
-if [ $? -eq 0 ]; then
- nr_matched=`grep -c "$TEST_STRING_PATTERN" pmsg-${backend}-0`
- if [ $nr_matched -eq 1 ]; then
- grep -q "$TEST_STRING_PATTERN"$prev_uuid pmsg-${backend}-0
- show_result $?
+prlog -n "Checking pmsg-"${backend}"-0 properly keeps the content written before crash ... "
+check_pmsg_content pmsg-${backend}-0
+
+prlog -n "Checking the last pmsg area is detected "
+last_num_pmsg=`ls -v pmsg-* | tail -n1 | sed -e "s/^pmsg-${backend}-\([0-9]*\).*$/\1/"`
+last_num_devpmsg=`ls -v /dev/pmsg* | tail -n1 | sed -e "s/^\/dev\/pmsg\([0-9]*\).*$/\1/"`
+#checks the last number of pmsg-*-* and /dev/pmsg* correspond.
+if [ "$last_num_pmsg" -eq "$last_num_devpmsg" ]; then
+ if [ "$last_num_pmsg" = "0" ]; then
+ prlog "... No multiple pmsg-*-* exists. We skip this testcase."
else
- prlog "FAIL"
- rc=1
+ prlog -n "(pmsg-${backend}-${last_num_pmsg}) ... "
+ check_pmsg_content pmsg-${backend}-${last_num_pmsg}
fi
else
prlog "FAIL"
diff --git a/tools/testing/selftests/pstore/pstore_tests b/tools/testing/selftests/pstore/pstore_tests
index f25d2a3..5abf0e5 100755
--- a/tools/testing/selftests/pstore/pstore_tests
+++ b/tools/testing/selftests/pstore/pstore_tests
@@ -13,9 +13,8 @@ prlog -n "Checking pstore console is registered ... "
dmesg | grep -q "console \[pstore"
show_result $?
-prlog -n "Checking /dev/pmsg0 exists ... "
-test -e /dev/pmsg0
-show_result $?
+prlog -n "Checking /dev/pmsg files exist ... "
+check_files_exist /dev/pmsg
prlog -n "Writing unique string to /dev/pmsg0 ... "
if [ -e "/dev/pmsg0" ]; then
@@ -27,4 +26,15 @@ else
rc=1
fi
+last_devpmsg=`ls -v /dev/pmsg* | tail -n1`
+prlog -n "Writing unique string to the last /dev/pmsgN "
+if [ "$last_devpmsg" = "/dev/pmsg0" ]; then
+ prlog "... No multiple /dev/pmsg* exists. We skip this testcase."
+else
+ prlog -n "(${last_devpmsg}) ... "
+ echo "${TEST_STRING_PATTERN}""$UUID" > ${last_devpmsg}
+ show_result $?
+
+fi
+
exit $rc
--
2.8.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 5/5] selftests/pstore: add testcases for multiple pmsg instances
2016-07-25 3:56 ` [PATCH v2 5/5] selftests/pstore: add testcases for " Nobuhiro Iwamatsu
@ 2016-09-08 21:55 ` Kees Cook
2016-10-05 4:14 ` 岩松信洋 / IWAMATSU，NOBUHIRO
0 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2016-09-08 21:55 UTC (permalink / raw)
To: Nobuhiro Iwamatsu
Cc: Anton Vorontsov, Colin Cross, Tony Luck, LKML, Hiraku Toyooka,
Mark Salyzyn, Seiji Aguchi, Shuah Khan
On Sun, Jul 24, 2016 at 8:56 PM, Nobuhiro Iwamatsu
<nobuhiro.iwamatsu.kw@hitachi.com> wrote:
> From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
>
> To test multiple pmsg, we should check that /dev/pmsg[N] (N > 0) is
> available. After reboot, we should check that pmsg-[backend]-[N] which
> keeps content is detected even if pmsg-[backend]-[N-M] (0 < M <= N)
> doesn't exist due to lack of contents.
>
> So this adds the following testcases.
> - pstore_tests
> - Write unique string to the last /dev/pmsgN
> - pstore_post_reboot_tests
> - Check the last pmsg area is detected
>
> Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
> Cc: Mark Salyzyn <salyzyn@android.com>
> Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
> Cc: Shuah Khan <shuahkh@osg.samsung.com>
> ---
> tools/testing/selftests/pstore/common_tests | 21 +++++++++++++++--
> .../selftests/pstore/pstore_post_reboot_tests | 27 ++++++++++++----------
> tools/testing/selftests/pstore/pstore_tests | 16 ++++++++++---
> 3 files changed, 47 insertions(+), 17 deletions(-)
>
> diff --git a/tools/testing/selftests/pstore/common_tests b/tools/testing/selftests/pstore/common_tests
> index 3ea64d7..566a25d 100755
> --- a/tools/testing/selftests/pstore/common_tests
> +++ b/tools/testing/selftests/pstore/common_tests
> @@ -27,9 +27,9 @@ show_result() { # result_value
> }
>
> check_files_exist() { # type of pstorefs file
> - if [ -e ${1}-${backend}-0 ]; then
> + if [ -e ${1}0 ]; then
> prlog "ok"
> - for f in `ls ${1}-${backend}-*`; do
> + for f in `ls ${1}*`; do
> prlog -e "\t${f}"
> done
> else
> @@ -53,6 +53,23 @@ operate_files() { # tested value, files, operation
> fi
> }
>
> +check_pmsg_content() { # pmsg filename
> + prev_uuid=`cat $TOP_DIR/prev_uuid`
> + if [ $? -eq 0 ]; then
> + nr_matched=`grep -c "$TEST_STRING_PATTERN" $1`
> + if [ "$nr_matched" = "1" ]; then
> + grep -q "$TEST_STRING_PATTERN"$prev_uuid $1
> + show_result $?
> + else
> + prlog "FAIL"
> + rc=1
> + fi
> + else
> + prlog "FAIL"
> + rc=1
> + fi
> +}
> +
> # Parameters
> TEST_STRING_PATTERN="Testing pstore: uuid="
> UUID=`cat /proc/sys/kernel/random/uuid`
> diff --git a/tools/testing/selftests/pstore/pstore_post_reboot_tests b/tools/testing/selftests/pstore/pstore_post_reboot_tests
> index 6ccb154..272498f 100755
> --- a/tools/testing/selftests/pstore/pstore_post_reboot_tests
> +++ b/tools/testing/selftests/pstore/pstore_post_reboot_tests
> @@ -35,13 +35,13 @@ fi
> cd ${mount_point}
>
> prlog -n "Checking dmesg files exist in pstore filesystem ... "
> -check_files_exist dmesg
> +check_files_exist dmesg-${backend}-
>
> prlog -n "Checking console files exist in pstore filesystem ... "
> -check_files_exist console
> +check_files_exist console-${backend}-
>
> prlog -n "Checking pmsg files exist in pstore filesystem ... "
> -check_files_exist pmsg
> +check_files_exist pmsg-${backend}-
>
> prlog -n "Checking dmesg files contain oops end marker"
> grep_end_trace() {
> @@ -54,16 +54,19 @@ prlog -n "Checking console file contains oops end marker ... "
> grep -q "\---\[ end trace" console-${backend}-0
> show_result $?
>
> -prlog -n "Checking pmsg file properly keeps the content written before crash ... "
> -prev_uuid=`cat $TOP_DIR/prev_uuid`
> -if [ $? -eq 0 ]; then
> - nr_matched=`grep -c "$TEST_STRING_PATTERN" pmsg-${backend}-0`
> - if [ $nr_matched -eq 1 ]; then
> - grep -q "$TEST_STRING_PATTERN"$prev_uuid pmsg-${backend}-0
> - show_result $?
> +prlog -n "Checking pmsg-"${backend}"-0 properly keeps the content written before crash ... "
> +check_pmsg_content pmsg-${backend}-0
> +
> +prlog -n "Checking the last pmsg area is detected "
> +last_num_pmsg=`ls -v pmsg-* | tail -n1 | sed -e "s/^pmsg-${backend}-\([0-9]*\).*$/\1/"`
> +last_num_devpmsg=`ls -v /dev/pmsg* | tail -n1 | sed -e "s/^\/dev\/pmsg\([0-9]*\).*$/\1/"`
> +#checks the last number of pmsg-*-* and /dev/pmsg* correspond.
> +if [ "$last_num_pmsg" -eq "$last_num_devpmsg" ]; then
> + if [ "$last_num_pmsg" = "0" ]; then
> + prlog "... No multiple pmsg-*-* exists. We skip this testcase."
> else
> - prlog "FAIL"
> - rc=1
> + prlog -n "(pmsg-${backend}-${last_num_pmsg}) ... "
> + check_pmsg_content pmsg-${backend}-${last_num_pmsg}
> fi
> else
> prlog "FAIL"
> diff --git a/tools/testing/selftests/pstore/pstore_tests b/tools/testing/selftests/pstore/pstore_tests
> index f25d2a3..5abf0e5 100755
> --- a/tools/testing/selftests/pstore/pstore_tests
> +++ b/tools/testing/selftests/pstore/pstore_tests
> @@ -13,9 +13,8 @@ prlog -n "Checking pstore console is registered ... "
> dmesg | grep -q "console \[pstore"
> show_result $?
>
> -prlog -n "Checking /dev/pmsg0 exists ... "
> -test -e /dev/pmsg0
> -show_result $?
> +prlog -n "Checking /dev/pmsg files exist ... "
> +check_files_exist /dev/pmsg
>
> prlog -n "Writing unique string to /dev/pmsg0 ... "
> if [ -e "/dev/pmsg0" ]; then
> @@ -27,4 +26,15 @@ else
> rc=1
> fi
>
> +last_devpmsg=`ls -v /dev/pmsg* | tail -n1`
> +prlog -n "Writing unique string to the last /dev/pmsgN "
> +if [ "$last_devpmsg" = "/dev/pmsg0" ]; then
> + prlog "... No multiple /dev/pmsg* exists. We skip this testcase."
> +else
> + prlog -n "(${last_devpmsg}) ... "
> + echo "${TEST_STRING_PATTERN}""$UUID" > ${last_devpmsg}
> + show_result $?
> +
Blank line here can be dropped.
> +fi
> +
> exit $rc
> --
> 2.8.1
>
>
Otherwise, this looks good. Thanks!
-Kees
--
Kees Cook
Nexus Security
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH v2 5/5] selftests/pstore: add testcases for multiple pmsg instances
2016-09-08 21:55 ` Kees Cook
@ 2016-10-05 4:14 ` 岩松信洋 / IWAMATSU，NOBUHIRO
0 siblings, 0 replies; 17+ messages in thread
From: 岩松信洋 / IWAMATSU，NOBUHIRO @ 2016-10-05 4:14 UTC (permalink / raw)
To: Kees Cook
Cc: Anton Vorontsov, Colin Cross, Tony Luck, LKML, Mark Salyzyn,
阿口誠司 / AGUCHI，SEIJI,
Shuah Khan
Hi,
> -----Original Message-----
> From: keescook@google.com [mailto:keescook@google.com] On Behalf Of Kees
> Cook
> Sent: Friday, September 09, 2016 6:56 AM
> To: 岩松信洋 / IWAMATSU，NOBUHIRO
> Cc: Anton Vorontsov; Colin Cross; Tony Luck; LKML; Hiraku Toyooka; Mark
> Salyzyn; 阿口誠司 / AGUCHI，SEIJI; Shuah Khan
> Subject: Re: [PATCH v2 5/5] selftests/pstore: add testcases for multiple
> pmsg instances
>
> On Sun, Jul 24, 2016 at 8:56 PM, Nobuhiro Iwamatsu
> <nobuhiro.iwamatsu.kw@hitachi.com> wrote:
> > From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> >
> > To test multiple pmsg, we should check that /dev/pmsg[N] (N > 0) is
> > available. After reboot, we should check that pmsg-[backend]-[N] which
> > keeps content is detected even if pmsg-[backend]-[N-M] (0 < M <= N)
> > doesn't exist due to lack of contents.
> >
> > So this adds the following testcases.
> > - pstore_tests
> > - Write unique string to the last /dev/pmsgN
> > - pstore_post_reboot_tests
> > - Check the last pmsg area is detected
> >
> > Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com>
> > Cc: Mark Salyzyn <salyzyn@android.com>
> > Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com>
> > Cc: Shuah Khan <shuahkh@osg.samsung.com>
> > ---
> > tools/testing/selftests/pstore/common_tests | 21
> +++++++++++++++--
> > .../selftests/pstore/pstore_post_reboot_tests | 27
> ++++++++++++----------
> > tools/testing/selftests/pstore/pstore_tests | 16
> ++++++++++---
> > 3 files changed, 47 insertions(+), 17 deletions(-)
> >
> > diff --git a/tools/testing/selftests/pstore/common_tests
> > b/tools/testing/selftests/pstore/common_tests
> > index 3ea64d7..566a25d 100755
> > --- a/tools/testing/selftests/pstore/common_tests
> > +++ b/tools/testing/selftests/pstore/common_tests
<snip>
> > +last_devpmsg=`ls -v /dev/pmsg* | tail -n1` prlog -n "Writing unique
> > +string to the last /dev/pmsgN "
> > +if [ "$last_devpmsg" = "/dev/pmsg0" ]; then
> > + prlog "... No multiple /dev/pmsg* exists. We skip this testcase."
> > +else
> > + prlog -n "(${last_devpmsg}) ... "
> > + echo "${TEST_STRING_PATTERN}""$UUID" > ${last_devpmsg}
> > + show_result $?
> > +
>
> Blank line here can be dropped.
>
> > +fi
> > +
> > exit $rc
> > --
> > 2.8.1
> >
> >
>
> Otherwise, this looks good. Thanks!
>
OK, I will fix these.
And thanks again for review this patch series.
> -Kees
>
> --
> Kees Cook
> Nexus Security
Best regards,
Nobuhiro
^ permalink raw reply [flat|nested] 17+ messages in thread