[PATCH 1/3] pstore: add vmalloc error check

[PATCH 1/3] pstore: add vmalloc error check

[Geliang Tang]

If vmalloc is failed, return -ENOMEM.

Signed-off-by: Geliang Tang <geliangtang@163.com>
Acked-by: Kees Cook <keescook@chromium.org>
---
 Send it again.
---
 fs/pstore/pmsg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
index feb5dd2..5a2f05a 100644
--- a/fs/pstore/pmsg.c
+++ b/fs/pstore/pmsg.c
@@ -37,6 +37,8 @@ static ssize_t write_pmsg(struct file *file, const char __user *buf,
 	if (buffer_size > PMSG_MAX_BOUNCE_BUFFER_SIZE)
 		buffer_size = PMSG_MAX_BOUNCE_BUFFER_SIZE;
 	buffer = vmalloc(buffer_size);
+	if (!buffer)
+		return -ENOMEM;
 
 	mutex_lock(&pmsg_lock);
 	for (i = 0; i < count; ) {
-- 
2.5.0

[PATCH 2/3] pstore: add a helper function pstore_register_kmsg

[Geliang Tang]

Add a new wraper function pstore_register_kmsg to keep the
consistency with the other similar pstore_register_* functions.

Signed-off-by: Geliang Tang <geliangtang@163.com>
---
 Send it again.
---
 fs/pstore/platform.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 791743d..1b11249 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -353,6 +353,11 @@ static struct kmsg_dumper pstore_dumper = {
 	.dump = pstore_dump,
 };
 
+static void pstore_register_kmsg(void)
+{
+	kmsg_dump_register(&pstore_dumper);
+}
+
 #ifdef CONFIG_PSTORE_CONSOLE
 static void pstore_console_write(struct console *con, const char *s, unsigned c)
 {
@@ -442,7 +447,7 @@ int pstore_register(struct pstore_info *psi)
 	if (pstore_is_mounted())
 		pstore_get_records(0);
 
-	kmsg_dump_register(&pstore_dumper);
+	pstore_register_kmsg();
 
 	if ((psi->flags & PSTORE_FLAGS_FRAGILE) == 0) {
 		pstore_register_console();
-- 
2.5.0

[PATCH 3/3] pstore: add 'rmmod ramoops' support

[Geliang Tang]

ramoops doesn't support unregistering yet, it was marked as TODO.
This patch adds some code to fix it:
 1) Add functions to unregister kmsg/console/ftrace/pmsg.
 2) Add a function to free compression buffer.
 3) Remove try_module_get() to allow this module can be unloaded.
 4) Unmap the memory and free it.
 5) Set console flags, make sure it can be registered again after
be unregistered.

Signed-off-by: Geliang Tang <geliangtang@163.com>
---
 fs/pstore/ftrace.c     | 15 ++++++++++++++-
 fs/pstore/internal.h   |  4 ++++
 fs/pstore/platform.c   | 40 ++++++++++++++++++++++++++++++++--------
 fs/pstore/pmsg.c       |  7 +++++++
 fs/pstore/ram.c        | 19 ++++++++-----------
 include/linux/pstore.h |  5 +++++
 6 files changed, 70 insertions(+), 20 deletions(-)

diff --git a/fs/pstore/ftrace.c b/fs/pstore/ftrace.c
index 76a4eeb..554c1ce 100644
--- a/fs/pstore/ftrace.c
+++ b/fs/pstore/ftrace.c
@@ -104,9 +104,10 @@ static const struct file_operations pstore_knob_fops = {
 	.write	= pstore_ftrace_knob_write,
 };
 
+static struct dentry *dir;
+
 void pstore_register_ftrace(void)
 {
-	struct dentry *dir;
 	struct dentry *file;
 
 	if (!psinfo->write_buf)
@@ -129,3 +130,15 @@ void pstore_register_ftrace(void)
 err_file:
 	debugfs_remove(dir);
 }
+
+void pstore_unregister_ftrace(void)
+{
+	mutex_lock(&pstore_ftrace_lock);
+	if (pstore_ftrace_enabled) {
+		unregister_ftrace_function(&pstore_ftrace_ops);
+		pstore_ftrace_enabled = 0;
+	}
+	mutex_unlock(&pstore_ftrace_lock);
+
+	debugfs_remove_recursive(dir);
+}
diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
index c36ba2c..96253c4 100644
--- a/fs/pstore/internal.h
+++ b/fs/pstore/internal.h
@@ -41,14 +41,18 @@ pstore_ftrace_decode_cpu(struct pstore_ftrace_record *rec)
 
 #ifdef CONFIG_PSTORE_FTRACE
 extern void pstore_register_ftrace(void);
+extern void pstore_unregister_ftrace(void);
 #else
 static inline void pstore_register_ftrace(void) {}
+static inline void pstore_unregister_ftrace(void) {}
 #endif
 
 #ifdef CONFIG_PSTORE_PMSG
 extern void pstore_register_pmsg(void);
+extern void pstore_unregister_pmsg(void);
 #else
 static inline void pstore_register_pmsg(void) {}
+static inline void pstore_unregister_pmsg(void) {}
 #endif
 
 extern struct pstore_info *psinfo;
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 1b11249..c474dc2 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -237,6 +237,12 @@ static void allocate_buf_for_compression(void)
 
 }
 
+static void free_buf_for_compression(void)
+{
+	kfree(stream.workspace);
+	kfree(big_oops_buf);
+}
+
 /*
  * Called when compression fails, since the printk buffer
  * would be fetched for compression calling it again when
@@ -358,6 +364,11 @@ static void pstore_register_kmsg(void)
 	kmsg_dump_register(&pstore_dumper);
 }
 
+static void pstore_unregister_kmsg(void)
+{
+	kmsg_dump_unregister(&pstore_dumper);
+}
+
 #ifdef CONFIG_PSTORE_CONSOLE
 static void pstore_console_write(struct console *con, const char *s, unsigned c)
 {
@@ -387,16 +398,22 @@ static void pstore_console_write(struct console *con, const char *s, unsigned c)
 static struct console pstore_console = {
 	.name	= "pstore",
 	.write	= pstore_console_write,
-	.flags	= CON_PRINTBUFFER | CON_ENABLED | CON_ANYTIME,
 	.index	= -1,
 };
 
 static void pstore_register_console(void)
 {
+	pstore_console.flags = CON_PRINTBUFFER | CON_ENABLED | CON_ANYTIME;
 	register_console(&pstore_console);
 }
+
+static void pstore_unregister_console(void)
+{
+	unregister_console(&pstore_console);
+}
 #else
 static void pstore_register_console(void) {}
+static void pstore_unregister_console(void) {}
 #endif
 
 static int pstore_write_compat(enum pstore_type_id type,
@@ -420,8 +437,6 @@ static int pstore_write_compat(enum pstore_type_id type,
  */
 int pstore_register(struct pstore_info *psi)
 {
-	struct module *owner = psi->owner;
-
 	if (backend && strcmp(backend, psi->name))
 		return -EPERM;
 
@@ -437,11 +452,6 @@ int pstore_register(struct pstore_info *psi)
 	mutex_init(&psinfo->read_mutex);
 	spin_unlock(&pstore_lock);
 
-	if (owner && !try_module_get(owner)) {
-		psinfo = NULL;
-		return -EINVAL;
-	}
-
 	allocate_buf_for_compression();
 
 	if (pstore_is_mounted())
@@ -473,6 +483,20 @@ int pstore_register(struct pstore_info *psi)
 }
 EXPORT_SYMBOL_GPL(pstore_register);
 
