[REPOST PATCH 1/2] pstore/ramoops: fixup driver removal

[REPOST PATCH 1/2] pstore/ramoops: fixup driver removal

[Sebastian Andrzej Siewior]

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>
Cc: Namhyung Kim <namhyung@kernel.org>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 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

[REPOST PATCH 2/2] pstore/core: drop cmpxchg based updates

[Sebastian Andrzej Siewior]

I have here a FPGA behind PCIe which exports SRAM which I use for
pstore. Now it seems that the FPGA no longer supports cmpxchg based
updates and writes back 0xff…ff and returns the same.  This leads to
crash during crash rendering pstore useless.
Since I doubt that there is much benefit from using cmpxchg() here, I am
dropping this atomic access and use the spinlock based version.

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>
Cc: Rabin Vincent <rabinv@axis.com>
Tested-by: Rabin Vincent <rabinv@axis.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
Also fixes this for ARMv7 as Rabin Vincent pointed out:
	https://lkml.kernel.org/g/CABXOdTfT7xMfiBvRuUS1hsVs=q5q2wY1x1Z8oCyyJNFckM0g0A@mail.gmail.com

 fs/pstore/ram_core.c | 43 ++-----------------------------------------
 1 file changed, 2 insertions(+), 41 deletions(-)

diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 76c3f80efdfa..4bae54bb61cd 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -47,39 +47,6 @@ static inline size_t buffer_start(struct persistent_ram_zone *prz)
 	return atomic_read(&prz->buffer->start);
 }
 
-/* increase and wrap the start pointer, returning the old value */
-static size_t buffer_start_add_atomic(struct persistent_ram_zone *prz, size_t a)
-{
-	int old;
-	int new;
-
-	do {
-		old = atomic_read(&prz->buffer->start);
-		new = old + a;
-		while (unlikely(new >= prz->buffer_size))
-			new -= prz->buffer_size;
-	} while (atomic_cmpxchg(&prz->buffer->start, old, new) != old);
-
-	return old;
-}
-
-/* increase the size counter until it hits the max size */
-static void buffer_size_add_atomic(struct persistent_ram_zone *prz, size_t a)
-{
-	size_t old;
-	size_t new;
-
-	if (atomic_read(&prz->buffer->size) == prz->buffer_size)
-		return;
-
-	do {
-		old = atomic_read(&prz->buffer->size);
-		new = old + a;
-		if (new > prz->buffer_size)
-			new = prz->buffer_size;
-	} while (atomic_cmpxchg(&prz->buffer->size, old, new) != old);
-}
-
 static DEFINE_RAW_SPINLOCK(buffer_lock);
 
 /* increase and wrap the start pointer, returning the old value */
@@ -124,9 +91,6 @@ static void buffer_size_add_locked(struct persistent_ram_zone *prz, size_t a)
 	raw_spin_unlock_irqrestore(&buffer_lock, flags);
 }
 
