* [PATCH] pstore/ramoops: fixup driver removal
@ 2016-08-19 16:52 Sebastian Andrzej Siewior
2016-08-31 3:27 ` Namhyung Kim
0 siblings, 1 reply; 2+ messages in thread
From: Sebastian Andrzej Siewior @ 2016-08-19 16:52 UTC (permalink / raw)
To: linux-kernel
Cc: Sebastian Andrzej Siewior, Anton Vorontsov, Colin Cross,
Kees Cook, Tony Luck
A basic rmmod ramoops segfaults. Let's see why.
Since commit 34f0ec82e0a9 ("pstore: Correct the max_dump_cnt clearing of
ramoops") sets ->max_dump_cnt to zero before looping over ->przs but we
didn't use it before that either.
And since commit ee1d267423a1 ("pstore: add pstore unregister") we free
that memory on rmmod.
But even then, we looped until a NULL pointer or ERR. I don't see where
it is ensured that the last member is NULL. Let's try this instead:
simply error recovery and free. Clean up in error case where ressouces
were allocated. And then, in the free path, rely on ->max_dump_cnt in
the free path.
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Tony Luck <tony.luck@intel.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
Not sure if I miss something obvious or not.
fs/pstore/ram.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 7a034d62cf8c..2340262a7e97 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -377,13 +377,14 @@ static void ramoops_free_przs(struct ramoops_context *cxt)
{
int i;
- cxt->max_dump_cnt = 0;
if (!cxt->przs)
return;
- for (i = 0; !IS_ERR_OR_NULL(cxt->przs[i]); i++)
+ for (i = 0; i < cxt->max_dump_cnt; i++)
persistent_ram_free(cxt->przs[i]);
+
kfree(cxt->przs);
+ cxt->max_dump_cnt = 0;
}
static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
@@ -408,7 +409,7 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
GFP_KERNEL);
if (!cxt->przs) {
dev_err(dev, "failed to initialize a prz array for dumps\n");
- goto fail_prz;
+ goto fail_mem;
}
for (i = 0; i < cxt->max_dump_cnt; i++) {
@@ -419,6 +420,11 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
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);
+
+ while (i > 0) {
+ i--;
+ persistent_ram_free(cxt->przs[i]);
+ }
goto fail_prz;
}
*paddr += cxt->record_size;
@@ -426,7 +432,9 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
return 0;
fail_prz:
- ramoops_free_przs(cxt);
+ kfree(cxt->przs);
+fail_mem:
+ cxt->max_dump_cnt = 0;
return err;
}
@@ -659,7 +667,6 @@ static int ramoops_remove(struct platform_device *pdev)
struct ramoops_context *cxt = &oops_cxt;
pstore_unregister(&cxt->pstore);
- cxt->max_dump_cnt = 0;
kfree(cxt->pstore.buf);
cxt->pstore.bufsize = 0;
--
2.9.3
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] pstore/ramoops: fixup driver removal
2016-08-19 16:52 [PATCH] pstore/ramoops: fixup driver removal Sebastian Andrzej Siewior
@ 2016-08-31 3:27 ` Namhyung Kim
0 siblings, 0 replies; 2+ messages in thread
From: Namhyung Kim @ 2016-08-31 3:27 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: LKML, Anton Vorontsov, Colin Cross, Kees Cook, Tony Luck
(Resending with all recipients, sorry)
On Sat, Aug 20, 2016 at 1:52 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> A basic rmmod ramoops segfaults. Let's see why.
>
> Since commit 34f0ec82e0a9 ("pstore: Correct the max_dump_cnt clearing of
> ramoops") sets ->max_dump_cnt to zero before looping over ->przs but we
> didn't use it before that either.
>
> And since commit ee1d267423a1 ("pstore: add pstore unregister") we free
> that memory on rmmod.
>
> But even then, we looped until a NULL pointer or ERR. I don't see where
> it is ensured that the last member is NULL. Let's try this instead:
> simply error recovery and free. Clean up in error case where ressouces
> were allocated. And then, in the free path, rely on ->max_dump_cnt in
> the free path.
>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Fwiw,
Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks,
Namhyung
> ---
>
> Not sure if I miss something obvious or not.
>
> fs/pstore/ram.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 7a034d62cf8c..2340262a7e97 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -377,13 +377,14 @@ static void ramoops_free_przs(struct ramoops_context *cxt)
> {
> int i;
>
> - cxt->max_dump_cnt = 0;
> if (!cxt->przs)
> return;
>
> - for (i = 0; !IS_ERR_OR_NULL(cxt->przs[i]); i++)
> + for (i = 0; i < cxt->max_dump_cnt; i++)
> persistent_ram_free(cxt->przs[i]);
> +
> kfree(cxt->przs);
> + cxt->max_dump_cnt = 0;
> }
>
> static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
> @@ -408,7 +409,7 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
> GFP_KERNEL);
> if (!cxt->przs) {
> dev_err(dev, "failed to initialize a prz array for dumps\n");
> - goto fail_prz;
> + goto fail_mem;
> }
>
> for (i = 0; i < cxt->max_dump_cnt; i++) {
> @@ -419,6 +420,11 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
> 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);
> +
> + while (i > 0) {
> + i--;
> + persistent_ram_free(cxt->przs[i]);
> + }
> goto fail_prz;
> }
> *paddr += cxt->record_size;
> @@ -426,7 +432,9 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
>
> return 0;
> fail_prz:
> - ramoops_free_przs(cxt);
> + kfree(cxt->przs);
> +fail_mem:
> + cxt->max_dump_cnt = 0;
> return err;
> }
>
> @@ -659,7 +667,6 @@ static int ramoops_remove(struct platform_device *pdev)
> struct ramoops_context *cxt = &oops_cxt;
>
> pstore_unregister(&cxt->pstore);
> - cxt->max_dump_cnt = 0;
>
> kfree(cxt->pstore.buf);
> cxt->pstore.bufsize = 0;
> --
> 2.9.3
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-08-31 3:27 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-19 16:52 [PATCH] pstore/ramoops: fixup driver removal Sebastian Andrzej Siewior
2016-08-31 3:27 ` Namhyung Kim
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.