+void pstore_unregister(void)
+{
+	pstore_unregister_pmsg();
+	pstore_unregister_ftrace();
+	pstore_unregister_console();
+	pstore_unregister_kmsg();
+
+	free_buf_for_compression();
+
+	psinfo = NULL;
+	backend = NULL;
+}
+EXPORT_SYMBOL_GPL(pstore_unregister);
+
 /*
  * Read all the records from the persistent store. Create
  * files in our filesystem.  Don't warn about -EEXIST errors
diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
index 5a2f05a..7de20cd 100644
--- a/fs/pstore/pmsg.c
+++ b/fs/pstore/pmsg.c
@@ -114,3 +114,10 @@ err_class:
 err:
 	return;
 }
+
+void pstore_unregister_pmsg(void)
+{
+	device_destroy(pmsg_class, MKDEV(pmsg_major, 0));
+	class_destroy(pmsg_class);
+	unregister_chrdev(pmsg_major, PMSG_NAME);
+}
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 6c26c4d..d938bc1 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -580,28 +580,25 @@ fail_out:
 
 static int __exit ramoops_remove(struct platform_device *pdev)
 {
-#if 0
-	/* TODO(kees): We cannot unload ramoops since pstore doesn't support
-	 * unregistering yet.
-	 */
 	struct ramoops_context *cxt = &oops_cxt;
 
-	iounmap(cxt->virt_addr);
-	release_mem_region(cxt->phys_addr, cxt->size);
-	cxt->max_dump_cnt = 0;
+	pstore_unregister();
 
-	/* TODO(kees): When pstore supports unregistering, call it here. */
+	cxt->max_dump_cnt = 0;
 	kfree(cxt->pstore.buf);
 	cxt->pstore.bufsize = 0;
 
+	persistent_ram_free(cxt->mprz);
+	persistent_ram_free(cxt->fprz);
+	persistent_ram_free(cxt->cprz);
+	ramoops_free_przs(cxt);
+
 	return 0;
-#endif
-	return -EBUSY;
 }
 
 static struct platform_driver ramoops_driver = {
 	.probe		= ramoops_probe,
-	.remove		= __exit_p(ramoops_remove),
+	.remove		= ramoops_remove,
 	.driver		= {
 		.name	= "ramoops",
 	},
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 8e7a25b..34e4a6d 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -77,6 +77,7 @@ struct pstore_info {
 
 #ifdef CONFIG_PSTORE
 extern int pstore_register(struct pstore_info *);
+extern void pstore_unregister(void);
 extern bool pstore_cannot_block_path(enum kmsg_dump_reason reason);
 #else
 static inline int
@@ -84,6 +85,10 @@ pstore_register(struct pstore_info *psi)
 {
 	return -ENODEV;
 }
+static inline void
+pstore_unregister(void)
+{
+}
 static inline bool
 pstore_cannot_block_path(enum kmsg_dump_reason reason)
 {
-- 
2.5.0

Re: [PATCH 3/3] pstore: add 'rmmod ramoops' support

[Kees Cook]

On Fri, Oct 16, 2015 at 8:25 AM, Geliang Tang <geliangtang@163.com> wrote:
> ramoops doesn't support unregistering yet, it was marked as TODO.
> This patch adds some code to fix it:
>  1) Add functions to unregister kmsg/console/ftrace/pmsg.
>  2) Add a function to free compression buffer.
>  3) Remove try_module_get() to allow this module can be unloaded.
>  4) Unmap the memory and free it.
>  5) Set console flags, make sure it can be registered again after
> be unregistered.
>
> Signed-off-by: Geliang Tang <geliangtang@163.com>
> ---
>  fs/pstore/ftrace.c     | 15 ++++++++++++++-
>  fs/pstore/internal.h   |  4 ++++
>  fs/pstore/platform.c   | 40 ++++++++++++++++++++++++++++++++--------
>  fs/pstore/pmsg.c       |  7 +++++++
>  fs/pstore/ram.c        | 19 ++++++++-----------
>  include/linux/pstore.h |  5 +++++
>  6 files changed, 70 insertions(+), 20 deletions(-)
>
> diff --git a/fs/pstore/ftrace.c b/fs/pstore/ftrace.c
> index 76a4eeb..554c1ce 100644
> --- a/fs/pstore/ftrace.c
> +++ b/fs/pstore/ftrace.c
> @@ -104,9 +104,10 @@ static const struct file_operations pstore_knob_fops = {
>         .write  = pstore_ftrace_knob_write,
>  };
>
> +static struct dentry *dir;

Since this is no longer a local, it should probably be renamed
something more descriptive.