-static size_t (*buffer_start_add)(struct persistent_ram_zone *, size_t) = buffer_start_add_atomic;
-static void (*buffer_size_add)(struct persistent_ram_zone *, size_t) = buffer_size_add_atomic;
-
 static void notrace persistent_ram_encode_rs8(struct persistent_ram_zone *prz,
 	uint8_t *data, size_t len, uint8_t *ecc)
 {
@@ -338,9 +302,9 @@ int notrace persistent_ram_write(struct persistent_ram_zone *prz,
 		c = prz->buffer_size;
 	}
 
-	buffer_size_add(prz, c);
+	buffer_size_add_locked(prz, c);
 
-	start = buffer_start_add(prz, c);
+	start = buffer_start_add_locked(prz, c);
 
 	rem = prz->buffer_size - start;
 	if (unlikely(rem < c)) {
@@ -426,9 +390,6 @@ static void *persistent_ram_iomap(phys_addr_t start, size_t size,
 		return NULL;
 	}
 
-	buffer_start_add = buffer_start_add_locked;
-	buffer_size_add = buffer_size_add_locked;
-
 	if (memtype)
 		va = ioremap(start, size);
 	else
-- 
2.9.3

Re: [REPOST PATCH 1/2] pstore/ramoops: fixup driver removal

[Kees Cook]

On Thu, Sep 8, 2016 at 4:48 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>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Acked-by: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Thanks! I've applied this for -next.

-Kees

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



-- 
Kees Cook
Nexus Security

Re: [REPOST PATCH 2/2] pstore/core: drop cmpxchg based updates

[Kees Cook]

On Thu, Sep 8, 2016 at 4:48 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> I have here a FPGA behind PCIe which exports SRAM which I use for
> pstore. Now it seems that the FPGA no longer supports cmpxchg based
> updates and writes back 0xff…ff and returns the same.  This leads to
> crash during crash rendering pstore useless.
> Since I doubt that there is much benefit from using cmpxchg() here, I am
> dropping this atomic access and use the spinlock based version.
>
> 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>
> Cc: Rabin Vincent <rabinv@axis.com>
> Tested-by: Rabin Vincent <rabinv@axis.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> Also fixes this for ARMv7 as Rabin Vincent pointed out:
>         https://lkml.kernel.org/g/CABXOdTfT7xMfiBvRuUS1hsVs=q5q2wY1x1Z8oCyyJNFckM0g0A@mail.gmail.com

Thanks! I've applied this for -next.

-Kees

>
>  fs/pstore/ram_core.c | 43 ++-----------------------------------------
>  1 file changed, 2 insertions(+), 41 deletions(-)
>
> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
> index 76c3f80efdfa..4bae54bb61cd 100644
> --- a/fs/pstore/ram_core.c
> +++ b/fs/pstore/ram_core.c
> @@ -47,39 +47,6 @@ static inline size_t buffer_start(struct persistent_ram_zone *prz)
>         return atomic_read(&prz->buffer->start);
>  }
>
> -/* increase and wrap the start pointer, returning the old value */
> -static size_t buffer_start_add_atomic(struct persistent_ram_zone *prz, size_t a)
> -{
> -       int old;
> -       int new;
> -
> -       do {
> -               old = atomic_read(&prz->buffer->start);
> -               new = old + a;
> -               while (unlikely(new >= prz->buffer_size))
> -                       new -= prz->buffer_size;
> -       } while (atomic_cmpxchg(&prz->buffer->start, old, new) != old);
> -
> -       return old;
> -}
> -
> -/* increase the size counter until it hits the max size */
> -static void buffer_size_add_atomic(struct persistent_ram_zone *prz, size_t a)
> -{
> -       size_t old;
> -       size_t new;
> -
> -       if (atomic_read(&prz->buffer->size) == prz->buffer_size)
> -               return;
> -
> -       do {
> -               old = atomic_read(&prz->buffer->size);
> -               new = old + a;
> -               if (new > prz->buffer_size)
> -                       new = prz->buffer_size;
> -       } while (atomic_cmpxchg(&prz->buffer->size, old, new) != old);
> -}
> -
>  static DEFINE_RAW_SPINLOCK(buffer_lock);
>
>  /* increase and wrap the start pointer, returning the old value */
> @@ -124,9 +91,6 @@ static void buffer_size_add_locked(struct persistent_ram_zone *prz, size_t a)
>         raw_spin_unlock_irqrestore(&buffer_lock, flags);
>  }
>
> -static size_t (*buffer_start_add)(struct persistent_ram_zone *, size_t) = buffer_start_add_atomic;
> -static void (*buffer_size_add)(struct persistent_ram_zone *, size_t) = buffer_size_add_atomic;
> -
>  static void notrace persistent_ram_encode_rs8(struct persistent_ram_zone *prz,
>         uint8_t *data, size_t len, uint8_t *ecc)
>  {
> @@ -338,9 +302,9 @@ int notrace persistent_ram_write(struct persistent_ram_zone *prz,
>                 c = prz->buffer_size;
>         }
>
> -       buffer_size_add(prz, c);
> +       buffer_size_add_locked(prz, c);
>
> -       start = buffer_start_add(prz, c);
> +       start = buffer_start_add_locked(prz, c);
>
>         rem = prz->buffer_size - start;
>         if (unlikely(rem < c)) {
> @@ -426,9 +390,6 @@ static void *persistent_ram_iomap(phys_addr_t start, size_t size,
>                 return NULL;
>         }
>
> -       buffer_start_add = buffer_start_add_locked;
> -       buffer_size_add = buffer_size_add_locked;
> -
>         if (memtype)
>                 va = ioremap(start, size);
>         else
> --
> 2.9.3
>



-- 
Kees Cook
Nexus Security