> +
>  void pstore_register_ftrace(void)
>  {
> -       struct dentry *dir;
>         struct dentry *file;
>
>         if (!psinfo->write_buf)
> @@ -129,3 +130,15 @@ void pstore_register_ftrace(void)
>  err_file:
>         debugfs_remove(dir);
>  }
> +
> +void pstore_unregister_ftrace(void)
> +{
> +       mutex_lock(&pstore_ftrace_lock);
> +       if (pstore_ftrace_enabled) {
> +               unregister_ftrace_function(&pstore_ftrace_ops);
> +               pstore_ftrace_enabled = 0;
> +       }
> +       mutex_unlock(&pstore_ftrace_lock);
> +
> +       debugfs_remove_recursive(dir);
> +}
> diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
> index c36ba2c..96253c4 100644
> --- a/fs/pstore/internal.h
> +++ b/fs/pstore/internal.h
> @@ -41,14 +41,18 @@ pstore_ftrace_decode_cpu(struct pstore_ftrace_record *rec)
>
>  #ifdef CONFIG_PSTORE_FTRACE
>  extern void pstore_register_ftrace(void);
> +extern void pstore_unregister_ftrace(void);
>  #else
>  static inline void pstore_register_ftrace(void) {}
> +static inline void pstore_unregister_ftrace(void) {}
>  #endif
>
>  #ifdef CONFIG_PSTORE_PMSG
>  extern void pstore_register_pmsg(void);
> +extern void pstore_unregister_pmsg(void);
>  #else
>  static inline void pstore_register_pmsg(void) {}
> +static inline void pstore_unregister_pmsg(void) {}
>  #endif
>
>  extern struct pstore_info *psinfo;
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 1b11249..c474dc2 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -237,6 +237,12 @@ static void allocate_buf_for_compression(void)
>
>  }
>
> +static void free_buf_for_compression(void)
> +{
> +       kfree(stream.workspace);
> +       kfree(big_oops_buf);

I think both of these should be set to NULL after their kfrees.

> +}
> +
>  /*
>   * Called when compression fails, since the printk buffer
>   * would be fetched for compression calling it again when
> @@ -358,6 +364,11 @@ static void pstore_register_kmsg(void)
>         kmsg_dump_register(&pstore_dumper);
>  }
>
> +static void pstore_unregister_kmsg(void)
> +{
> +       kmsg_dump_unregister(&pstore_dumper);
> +}
> +
>  #ifdef CONFIG_PSTORE_CONSOLE
>  static void pstore_console_write(struct console *con, const char *s, unsigned c)
>  {
> @@ -387,16 +398,22 @@ static void pstore_console_write(struct console *con, const char *s, unsigned c)
>  static struct console pstore_console = {
>         .name   = "pstore",
>         .write  = pstore_console_write,
> -       .flags  = CON_PRINTBUFFER | CON_ENABLED | CON_ANYTIME,
>         .index  = -1,
>  };
>
>  static void pstore_register_console(void)
>  {
> +       pstore_console.flags = CON_PRINTBUFFER | CON_ENABLED | CON_ANYTIME;

Why do these flags need to move here?

>         register_console(&pstore_console);
>  }
> +
> +static void pstore_unregister_console(void)
> +{
> +       unregister_console(&pstore_console);
> +}
>  #else
>  static void pstore_register_console(void) {}
> +static void pstore_unregister_console(void) {}
>  #endif
>
>  static int pstore_write_compat(enum pstore_type_id type,
> @@ -420,8 +437,6 @@ static int pstore_write_compat(enum pstore_type_id type,
>   */
>  int pstore_register(struct pstore_info *psi)
>  {
> -       struct module *owner = psi->owner;
> -
>         if (backend && strcmp(backend, psi->name))
>                 return -EPERM;
>
> @@ -437,11 +452,6 @@ int pstore_register(struct pstore_info *psi)
>         mutex_init(&psinfo->read_mutex);
>         spin_unlock(&pstore_lock);
>
> -       if (owner && !try_module_get(owner)) {
> -               psinfo = NULL;
> -               return -EINVAL;
> -       }

Don't we still need to hold a module reference?

> -
>         allocate_buf_for_compression();
>
>         if (pstore_is_mounted())
> @@ -473,6 +483,20 @@ int pstore_register(struct pstore_info *psi)
>  }
>  EXPORT_SYMBOL_GPL(pstore_register);
>
> +void pstore_unregister(void)
> +{
> +       pstore_unregister_pmsg();
> +       pstore_unregister_ftrace();
> +       pstore_unregister_console();
> +       pstore_unregister_kmsg();
> +
> +       free_buf_for_compression();
> +
> +       psinfo = NULL;
> +       backend = NULL;
> +}
> +EXPORT_SYMBOL_GPL(pstore_unregister);
> +
>  /*
>   * Read all the records from the persistent store. Create
>   * files in our filesystem.  Don't warn about -EEXIST errors
> diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
> index 5a2f05a..7de20cd 100644
> --- a/fs/pstore/pmsg.c
> +++ b/fs/pstore/pmsg.c
> @@ -114,3 +114,10 @@ err_class:
>  err:
>         return;
>  }
> +
> +void pstore_unregister_pmsg(void)
> +{
> +       device_destroy(pmsg_class, MKDEV(pmsg_major, 0));
> +       class_destroy(pmsg_class);
> +       unregister_chrdev(pmsg_major, PMSG_NAME);
> +}
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 6c26c4d..d938bc1 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -580,28 +580,25 @@ fail_out:
>
>  static int __exit ramoops_remove(struct platform_device *pdev)
>  {
> -#if 0
> -       /* TODO(kees): We cannot unload ramoops since pstore doesn't support
> -        * unregistering yet.
> -        */
>         struct ramoops_context *cxt = &oops_cxt;
>
> -       iounmap(cxt->virt_addr);
> -       release_mem_region(cxt->phys_addr, cxt->size);
> -       cxt->max_dump_cnt = 0;
> +       pstore_unregister();
>
> -       /* TODO(kees): When pstore supports unregistering, call it here. */
> +       cxt->max_dump_cnt = 0;
>         kfree(cxt->pstore.buf);
>         cxt->pstore.bufsize = 0;
>
> +       persistent_ram_free(cxt->mprz);
> +       persistent_ram_free(cxt->fprz);
> +       persistent_ram_free(cxt->cprz);
> +       ramoops_free_przs(cxt);
> +
>         return 0;
> -#endif
> -       return -EBUSY;
>  }
>
>  static struct platform_driver ramoops_driver = {
>         .probe          = ramoops_probe,
> -       .remove         = __exit_p(ramoops_remove),
> +       .remove         = ramoops_remove,
>         .driver         = {
>                 .name   = "ramoops",
>         },
> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> index 8e7a25b..34e4a6d 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -77,6 +77,7 @@ struct pstore_info {
>
>  #ifdef CONFIG_PSTORE
>  extern int pstore_register(struct pstore_info *);
> +extern void pstore_unregister(void);
>  extern bool pstore_cannot_block_path(enum kmsg_dump_reason reason);
>  #else
>  static inline int
> @@ -84,6 +85,10 @@ pstore_register(struct pstore_info *psi)
>  {
>         return -ENODEV;
>  }
> +static inline void
> +pstore_unregister(void)
> +{
> +}
>  static inline bool
>  pstore_cannot_block_path(enum kmsg_dump_reason reason)
>  {
> --
> 2.5.0
>
>

Cool! Thanks for working on the unregister logic!

-Kees

-- 
Kees Cook
Chrome OS Security

[PATCH v2 0/3] pstore: add pstore unregister

[Geliang Tang]

On Fri, Oct 16, 2015 at 08:50:42AM -0700, Kees Cook wrote:
> On Fri, Oct 16, 2015 at 8:25 AM, Geliang Tang <geliangtang@163.com> wrote:
> > +static struct dentry *dir;
> 
> Since this is no longer a local, it should probably be renamed
> something more descriptive.
> 

I renamed it pstore_ftrace_dir.

> > +static void free_buf_for_compression(void)
> > +{
> > +       kfree(stream.workspace);
> > +       kfree(big_oops_buf);
> 
> I think both of these should be set to NULL after their kfrees.
> 

I did it as you suggested.

> >  static void pstore_register_console(void)
> >  {
> > +       pstore_console.flags = CON_PRINTBUFFER | CON_ENABLED | CON_ANYTIME;
> 
> Why do these flags need to move here?
> 

No need to deal these console flags here specifically. I drop it.

> > -       if (owner && !try_module_get(owner)) {
> > -               psinfo = NULL;
> > -               return -EINVAL;
> > -       }
> 
> Don't we still need to hold a module reference?
> 

Yes. I simply add module_put in pstore_register() to deal with it. 
Maybe it's not good enough. Please give me some advices.

Thanks.

Geliang Tang (3):
  pstore: add vmalloc error check
  pstore: add a helper function pstore_register_kmsg
  pstore: add pstore unregister

 fs/pstore/Kconfig      |  2 +-
 fs/pstore/Makefile     |  6 +++---
 fs/pstore/ftrace.c     | 23 ++++++++++++++++++-----
 fs/pstore/inode.c      |  7 +++++++
 fs/pstore/internal.h   |  4 ++++
 fs/pstore/platform.c   | 42 +++++++++++++++++++++++++++++++++++++++++-
 fs/pstore/pmsg.c       |  9 +++++++++
 fs/pstore/ram.c        | 17 +++++++----------
 include/linux/pstore.h | 14 +-------------
 kernel/printk/printk.c |  1 +
 10 files changed, 92 insertions(+), 33 deletions(-)

-- 
2.5.0

[PATCH v2 1/3] pstore: add vmalloc error check

[Geliang Tang]

When vmalloc is failed, let write_pmsg return -ENOMEM.

Signed-off-by: Geliang Tang <geliangtang@163.com>
Acked-by: Kees Cook <keescook@chromium.org>
---
Changes in v2:
 - update commit log.
---
 fs/pstore/pmsg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
index feb5dd2..5a2f05a 100644
--- a/fs/pstore/pmsg.c
+++ b/fs/pstore/pmsg.c
@@ -37,6 +37,8 @@ static ssize_t write_pmsg(struct file *file, const char __user *buf,
 	if (buffer_size > PMSG_MAX_BOUNCE_BUFFER_SIZE)
 		buffer_size = PMSG_MAX_BOUNCE_BUFFER_SIZE;
 	buffer = vmalloc(buffer_size);
+	if (!buffer)
+		return -ENOMEM;
 
 	mutex_lock(&pmsg_lock);
 	for (i = 0; i < count; ) {
-- 
2.5.0

[PATCH v2 2/3] pstore: add a helper function pstore_register_kmsg

[Geliang Tang]

Add a new wraper function pstore_register_kmsg to keep the
consistency with other similar pstore_register_* functions.

Signed-off-by: Geliang Tang <geliangtang@163.com>
---
Changes in v2:
 - update commit log.
---
 fs/pstore/platform.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 791743d..1b11249 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -353,6 +353,11 @@ static struct kmsg_dumper pstore_dumper = {
 	.dump = pstore_dump,
 };
 
+static void pstore_register_kmsg(void)
+{
+	kmsg_dump_register(&pstore_dumper);
+}
+
 #ifdef CONFIG_PSTORE_CONSOLE
 static void pstore_console_write(struct console *con, const char *s, unsigned c)
 {
@@ -442,7 +447,7 @@ int pstore_register(struct pstore_info *psi)
 	if (pstore_is_mounted())
 		pstore_get_records(0);
 
-	kmsg_dump_register(&pstore_dumper);
+	pstore_register_kmsg();
 
 	if ((psi->flags & PSTORE_FLAGS_FRAGILE) == 0) {
 		pstore_register_console();
-- 
2.5.0

[PATCH v2 3/3] pstore: add pstore unregister

[Geliang Tang]

pstore doesn't support unregistering yet. It was marked as TODO.
This patch adds some code to fix it:
 1) Add functions to unregister kmsg/console/ftrace/pmsg.
 2) Add a function to free compression buffer.
 3) Unmap the memory and free it.
 4) Add a function to unregister pstore filesystem.

Signed-off-by: Geliang Tang <geliangtang@163.com>
---
Changes in v2:
 - Add pstore filesystem unregister.
 - update commit log.
---
 fs/pstore/Kconfig      |  2 +-
 fs/pstore/Makefile     |  6 +++---
 fs/pstore/ftrace.c     | 23 ++++++++++++++++++-----
 fs/pstore/inode.c      |  7 +++++++
 fs/pstore/internal.h   |  4 ++++
 fs/pstore/platform.c   | 35 +++++++++++++++++++++++++++++++++++
 fs/pstore/pmsg.c       |  7 +++++++
 fs/pstore/ram.c        | 17 +++++++----------
 include/linux/pstore.h | 14 +-------------
 kernel/printk/printk.c |  1 +
 10 files changed, 84 insertions(+), 32 deletions(-)

diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
index 916b8e2..360ae43 100644
--- a/fs/pstore/Kconfig
+++ b/fs/pstore/Kconfig
@@ -1,5 +1,5 @@
 config PSTORE
-	bool "Persistent store support"
+	tristate "Persistent store support"
 	default n
 	select ZLIB_DEFLATE
 	select ZLIB_INFLATE
diff --git a/fs/pstore/Makefile b/fs/pstore/Makefile
index e647d8e..b8803cc 100644
--- a/fs/pstore/Makefile
+++ b/fs/pstore/Makefile
@@ -2,12 +2,12 @@
 # Makefile for the linux pstorefs routines.
 #
 
-obj-y += pstore.o
+obj-$(CONFIG_PSTORE) += pstore.o
 
 pstore-objs += inode.o platform.o
-obj-$(CONFIG_PSTORE_FTRACE)	+= ftrace.o
+pstore-$(CONFIG_PSTORE_FTRACE)	+= ftrace.o
 
-obj-$(CONFIG_PSTORE_PMSG)	+= pmsg.o
+pstore-$(CONFIG_PSTORE_PMSG)	+= pmsg.o
 
 ramoops-objs += ram.o ram_core.o
 obj-$(CONFIG_PSTORE_RAM)	+= ramoops.o
diff --git a/fs/pstore/ftrace.c b/fs/pstore/ftrace.c
index 76a4eeb..788600f 100644
--- a/fs/pstore/ftrace.c
+++ b/fs/pstore/ftrace.c
@@ -104,21 +104,22 @@ static const struct file_operations pstore_knob_fops = {
 	.write	= pstore_ftrace_knob_write,
 };
 
+static struct dentry *pstore_ftrace_dir;
+
 void pstore_register_ftrace(void)
 {
-	struct dentry *dir;
 	struct dentry *file;
 
 	if (!psinfo->write_buf)
 		return;
 
-	dir = debugfs_create_dir("pstore", NULL);
-	if (!dir) {
+	pstore_ftrace_dir = debugfs_create_dir("pstore", NULL);
+	if (!pstore_ftrace_dir) {
 		pr_err("%s: unable to create pstore directory\n", __func__);
 		return;
 	}
 
-	file = debugfs_create_file("record_ftrace", 0600, dir, NULL,
+	file = debugfs_create_file("record_ftrace", 0600, pstore_ftrace_dir, NULL,
 				   &pstore_knob_fops);
 	if (!file) {
 		pr_err("%s: unable to create record_ftrace file\n", __func__);
@@ -127,5 +128,17 @@ void pstore_register_ftrace(void)
 
 	return;
 err_file:
-	debugfs_remove(dir);
+	debugfs_remove(pstore_ftrace_dir);
+}
+
+void pstore_unregister_ftrace(void)
+{
+	mutex_lock(&pstore_ftrace_lock);
+	if (pstore_ftrace_enabled) {
+		unregister_ftrace_function(&pstore_ftrace_ops);
+		pstore_ftrace_enabled = 0;
+	}
+	mutex_unlock(&pstore_ftrace_lock);
+
+	debugfs_remove_recursive(pstore_ftrace_dir);
 }
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 3adcc46..598b52a 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -479,5 +479,12 @@ out:
 }
 module_init(init_pstore_fs)
 
+static void __exit exit_pstore_fs(void)
+{
+	unregister_filesystem(&pstore_fs_type);
+	sysfs_remove_mount_point(fs_kobj, "pstore");
+}
+module_exit(exit_pstore_fs)
+
 MODULE_AUTHOR("Tony Luck <tony.luck@intel.com>");
 MODULE_LICENSE("GPL");
diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
index c36ba2c..96253c4 100644
--- a/fs/pstore/internal.h
+++ b/fs/pstore/internal.h
@@ -41,14 +41,18 @@ pstore_ftrace_decode_cpu(struct pstore_ftrace_record *rec)
 
 #ifdef CONFIG_PSTORE_FTRACE
 extern void pstore_register_ftrace(void);
+extern void pstore_unregister_ftrace(void);
 #else
 static inline void pstore_register_ftrace(void) {}
+static inline void pstore_unregister_ftrace(void) {}
 #endif
 
 #ifdef CONFIG_PSTORE_PMSG
 extern void pstore_register_pmsg(void);
+extern void pstore_unregister_pmsg(void);
 #else
 static inline void pstore_register_pmsg(void) {}
+static inline void pstore_unregister_pmsg(void) {}
 #endif
 
 extern struct pstore_info *psinfo;
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 1b11249..0aab920 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -237,6 +237,14 @@ static void allocate_buf_for_compression(void)
 
 }
 
+static void free_buf_for_compression(void)
+{
+	kfree(stream.workspace);
+	stream.workspace = NULL;
+	kfree(big_oops_buf);
+	big_oops_buf = NULL;
+}
+
 /*
  * Called when compression fails, since the printk buffer
  * would be fetched for compression calling it again when
@@ -358,6 +366,11 @@ static void pstore_register_kmsg(void)
 	kmsg_dump_register(&pstore_dumper);
 }
 
+static void pstore_unregister_kmsg(void)
+{
+	kmsg_dump_unregister(&pstore_dumper);
+}
+
 #ifdef CONFIG_PSTORE_CONSOLE
 static void pstore_console_write(struct console *con, const char *s, unsigned c)
 {
@@ -395,8 +408,14 @@ static void pstore_register_console(void)
 {
 	register_console(&pstore_console);
 }
+
+static void pstore_unregister_console(void)
+{
+	unregister_console(&pstore_console);
+}
 #else
 static void pstore_register_console(void) {}
+static void pstore_unregister_console(void) {}
 #endif
 
 static int pstore_write_compat(enum pstore_type_id type,
@@ -467,12 +486,28 @@ int pstore_register(struct pstore_info *psi)
 	 */
 	backend = psi->name;
 
+	module_put(owner);
+
 	pr_info("Registered %s as persistent store backend\n", psi->name);
 
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pstore_register);
 
+void pstore_unregister(struct pstore_info *psi)
+{
+	pstore_unregister_pmsg();
+	pstore_unregister_ftrace();
+	pstore_unregister_console();
+	pstore_unregister_kmsg();
+
+	free_buf_for_compression();
+
+	psinfo = NULL;
+	backend = NULL;
+}
+EXPORT_SYMBOL_GPL(pstore_unregister);
+
 /*
  * Read all the records from the persistent store. Create
  * files in our filesystem.  Don't warn about -EEXIST errors
diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
index 5a2f05a..7de20cd 100644
--- a/fs/pstore/pmsg.c
+++ b/fs/pstore/pmsg.c
@@ -114,3 +114,10 @@ err_class:
 err:
 	return;
 }
+
+void pstore_unregister_pmsg(void)
+{
+	device_destroy(pmsg_class, MKDEV(pmsg_major, 0));
+	class_destroy(pmsg_class);
+	unregister_chrdev(pmsg_major, PMSG_NAME);
+}
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 6c26c4d..68889a7 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -580,28 +580,25 @@ fail_out:
 
 static int __exit ramoops_remove(struct platform_device *pdev)
 {
-#if 0
-	/* TODO(kees): We cannot unload ramoops since pstore doesn't support
-	 * unregistering yet.
-	 */
 	struct ramoops_context *cxt = &oops_cxt;
 
-	iounmap(cxt->virt_addr);
-	release_mem_region(cxt->phys_addr, cxt->size);
+	pstore_unregister(&cxt->pstore);
 	cxt->max_dump_cnt = 0;
 
-	/* TODO(kees): When pstore supports unregistering, call it here. */
 	kfree(cxt->pstore.buf);
 	cxt->pstore.bufsize = 0;
 
+	persistent_ram_free(cxt->mprz);
+	persistent_ram_free(cxt->fprz);
+	persistent_ram_free(cxt->cprz);
+	ramoops_free_przs(cxt);
+
 	return 0;
-#endif
-	return -EBUSY;
 }
 
 static struct platform_driver ramoops_driver = {
 	.probe		= ramoops_probe,
-	.remove		= __exit_p(ramoops_remove),
+	.remove		= ramoops_remove,
 	.driver		= {
 		.name	= "ramoops",
 	},
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 8e7a25b..831479f 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -75,20 +75,8 @@ struct pstore_info {
 
 #define	PSTORE_FLAGS_FRAGILE	1
 
-#ifdef CONFIG_PSTORE
 extern int pstore_register(struct pstore_info *);
+extern void pstore_unregister(struct pstore_info *);
 extern bool pstore_cannot_block_path(enum kmsg_dump_reason reason);
-#else
-static inline int
-pstore_register(struct pstore_info *psi)
-{
-	return -ENODEV;
-}
-static inline bool
-pstore_cannot_block_path(enum kmsg_dump_reason reason)
-{
-	return false;
-}
-#endif
 
 #endif /*_LINUX_PSTORE_H*/
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 8f0324e..b16f354 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -517,6 +517,7 @@ int check_syslog_permissions(int type, int source)
 ok:
 	return security_syslog(type);
 }
+EXPORT_SYMBOL_GPL(check_syslog_permissions);
 
 static void append_char(char **pp, char *e, char c)
 {
-- 
2.5.0

RE: [PATCH v2 3/3] pstore: add pstore unregister

[Luck, Tony]

> pstore doesn't support unregistering yet. It was marked as TODO.

Thanks for looking to close out this TODO item.

The thing that scared me about unloading pstore was what happens to
a process that is in the middle of reading some /sys/fs/pstore/file-name-here

Do we have all the right reference counts to make sure that process doesn't do
weird things if you rmmod pstore in the middle of a read? Or for a subsequent
read from the still-open file descriptor?

-Tony

[PATCH v3 0/3] pstore: add pstore unregister

[Geliang Tang]

On Mon, Oct 19, 2015 at 10:56:54PM +0000, Luck, Tony wrote:
> Thanks for looking to close out this TODO item.
> 
> The thing that scared me about unloading pstore was what happens to
> a process that is in the middle of reading some /sys/fs/pstore/file-name-here
> 
> Do we have all the right reference counts to make sure that process doesn't do
> weird things if you rmmod pstore in the middle of a read? Or for a subsequent
> read from the still-open file descriptor?
> 
> -Tony

Thanks for your review. I updated the patches as you suggested.

// Increase a reference count when pstore file is read.
 static const struct file_operations pstore_file_operations = {
+       .owner          = THIS_MODULE,
        .open           = pstore_file_open,
        .read           = pstore_file_read,
        .llseek         = pstore_file_llseek,

// Increase a reference count when pstore is mounted.
 static struct file_system_type pstore_fs_type = {
+       .owner          = THIS_MODULE,
        .name           = "pstore",
        .mount          = pstore_mount,
        .kill_sb        = pstore_kill_sb,

---
Changes in v3:
 - Increase a reference count when pstore is used.
Changes in v2:
 - Add pstore filesystem unregister.
 - update commit log.
---

Geliang Tang (3):
  pstore: add vmalloc error check
  pstore: add a helper function pstore_register_kmsg
  pstore: add pstore unregister

 fs/pstore/Kconfig      |  2 +-
 fs/pstore/Makefile     |  6 +++---
 fs/pstore/ftrace.c     | 23 ++++++++++++++++++-----
 fs/pstore/inode.c      |  9 +++++++++
 fs/pstore/internal.h   |  4 ++++
 fs/pstore/platform.c   | 42 +++++++++++++++++++++++++++++++++++++++++-
 fs/pstore/pmsg.c       |  9 +++++++++
 fs/pstore/ram.c        | 17 +++++++----------
 include/linux/pstore.h | 14 +-------------
 kernel/printk/printk.c |  1 +
 10 files changed, 94 insertions(+), 33 deletions(-)

-- 
1.9.1

[PATCH v3 1/3] pstore: add vmalloc error check

[Geliang Tang]

When vmalloc is failed, let write_pmsg return -ENOMEM.

Signed-off-by: Geliang Tang <geliangtang@163.com>
Acked-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/pmsg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
index feb5dd2..5a2f05a 100644
--- a/fs/pstore/pmsg.c
+++ b/fs/pstore/pmsg.c
@@ -37,6 +37,8 @@ static ssize_t write_pmsg(struct file *file, const char __user *buf,
 	if (buffer_size > PMSG_MAX_BOUNCE_BUFFER_SIZE)
 		buffer_size = PMSG_MAX_BOUNCE_BUFFER_SIZE;
 	buffer = vmalloc(buffer_size);
+	if (!buffer)
+		return -ENOMEM;
 
 	mutex_lock(&pmsg_lock);
 	for (i = 0; i < count; ) {
-- 
1.9.1

[PATCH v3 2/3] pstore: add a helper function pstore_register_kmsg

[Geliang Tang]

Add a new wraper function pstore_register_kmsg to keep the
consistency with other similar pstore_register_* functions.

Signed-off-by: Geliang Tang <geliangtang@163.com>
---
 fs/pstore/platform.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 791743d..1b11249 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -353,6 +353,11 @@ static struct kmsg_dumper pstore_dumper = {
 	.dump = pstore_dump,
 };
 
+static void pstore_register_kmsg(void)
+{
+	kmsg_dump_register(&pstore_dumper);
+}
+
 #ifdef CONFIG_PSTORE_CONSOLE
 static void pstore_console_write(struct console *con, const char *s, unsigned c)
 {
@@ -442,7 +447,7 @@ int pstore_register(struct pstore_info *psi)
 	if (pstore_is_mounted())
 		pstore_get_records(0);
 
-	kmsg_dump_register(&pstore_dumper);
+	pstore_register_kmsg();
 
 	if ((psi->flags & PSTORE_FLAGS_FRAGILE) == 0) {
 		pstore_register_console();
-- 
1.9.1

[PATCH v3 3/3] pstore: add pstore unregister

[Geliang Tang]

pstore doesn't support unregistering yet. It was marked as TODO.
This patch adds some code to fix it:
 1) Add functions to unregister kmsg/console/ftrace/pmsg.
 2) Add a function to free compression buffer.
 3) Unmap the memory and free it.
 4) Add a function to unregister pstore filesystem.

Signed-off-by: Geliang Tang <geliangtang@163.com>
---
 fs/pstore/Kconfig      |  2 +-
 fs/pstore/Makefile     |  6 +++---
 fs/pstore/ftrace.c     | 23 ++++++++++++++++++-----
 fs/pstore/inode.c      |  9 +++++++++
 fs/pstore/internal.h   |  4 ++++
 fs/pstore/platform.c   | 35 +++++++++++++++++++++++++++++++++++
 fs/pstore/pmsg.c       |  7 +++++++
 fs/pstore/ram.c        | 17 +++++++----------
 include/linux/pstore.h | 14 +-------------
 kernel/printk/printk.c |  1 +
 10 files changed, 86 insertions(+), 32 deletions(-)

diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
index 916b8e2..360ae43 100644
--- a/fs/pstore/Kconfig
+++ b/fs/pstore/Kconfig
@@ -1,5 +1,5 @@
 config PSTORE
-	bool "Persistent store support"
+	tristate "Persistent store support"
 	default n
 	select ZLIB_DEFLATE
 	select ZLIB_INFLATE
diff --git a/fs/pstore/Makefile b/fs/pstore/Makefile
index e647d8e..b8803cc 100644
--- a/fs/pstore/Makefile
+++ b/fs/pstore/Makefile
@@ -2,12 +2,12 @@
 # Makefile for the linux pstorefs routines.
 #
 
-obj-y += pstore.o
+obj-$(CONFIG_PSTORE) += pstore.o
 
 pstore-objs += inode.o platform.o
-obj-$(CONFIG_PSTORE_FTRACE)	+= ftrace.o
+pstore-$(CONFIG_PSTORE_FTRACE)	+= ftrace.o
 
-obj-$(CONFIG_PSTORE_PMSG)	+= pmsg.o
+pstore-$(CONFIG_PSTORE_PMSG)	+= pmsg.o
 
 ramoops-objs += ram.o ram_core.o
 obj-$(CONFIG_PSTORE_RAM)	+= ramoops.o
diff --git a/fs/pstore/ftrace.c b/fs/pstore/ftrace.c
index 76a4eeb..788600f 100644
--- a/fs/pstore/ftrace.c
+++ b/fs/pstore/ftrace.c
@@ -104,21 +104,22 @@ static const struct file_operations pstore_knob_fops = {
 	.write	= pstore_ftrace_knob_write,
 };
 
+static struct dentry *pstore_ftrace_dir;
+
 void pstore_register_ftrace(void)
 {
-	struct dentry *dir;
 	struct dentry *file;
 
 	if (!psinfo->write_buf)
 		return;
 
-	dir = debugfs_create_dir("pstore", NULL);
-	if (!dir) {
+	pstore_ftrace_dir = debugfs_create_dir("pstore", NULL);
+	if (!pstore_ftrace_dir) {
 		pr_err("%s: unable to create pstore directory\n", __func__);
 		return;
 	}
 
-	file = debugfs_create_file("record_ftrace", 0600, dir, NULL,
+	file = debugfs_create_file("record_ftrace", 0600, pstore_ftrace_dir, NULL,
 				   &pstore_knob_fops);
 	if (!file) {
 		pr_err("%s: unable to create record_ftrace file\n", __func__);
@@ -127,5 +128,17 @@ void pstore_register_ftrace(void)
 
 	return;
 err_file:
-	debugfs_remove(dir);
+	debugfs_remove(pstore_ftrace_dir);
+}
+
+void pstore_unregister_ftrace(void)
+{
+	mutex_lock(&pstore_ftrace_lock);
+	if (pstore_ftrace_enabled) {
+		unregister_ftrace_function(&pstore_ftrace_ops);
+		pstore_ftrace_enabled = 0;
+	}
+	mutex_unlock(&pstore_ftrace_lock);
+
+	debugfs_remove_recursive(pstore_ftrace_dir);
 }
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 3adcc46..3586491 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -178,6 +178,7 @@ static loff_t pstore_file_llseek(struct file *file, loff_t off, int whence)
 }
 
 static const struct file_operations pstore_file_operations = {
+	.owner		= THIS_MODULE,
 	.open		= pstore_file_open,
 	.read		= pstore_file_read,
 	.llseek		= pstore_file_llseek,
@@ -456,6 +457,7 @@ static void pstore_kill_sb(struct super_block *sb)
 }
 
 static struct file_system_type pstore_fs_type = {
+	.owner          = THIS_MODULE,
 	.name		= "pstore",
 	.mount		= pstore_mount,
 	.kill_sb	= pstore_kill_sb,
@@ -479,5 +481,12 @@ out:
 }
 module_init(init_pstore_fs)
 
+static void __exit exit_pstore_fs(void)
+{
+	unregister_filesystem(&pstore_fs_type);
+	sysfs_remove_mount_point(fs_kobj, "pstore");
+}
+module_exit(exit_pstore_fs)
+
 MODULE_AUTHOR("Tony Luck <tony.luck@intel.com>");
 MODULE_LICENSE("GPL");
diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
index c36ba2c..96253c4 100644
--- a/fs/pstore/internal.h
+++ b/fs/pstore/internal.h
@@ -41,14 +41,18 @@ pstore_ftrace_decode_cpu(struct pstore_ftrace_record *rec)
 
 #ifdef CONFIG_PSTORE_FTRACE
 extern void pstore_register_ftrace(void);
+extern void pstore_unregister_ftrace(void);
 #else
 static inline void pstore_register_ftrace(void) {}
+static inline void pstore_unregister_ftrace(void) {}
 #endif
 
 #ifdef CONFIG_PSTORE_PMSG
 extern void pstore_register_pmsg(void);
+extern void pstore_unregister_pmsg(void);
 #else
 static inline void pstore_register_pmsg(void) {}
+static inline void pstore_unregister_pmsg(void) {}
 #endif
 
 extern struct pstore_info *psinfo;
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 1b11249..0aab920 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -237,6 +237,14 @@ static void allocate_buf_for_compression(void)
 
 }
 
+static void free_buf_for_compression(void)
+{
+	kfree(stream.workspace);
+	stream.workspace = NULL;
+	kfree(big_oops_buf);
+	big_oops_buf = NULL;
+}
+
 /*
  * Called when compression fails, since the printk buffer
  * would be fetched for compression calling it again when
@@ -358,6 +366,11 @@ static void pstore_register_kmsg(void)
 	kmsg_dump_register(&pstore_dumper);
 }
 
+static void pstore_unregister_kmsg(void)
+{
+	kmsg_dump_unregister(&pstore_dumper);
+}
+
 #ifdef CONFIG_PSTORE_CONSOLE
 static void pstore_console_write(struct console *con, const char *s, unsigned c)
 {
@@ -395,8 +408,14 @@ static void pstore_register_console(void)
 {
 	register_console(&pstore_console);
 }
+
+static void pstore_unregister_console(void)
+{
+	unregister_console(&pstore_console);
+}
 #else
 static void pstore_register_console(void) {}
+static void pstore_unregister_console(void) {}
 #endif
 
 static int pstore_write_compat(enum pstore_type_id type,
@@ -467,12 +486,28 @@ int pstore_register(struct pstore_info *psi)
 	 */
 	backend = psi->name;
 
+	module_put(owner);
+
 	pr_info("Registered %s as persistent store backend\n", psi->name);
 
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pstore_register);
 
+void pstore_unregister(struct pstore_info *psi)
+{
+	pstore_unregister_pmsg();
+	pstore_unregister_ftrace();
+	pstore_unregister_console();
+	pstore_unregister_kmsg();
+
+	free_buf_for_compression();
+
+	psinfo = NULL;
+	backend = NULL;
+}
+EXPORT_SYMBOL_GPL(pstore_unregister);
+
 /*
  * Read all the records from the persistent store. Create
  * files in our filesystem.  Don't warn about -EEXIST errors
diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
index 5a2f05a..7de20cd 100644
--- a/fs/pstore/pmsg.c
+++ b/fs/pstore/pmsg.c
@@ -114,3 +114,10 @@ err_class:
 err:
 	return;
 }
+
+void pstore_unregister_pmsg(void)
+{
+	device_destroy(pmsg_class, MKDEV(pmsg_major, 0));
+	class_destroy(pmsg_class);
+	unregister_chrdev(pmsg_major, PMSG_NAME);
+}
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 6c26c4d..68889a7 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -580,28 +580,25 @@ fail_out:
 
 static int __exit ramoops_remove(struct platform_device *pdev)
 {
-#if 0
-	/* TODO(kees): We cannot unload ramoops since pstore doesn't support
-	 * unregistering yet.
-	 */
 	struct ramoops_context *cxt = &oops_cxt;
 
-	iounmap(cxt->virt_addr);
-	release_mem_region(cxt->phys_addr, cxt->size);
+	pstore_unregister(&cxt->pstore);
 	cxt->max_dump_cnt = 0;
 
-	/* TODO(kees): When pstore supports unregistering, call it here. */
 	kfree(cxt->pstore.buf);
 	cxt->pstore.bufsize = 0;
 
+	persistent_ram_free(cxt->mprz);
+	persistent_ram_free(cxt->fprz);
+	persistent_ram_free(cxt->cprz);
+	ramoops_free_przs(cxt);
+
 	return 0;
-#endif
-	return -EBUSY;
 }
 
 static struct platform_driver ramoops_driver = {
 	.probe		= ramoops_probe,
-	.remove		= __exit_p(ramoops_remove),
+	.remove		= ramoops_remove,
 	.driver		= {
 		.name	= "ramoops",
 	},
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 8e7a25b..831479f 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -75,20 +75,8 @@ struct pstore_info {
 
 #define	PSTORE_FLAGS_FRAGILE	1
 
-#ifdef CONFIG_PSTORE
 extern int pstore_register(struct pstore_info *);
+extern void pstore_unregister(struct pstore_info *);
 extern bool pstore_cannot_block_path(enum kmsg_dump_reason reason);
-#else
-static inline int
-pstore_register(struct pstore_info *psi)
-{
-	return -ENODEV;
-}
-static inline bool
-pstore_cannot_block_path(enum kmsg_dump_reason reason)
-{
-	return false;
-}
-#endif
 
 #endif /*_LINUX_PSTORE_H*/
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 8f0324e..b16f354 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -517,6 +517,7 @@ int check_syslog_permissions(int type, int source)
 ok:
 	return security_syslog(type);
 }
+EXPORT_SYMBOL_GPL(check_syslog_permissions);
 
 static void append_char(char **pp, char *e, char c)
 {
-- 
1.9.1

Re: [PATCH v3 0/3] pstore: add pstore unregister

[Kees Cook]

On Tue, Oct 20, 2015 at 12:39 AM, Geliang Tang <geliangtang@163.com> wrote:
> On Mon, Oct 19, 2015 at 10:56:54PM +0000, Luck, Tony wrote:
>> Thanks for looking to close out this TODO item.
>>
>> The thing that scared me about unloading pstore was what happens to
>> a process that is in the middle of reading some /sys/fs/pstore/file-name-here

Were you able to verify that this reading-while-rmmod case works correctly?

-Kees

>>
>> Do we have all the right reference counts to make sure that process doesn't do
>> weird things if you rmmod pstore in the middle of a read? Or for a subsequent
>> read from the still-open file descriptor?
>>
>> -Tony
>
> Thanks for your review. I updated the patches as you suggested.
>
> // Increase a reference count when pstore file is read.
>  static const struct file_operations pstore_file_operations = {
> +       .owner          = THIS_MODULE,
>         .open           = pstore_file_open,
>         .read           = pstore_file_read,
>         .llseek         = pstore_file_llseek,
>
> // Increase a reference count when pstore is mounted.
>  static struct file_system_type pstore_fs_type = {
> +       .owner          = THIS_MODULE,
>         .name           = "pstore",
>         .mount          = pstore_mount,
>         .kill_sb        = pstore_kill_sb,
>
> ---
> Changes in v3:
>  - Increase a reference count when pstore is used.
> Changes in v2:
>  - Add pstore filesystem unregister.
>  - update commit log.
> ---
>
> Geliang Tang (3):
>   pstore: add vmalloc error check
>   pstore: add a helper function pstore_register_kmsg
>   pstore: add pstore unregister
>
>  fs/pstore/Kconfig      |  2 +-
>  fs/pstore/Makefile     |  6 +++---
>  fs/pstore/ftrace.c     | 23 ++++++++++++++++++-----
>  fs/pstore/inode.c      |  9 +++++++++
>  fs/pstore/internal.h   |  4 ++++
>  fs/pstore/platform.c   | 42 +++++++++++++++++++++++++++++++++++++++++-
>  fs/pstore/pmsg.c       |  9 +++++++++
>  fs/pstore/ram.c        | 17 +++++++----------
>  include/linux/pstore.h | 14 +-------------
>  kernel/printk/printk.c |  1 +
>  10 files changed, 94 insertions(+), 33 deletions(-)
>
> --
> 1.9.1
>
>



-- 
Kees Cook
Chrome OS Security

Re: [PATCH v3 0/3] pstore: add pstore unregister

[Geliang Tang]

On Tue, Oct 20, 2015 at 10:19:09AM -0700, Kees Cook wrote:
> On Tue, Oct 20, 2015 at 12:39 AM, Geliang Tang <geliangtang@163.com> wrote:
> > On Mon, Oct 19, 2015 at 10:56:54PM +0000, Luck, Tony wrote:
> >> Thanks for looking to close out this TODO item.
> >>
> >> The thing that scared me about unloading pstore was what happens to
> >> a process that is in the middle of reading some /sys/fs/pstore/file-name-here
> 
> Were you able to verify that this reading-while-rmmod case works correctly?
> 
> -Kees

$ sudo insmod zlib_deflate.ko
$ sudo insmod pstore.ko
$ lsmod
Module                  Size  Used by
pstore                 11225  0
zlib_deflate           18292  1 pstore

$ sudo mount -t pstore pstore /sys/fs/pstore
$ lsmod
Module                  Size  Used by
pstore                 11225  1
zlib_deflate           18292  1 pstore

$ sudo insmod reed_solomon.ko
$ sudo insmod ramoops.ko mem_address=0x40000000 mem_size=0x400000
$ lsmod
Module                  Size  Used by
ramoops                 9638  0
reed_solomon            5150  1 ramoops
pstore                 11225  2 ramoops
zlib_deflate           18292  1 pstore

$ tail -f /sys/fs/pstore/console-ramoops-0 &
[1] 3483
$ lsmod
Module                  Size  Used by
ramoops                 9638  0
reed_solomon            5150  1 ramoops
pstore                 11225  3 ramoops
zlib_deflate           18292  1 pstore

$ kill -9 3483
$ lsmod
Module                  Size  Used by
ramoops                 9638  0
reed_solomon            5150  1 ramoops
pstore                 11225  2 ramoops
zlib_deflate           18292  1 pstore

$ sudo rmmod ramoops
$ lsmod
Module                  Size  Used by
reed_solomon            5150  0
pstore                 11225  1
zlib_deflate           18292  1 pstore

$ sudo umount /sys/fs/pstore/
$ lsmod
Module                  Size  Used by
reed_solomon            5150  0 
pstore                 11225  0 
zlib_deflate           18292  1 pstore

$ sudo rmmod pstore
$ lsmod
Module                  Size  Used by
reed_solomon            5150  0 
zlib_deflate           18292  0

Thanks.
Geliang Tang

Re: [PATCH v3 0/3] pstore: add pstore unregister

[Kees Cook]

On Tue, Oct 20, 2015 at 7:52 PM, Geliang Tang <geliangtang@163.com> wrote:
> On Tue, Oct 20, 2015 at 10:19:09AM -0700, Kees Cook wrote:
>> On Tue, Oct 20, 2015 at 12:39 AM, Geliang Tang <geliangtang@163.com> wrote:
>> > On Mon, Oct 19, 2015 at 10:56:54PM +0000, Luck, Tony wrote:
>> >> Thanks for looking to close out this TODO item.
>> >>
>> >> The thing that scared me about unloading pstore was what happens to
>> >> a process that is in the middle of reading some /sys/fs/pstore/file-name-here
>>
>> Were you able to verify that this reading-while-rmmod case works correctly?
>>
>> -Kees
>
> $ sudo insmod zlib_deflate.ko
> $ sudo insmod pstore.ko
> $ lsmod
> Module                  Size  Used by
> pstore                 11225  0
> zlib_deflate           18292  1 pstore
>
> $ sudo mount -t pstore pstore /sys/fs/pstore
> $ lsmod
> Module                  Size  Used by
> pstore                 11225  1
> zlib_deflate           18292  1 pstore
>
> $ sudo insmod reed_solomon.ko
> $ sudo insmod ramoops.ko mem_address=0x40000000 mem_size=0x400000
> $ lsmod
> Module                  Size  Used by
> ramoops                 9638  0
> reed_solomon            5150  1 ramoops
> pstore                 11225  2 ramoops
> zlib_deflate           18292  1 pstore
>
> $ tail -f /sys/fs/pstore/console-ramoops-0 &
> [1] 3483
> $ lsmod
> Module                  Size  Used by
> ramoops                 9638  0
> reed_solomon            5150  1 ramoops
> pstore                 11225  3 ramoops
> zlib_deflate           18292  1 pstore
>
> $ kill -9 3483
> $ lsmod
> Module                  Size  Used by
> ramoops                 9638  0
> reed_solomon            5150  1 ramoops
> pstore                 11225  2 ramoops
> zlib_deflate           18292  1 pstore
>
> $ sudo rmmod ramoops
> $ lsmod
> Module                  Size  Used by
> reed_solomon            5150  0
> pstore                 11225  1
> zlib_deflate           18292  1 pstore

What happens if you leave the tail running and try to rmmod ramoops?
(I assume it'll just refuse.)

Nice!

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

>
> $ sudo umount /sys/fs/pstore/
> $ lsmod
> Module                  Size  Used by
> reed_solomon            5150  0
> pstore                 11225  0
> zlib_deflate           18292  1 pstore
>
> $ sudo rmmod pstore
> $ lsmod
> Module                  Size  Used by
> reed_solomon            5150  0
> zlib_deflate           18292  0
>
> Thanks.
> Geliang Tang
>



-- 
Kees Cook
Chrome OS Security

Re: [PATCH v3 0/3] pstore: add pstore unregister

[Geliang Tang]

On Tue, Oct 20, 2015 at 08:01:20PM -0700, Kees Cook wrote:
> On Tue, Oct 20, 2015 at 7:52 PM, Geliang Tang <geliangtang@163.com> wrote:
> > On Tue, Oct 20, 2015 at 10:19:09AM -0700, Kees Cook wrote:
> >> On Tue, Oct 20, 2015 at 12:39 AM, Geliang Tang <geliangtang@163.com> wrote:
> >> > On Mon, Oct 19, 2015 at 10:56:54PM +0000, Luck, Tony wrote:
> >> >> Thanks for looking to close out this TODO item.
> >> >>
> >> >> The thing that scared me about unloading pstore was what happens to
> >> >> a process that is in the middle of reading some /sys/fs/pstore/file-name-here
> >>
> >> Were you able to verify that this reading-while-rmmod case works correctly?
> >>
> >> -Kees
> >
> > $ sudo insmod zlib_deflate.ko
> > $ sudo insmod pstore.ko
> > $ lsmod
> > Module                  Size  Used by
> > pstore                 11225  0
> > zlib_deflate           18292  1 pstore
> >
> > $ sudo mount -t pstore pstore /sys/fs/pstore
> > $ lsmod
> > Module                  Size  Used by
> > pstore                 11225  1
> > zlib_deflate           18292  1 pstore
> >
> > $ sudo insmod reed_solomon.ko
> > $ sudo insmod ramoops.ko mem_address=0x40000000 mem_size=0x400000
> > $ lsmod
> > Module                  Size  Used by
> > ramoops                 9638  0
> > reed_solomon            5150  1 ramoops
> > pstore                 11225  2 ramoops
> > zlib_deflate           18292  1 pstore
> >
> > $ tail -f /sys/fs/pstore/console-ramoops-0 &
> > [1] 3483
> > $ lsmod
> > Module                  Size  Used by
> > ramoops                 9638  0
> > reed_solomon            5150  1 ramoops
> > pstore                 11225  3 ramoops
> > zlib_deflate           18292  1 pstore
> >
> > $ kill -9 3483
> > $ lsmod
> > Module                  Size  Used by
> > ramoops                 9638  0
> > reed_solomon            5150  1 ramoops
> > pstore                 11225  2 ramoops
> > zlib_deflate           18292  1 pstore
> >
> > $ sudo rmmod ramoops
> > $ lsmod
> > Module                  Size  Used by
> > reed_solomon            5150  0
> > pstore                 11225  1
> > zlib_deflate           18292  1 pstore
> 
> What happens if you leave the tail running and try to rmmod ramoops?
> (I assume it'll just refuse.)
> 

It'll refuse if we try to unload pstore module. But we can unload
ramoops module. Because we only increase a reference count of pstore
module when the file is opened. We didn't increase ramoops module's
reference count.

Thanks.
Geliang Tang

RE: [PATCH v3 0/3] pstore: add pstore unregister

[Luck, Tony]

> It'll refuse if we try to unload pstore module. But we can unload
> ramoops module. Because we only increase a reference count of pstore
> module when the file is opened. We didn't increase ramoops module's
> reference count.

Thanks.  Series applied.

I changed:

part1: re-worded commit a little
part2: spelling fix s/wraper/wrapper/
part3: fixed one "line over 80 character" checkpatch warning

Assuming the 0-day robots don't find any additional problems this should
show up in linux-next in a day or two. I'll ask Linus to pull in the 4.4 merge
window.

-Tony