[PATCH 00/10] pstore: Remove filesystem records when backend is unregistered

[PATCH 00/10] pstore: Remove filesystem records when backend is unregistered

[Kees Cook]

Hi,

This fixes a long-standing problem[1] with pstore where the filesystem
view of backend records was not updated when the backend was unloaded
(in a modular build) through pstore_unregister(). This series is
mostly refactoring and improvements to the various locking semantics
around management of the active backend and the filesystem mount before
ultimately providing the routine to walk the filesystem to remove the
records associated with a given backend.

I'm still doing more build and runtime testing, but I just wanted to get
this posted so I can let other people look at it if they want while the
testing finishes.

Thanks!

-Kees

[1] https://lore.kernel.org/lkml/87o8yrmv69.fsf@suse.com

Kees Cook (10):
  pstore: Drop useless try_module_get() for backend
  pstore: Rename "pstore_lock" to "psinfo_lock"
  pstore: Convert "psinfo" locking to mutex
  pstore: Rename "allpstore" to "records_list"
  pstore: Convert "records_list" locking to mutex
  pstore: Add proper unregister lock checking
  pstore: Refactor pstorefs record list removal
  pstore: Add locking around superblock changes
  pstore: Do not leave timer disabled for next backend
  pstore: Remove filesystem records when backend is unregistered

 fs/pstore/inode.c    | 127 +++++++++++++++++++++++++++++++------------
 fs/pstore/internal.h |   2 +-
 fs/pstore/platform.c |  72 ++++++++++++++----------
 3 files changed, 134 insertions(+), 67 deletions(-)

-- 
2.20.1

[PATCH 01/10] pstore: Drop useless try_module_get() for backend

[Kees Cook]

There is no reason to be doing a module get/put in pstore_register(),
since the module calling pstore_register() cannot be unloaded since it
hasn't finished its initialization. Remove it so there is no confusion
about how registration ordering works.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/platform.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 408277ee3cdb..44f8b9742263 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -555,8 +555,6 @@ static int pstore_write_user_compat(struct pstore_record *record,
  */
 int pstore_register(struct pstore_info *psi)
 {
-	struct module *owner = psi->owner;
-
 	if (backend && strcmp(backend, psi->name)) {
 		pr_warn("ignoring unexpected backend '%s'\n", psi->name);
 		return -EPERM;
@@ -591,10 +589,6 @@ int pstore_register(struct pstore_info *psi)
 	sema_init(&psinfo->buf_lock, 1);
 	spin_unlock(&pstore_lock);
 
-	if (owner && !try_module_get(owner)) {
-		psinfo = NULL;
-		return -EINVAL;
-	}
 
 	if (psi->flags & PSTORE_FLAGS_DMESG)
 		allocate_buf_for_compression();
@@ -626,8 +620,6 @@ int pstore_register(struct pstore_info *psi)
 
 	pr_info("Registered %s as persistent store backend\n", psi->name);
 
-	module_put(owner);
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pstore_register);
-- 
2.20.1

[PATCH 02/10] pstore: Rename "pstore_lock" to "psinfo_lock"

[Kees Cook]

The name "pstore_lock" sounds very global, but it is only supposed to be
used for managing changes to "psinfo", so rename it accordingly.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/platform.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 44f8b9742263..347b6c07f4cf 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -69,10 +69,10 @@ static void pstore_dowork(struct work_struct *);
 static DECLARE_WORK(pstore_work, pstore_dowork);
 
 /*
- * pstore_lock just protects "psinfo" during
+ * psinfo_lock just protects "psinfo" during
  * calls to pstore_register()
  */
-static DEFINE_SPINLOCK(pstore_lock);
+static DEFINE_SPINLOCK(psinfo_lock);
 struct pstore_info *psinfo;
 
 static char *backend;
@@ -574,11 +574,11 @@ int pstore_register(struct pstore_info *psi)
 		return -EINVAL;
 	}
 
-	spin_lock(&pstore_lock);
+	spin_lock(&psinfo_lock);
 	if (psinfo) {
 		pr_warn("backend '%s' already loaded: ignoring '%s'\n",
 			psinfo->name, psi->name);
-		spin_unlock(&pstore_lock);
+		spin_unlock(&psinfo_lock);
 		return -EBUSY;
 	}
 
@@ -587,7 +587,7 @@ int pstore_register(struct pstore_info *psi)
 	psinfo = psi;
 	mutex_init(&psinfo->read_mutex);
 	sema_init(&psinfo->buf_lock, 1);
-	spin_unlock(&pstore_lock);
+	spin_unlock(&psinfo_lock);
 
 
 	if (psi->flags & PSTORE_FLAGS_DMESG)
-- 
2.20.1

[PATCH 03/10] pstore: Convert "psinfo" locking to mutex

[Kees Cook]

Currently pstore can only have a single backend attached at a time, and it
tracks the active backend via "psinfo", under a lock. The locking for this
does not need to be a spinlock, and in order to avoid may_sleep() issues
during future changes to pstore_unregister(), switch to a mutex instead.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/platform.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 347b6c07f4cf..d0ce22237589 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -72,7 +72,7 @@ static DECLARE_WORK(pstore_work, pstore_dowork);
  * psinfo_lock just protects "psinfo" during
  * calls to pstore_register()
  */
-static DEFINE_SPINLOCK(psinfo_lock);
+static DEFINE_MUTEX(psinfo_lock);
 struct pstore_info *psinfo;
 
 static char *backend;
@@ -574,11 +574,11 @@ int pstore_register(struct pstore_info *psi)
 		return -EINVAL;
 	}
 
-	spin_lock(&psinfo_lock);
+	mutex_lock(&psinfo_lock);
 	if (psinfo) {
 		pr_warn("backend '%s' already loaded: ignoring '%s'\n",
 			psinfo->name, psi->name);
-		spin_unlock(&psinfo_lock);
+		mutex_unlock(&psinfo_lock);
 		return -EBUSY;
 	}
 
@@ -587,7 +587,7 @@ int pstore_register(struct pstore_info *psi)
 	psinfo = psi;
 	mutex_init(&psinfo->read_mutex);
 	sema_init(&psinfo->buf_lock, 1);
-	spin_unlock(&psinfo_lock);
+	mutex_unlock(&psinfo_lock);
 
 
 	if (psi->flags & PSTORE_FLAGS_DMESG)
-- 
2.20.1

[PATCH 04/10] pstore: Rename "allpstore" to "records_list"

[Kees Cook]

The name "allpstore" doesn't carry much meaning, so rename it to what it
actually is: the list of all records present in the filesystem. The lock
is also renamed accordingly.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/inode.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index d99b5d39aa90..5cc09cb315f9 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -29,8 +29,8 @@
 
 #define	PSTORE_NAMELEN	64
 
-static DEFINE_SPINLOCK(allpstore_lock);
-static LIST_HEAD(allpstore);
+static DEFINE_SPINLOCK(records_list_lock);
+static LIST_HEAD(records_list);
 
 struct pstore_private {
 	struct list_head list;
@@ -196,9 +196,9 @@ static void pstore_evict_inode(struct inode *inode)
 
 	clear_inode(inode);
 	if (p) {
-		spin_lock_irqsave(&allpstore_lock, flags);
+		spin_lock_irqsave(&records_list_lock, flags);
 		list_del(&p->list);
-		spin_unlock_irqrestore(&allpstore_lock, flags);
+		spin_unlock_irqrestore(&records_list_lock, flags);
 		free_pstore_private(p);
 	}
 }
@@ -302,8 +302,8 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 
 	WARN_ON(!inode_is_locked(d_inode(root)));
 
-	spin_lock_irqsave(&allpstore_lock, flags);
-	list_for_each_entry(pos, &allpstore, list) {
+	spin_lock_irqsave(&records_list_lock, flags);
+	list_for_each_entry(pos, &records_list, list) {
 		if (pos->record->type == record->type &&
 		    pos->record->id == record->id &&
 		    pos->record->psi == record->psi) {
@@ -311,7 +311,7 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 			break;
 		}
 	}
-	spin_unlock_irqrestore(&allpstore_lock, flags);
+	spin_unlock_irqrestore(&records_list_lock, flags);
 	if (rc)
 		return rc;
 
@@ -343,9 +343,9 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 
 	d_add(dentry, inode);
 
-	spin_lock_irqsave(&allpstore_lock, flags);
-	list_add(&private->list, &allpstore);
-	spin_unlock_irqrestore(&allpstore_lock, flags);
+	spin_lock_irqsave(&records_list_lock, flags);
+	list_add(&private->list, &records_list);
+	spin_unlock_irqrestore(&records_list_lock, flags);
 
 	return 0;
 
-- 
2.20.1

[PATCH 05/10] pstore: Convert "records_list" locking to mutex

[Kees Cook]

The pstorefs internal list lock doesn't need to be a spinlock and will
create problems when trying to access the list in the subsequent patch
that will walk the pstorefs records during pstore_unregister(). Change
this to a mutex to avoid may_sleep() warnings when unregistering devices.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/inode.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 5cc09cb315f9..92ebcc75434f 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -22,14 +22,13 @@
 #include <linux/magic.h>
 #include <linux/pstore.h>
 #include <linux/slab.h>
-#include <linux/spinlock.h>
 #include <linux/uaccess.h>
 
 #include "internal.h"
 
 #define	PSTORE_NAMELEN	64
 
-static DEFINE_SPINLOCK(records_list_lock);
+static DEFINE_MUTEX(records_list_lock);
 static LIST_HEAD(records_list);
 
 struct pstore_private {
@@ -192,13 +191,12 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
 static void pstore_evict_inode(struct inode *inode)
 {
 	struct pstore_private	*p = inode->i_private;
-	unsigned long		flags;
 
 	clear_inode(inode);
 	if (p) {
-		spin_lock_irqsave(&records_list_lock, flags);
+		mutex_lock(&records_list_lock);
 		list_del(&p->list);
-		spin_unlock_irqrestore(&records_list_lock, flags);
+		mutex_unlock(&records_list_lock);
 		free_pstore_private(p);
 	}
 }
@@ -297,12 +295,11 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 	int			rc = 0;
 	char			name[PSTORE_NAMELEN];
 	struct pstore_private	*private, *pos;
-	unsigned long		flags;
 	size_t			size = record->size + record->ecc_notice_size;
 
 	WARN_ON(!inode_is_locked(d_inode(root)));
 
-	spin_lock_irqsave(&records_list_lock, flags);
+	mutex_lock(&records_list_lock);
 	list_for_each_entry(pos, &records_list, list) {
 		if (pos->record->type == record->type &&
 		    pos->record->id == record->id &&
@@ -311,7 +308,7 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 			break;
 		}
 	}
-	spin_unlock_irqrestore(&records_list_lock, flags);
+	mutex_unlock(&records_list_lock);
 	if (rc)
 		return rc;
 
@@ -343,9 +340,9 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 
 	d_add(dentry, inode);
 
-	spin_lock_irqsave(&records_list_lock, flags);
+	mutex_lock(&records_list_lock);
 	list_add(&private->list, &records_list);
-	spin_unlock_irqrestore(&records_list_lock, flags);
+	mutex_unlock(&records_list_lock);
 
 	return 0;
 
-- 
2.20.1

[PATCH 06/10] pstore: Add proper unregister lock checking

[Kees Cook]

The pstore backend lock wasn't being used during pstore_unregister().
Add sanity check and locking.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/platform.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index d0ce22237589..03bc847a6951 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -69,8 +69,9 @@ static void pstore_dowork(struct work_struct *);
 static DECLARE_WORK(pstore_work, pstore_dowork);
 
 /*
- * psinfo_lock just protects "psinfo" during
- * calls to pstore_register()
+ * psinfo_lock protects "psinfo" during calls to
+ * pstore_register(), pstore_unregister(), and
+ * the filesystem mount/unmount routines.
  */
 static DEFINE_MUTEX(psinfo_lock);
 struct pstore_info *psinfo;
@@ -626,6 +627,18 @@ EXPORT_SYMBOL_GPL(pstore_register);
 
 void pstore_unregister(struct pstore_info *psi)
 {
+	/* It's okay to unregister nothing. */
+	if (!psi)
+		return;
+
+	mutex_lock(&psinfo_lock);
+
+	/* Only one backend can be registered at a time. */
+	if (WARN_ON(psi != psinfo)) {
+		mutex_unlock(&psinfo_lock);
+		return;
+	}
+
 	/* Stop timer and make sure all work has finished. */
 	pstore_update_ms = -1;
 	del_timer_sync(&pstore_timer);
@@ -644,6 +657,7 @@ void pstore_unregister(struct pstore_info *psi)
 
 	psinfo = NULL;
 	backend = NULL;
+	mutex_unlock(&psinfo_lock);
 }
 EXPORT_SYMBOL_GPL(pstore_unregister);
 
-- 
2.20.1

[PATCH 07/10] pstore: Refactor pstorefs record list removal

[Kees Cook]

The "unlink" handling should perform list removal (which can also make
sure records don't get double-erased), and the "evict" handling should
be responsible only for memory freeing.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/inode.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 92ebcc75434f..5f08b21b7a46 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -177,10 +177,21 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
 {
 	struct pstore_private *p = d_inode(dentry)->i_private;
 	struct pstore_record *record = p->record;
+	int rc = 0;
 
 	if (!record->psi->erase)
 		return -EPERM;
 
+	/* Make sure we can't race while removing this file. */
+	mutex_lock(&records_list_lock);
+	if (!list_empty(&p->list))
+		list_del_init(&p->list);
+	else
+		rc = -ENOENT;
+	mutex_unlock(&records_list_lock);
+	if (rc)
+		return rc;
+
 	mutex_lock(&record->psi->read_mutex);
 	record->psi->erase(record);
 	mutex_unlock(&record->psi->read_mutex);
@@ -193,12 +204,7 @@ static void pstore_evict_inode(struct inode *inode)
 	struct pstore_private	*p = inode->i_private;
 
 	clear_inode(inode);
-	if (p) {
-		mutex_lock(&records_list_lock);
-		list_del(&p->list);
-		mutex_unlock(&records_list_lock);
-		free_pstore_private(p);
-	}
+	free_pstore_private(p);
 }
 
 static const struct inode_operations pstore_dir_inode_operations = {
@@ -417,6 +423,7 @@ static void pstore_kill_sb(struct super_block *sb)
 {
 	kill_litter_super(sb);
 	pstore_sb = NULL;
+	INIT_LIST_HEAD(&records_list);
 }
 
 static struct file_system_type pstore_fs_type = {
-- 
2.20.1

[PATCH 08/10] pstore: Add locking around superblock changes

[Kees Cook]

Nothing was protecting changes to the pstorefs superblock. Add locking
and refactor away is_pstore_mounted(), instead using a helper to add a
way to safely lock the pstorefs root inode during filesystem changes.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/inode.c    | 65 +++++++++++++++++++++++++++++---------------
 fs/pstore/internal.h |  1 -
 fs/pstore/platform.c |  5 ++--
 3 files changed, 45 insertions(+), 26 deletions(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 5f08b21b7a46..e13482c8e180 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -31,6 +31,9 @@
 static DEFINE_MUTEX(records_list_lock);
 static LIST_HEAD(records_list);
 
+static DEFINE_MUTEX(pstore_sb_lock);
+static struct super_block *pstore_sb;
+
 struct pstore_private {
 	struct list_head list;
 	struct pstore_record *record;
@@ -282,11 +285,25 @@ static const struct super_operations pstore_ops = {
 	.show_options	= pstore_show_options,
 };
 
-static struct super_block *pstore_sb;
-
-bool pstore_is_mounted(void)
+struct dentry *psinfo_lock_root(void)
 {
-	return pstore_sb != NULL;
+	struct dentry *root;
+
+	mutex_lock(&pstore_sb_lock);
+	/*
+	 * Having no backend is fine -- no records appear.
+	 * Not being mounted is fine -- nothing to do.
+	 */
+	if (!psinfo || !pstore_sb) {
+		mutex_unlock(&pstore_sb_lock);
+		return NULL;
+	}
+
+	root = pstore_sb->s_root;
+	inode_lock(d_inode(root));
+	mutex_unlock(&pstore_sb_lock);
+
+	return root;
 }
 
 /*
@@ -303,20 +320,18 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 	struct pstore_private	*private, *pos;
 	size_t			size = record->size + record->ecc_notice_size;
 
-	WARN_ON(!inode_is_locked(d_inode(root)));
+	if (WARN_ON(!inode_is_locked(d_inode(root))))
+		return -EINVAL;
 
+	rc = -EEXIST;
+	/* Skip records that are already present in the filesystem. */
 	mutex_lock(&records_list_lock);
 	list_for_each_entry(pos, &records_list, list) {
 		if (pos->record->type == record->type &&
 		    pos->record->id == record->id &&
-		    pos->record->psi == record->psi) {
-			rc = -EEXIST;
-			break;
-		}
+		    pos->record->psi == record->psi)
+			goto fail;
 	}
-	mutex_unlock(&records_list_lock);
-	if (rc)
-		return rc;
 
 	rc = -ENOMEM;
 	inode = pstore_get_inode(root->d_sb);
@@ -346,7 +361,6 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 
 	d_add(dentry, inode);
 
-	mutex_lock(&records_list_lock);
 	list_add(&private->list, &records_list);
 	mutex_unlock(&records_list_lock);
 
@@ -356,8 +370,8 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 	free_pstore_private(private);
 fail_inode:
 	iput(inode);
-
 fail:
+	mutex_unlock(&records_list_lock);
 	return rc;
 }
 
@@ -369,16 +383,13 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
  */
 void pstore_get_records(int quiet)
 {
-	struct pstore_info *psi = psinfo;
 	struct dentry *root;
 
-	if (!psi || !pstore_sb)
+	root = psinfo_lock_root();
+	if (!root)
 		return;
 
-	root = pstore_sb->s_root;
-
-	inode_lock(d_inode(root));
-	pstore_get_backend_records(psi, root, quiet);
+	pstore_get_backend_records(psinfo, root, quiet);
 	inode_unlock(d_inode(root));
 }
 
@@ -386,8 +397,6 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent)
 {
 	struct inode *inode;
 
-	pstore_sb = sb;
-
 	sb->s_maxbytes		= MAX_LFS_FILESIZE;
 	sb->s_blocksize		= PAGE_SIZE;
 	sb->s_blocksize_bits	= PAGE_SHIFT;
@@ -408,6 +417,10 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent)
 	if (!sb->s_root)
 		return -ENOMEM;
 
+	mutex_lock(&pstore_sb_lock);
+	pstore_sb = sb;
+	mutex_unlock(&pstore_sb_lock);
+
 	pstore_get_records(0);
 
 	return 0;
@@ -421,9 +434,17 @@ static struct dentry *pstore_mount(struct file_system_type *fs_type,
 
 static void pstore_kill_sb(struct super_block *sb)
 {
+	mutex_lock(&pstore_sb_lock);
+	WARN_ON(pstore_sb != sb);
+
 	kill_litter_super(sb);
 	pstore_sb = NULL;
+
+	mutex_lock(&records_list_lock);
 	INIT_LIST_HEAD(&records_list);
+	mutex_unlock(&records_list_lock);
+
+	mutex_unlock(&pstore_sb_lock);
 }
 
 static struct file_system_type pstore_fs_type = {
diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
index 7062ea4bc57c..fe5f7ef7323f 100644
--- a/fs/pstore/internal.h
+++ b/fs/pstore/internal.h
@@ -33,7 +33,6 @@ extern void	pstore_get_backend_records(struct pstore_info *psi,
 					   struct dentry *root, int quiet);
 extern int	pstore_mkfile(struct dentry *root,
 			      struct pstore_record *record);
-extern bool	pstore_is_mounted(void);
 extern void	pstore_record_init(struct pstore_record *record,
 				   struct pstore_info *psi);
 
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 03bc847a6951..55f46837a7f4 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -460,7 +460,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 		}
 
 		ret = psinfo->write(&record);
-		if (ret == 0 && reason == KMSG_DUMP_OOPS && pstore_is_mounted())
+		if (ret == 0 && reason == KMSG_DUMP_OOPS)
 			pstore_new_entry = 1;
 
 		total += record.size;
@@ -594,8 +594,7 @@ int pstore_register(struct pstore_info *psi)
 	if (psi->flags & PSTORE_FLAGS_DMESG)
 		allocate_buf_for_compression();
 
-	if (pstore_is_mounted())
-		pstore_get_records(0);
+	pstore_get_records(0);
 
 	if (psi->flags & PSTORE_FLAGS_DMESG)
 		pstore_register_kmsg();
-- 
2.20.1

[PATCH 09/10] pstore: Do not leave timer disabled for next backend

[Kees Cook]

The pstore.update_ms value was being disabled during pstore_unregister(),
which would cause any prior value to go unnoticed on the next
pstore_register(). Instead, just let del_timer() stop the timer, which
was always sufficient. This additionally refactors the timer reset code
and allows the timer to be enabled if the module parameter is changed
away from the default.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/platform.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 55f46837a7f4..03a17b401533 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -44,7 +44,7 @@ static int pstore_update_ms = -1;
 module_param_named(update_ms, pstore_update_ms, int, 0600);
 MODULE_PARM_DESC(update_ms, "milliseconds before pstore updates its content "
 		 "(default is -1, which means runtime updates are disabled; "
-		 "enabling this option is not safe, it may lead to further "
+		 "enabling this option may not be safe; it may lead to further "
 		 "corruption on Oopses)");
 
 /* Names should be in the same order as the enum pstore_type_id */
@@ -150,6 +150,14 @@ static const char *get_reason_str(enum kmsg_dump_reason reason)
 	}
 }
 
+static void pstore_timer_kick(void)
+{
+	if (pstore_update_ms < 0)
+		return;
+
+	mod_timer(&pstore_timer, jiffies + msecs_to_jiffies(pstore_update_ms));
+}
+
 /*
  * Should pstore_dump() wait for a concurrent pstore_dump()? If
  * not, the current pstore_dump() will report a failure to dump
@@ -460,8 +468,10 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 		}
 
 		ret = psinfo->write(&record);
-		if (ret == 0 && reason == KMSG_DUMP_OOPS)
+		if (ret == 0 && reason == KMSG_DUMP_OOPS) {
 			pstore_new_entry = 1;
+			pstore_timer_kick();
+		}
 
 		total += record.size;
 		part++;
@@ -606,11 +616,7 @@ int pstore_register(struct pstore_info *psi)
 		pstore_register_pmsg();
 
 	/* Start watching for new records, if desired. */
-	if (pstore_update_ms >= 0) {
-		pstore_timer.expires = jiffies +
-			msecs_to_jiffies(pstore_update_ms);
-		add_timer(&pstore_timer);
-	}
+	pstore_timer_kick();
 
 	/*
 	 * Update the module parameter backend, so it is visible
@@ -638,11 +644,7 @@ void pstore_unregister(struct pstore_info *psi)
 		return;
 	}
 
-	/* Stop timer and make sure all work has finished. */
-	pstore_update_ms = -1;
-	del_timer_sync(&pstore_timer);
-	flush_work(&pstore_work);
-
+	/* Unregister all callbacks. */
 	if (psi->flags & PSTORE_FLAGS_PMSG)
 		pstore_unregister_pmsg();
 	if (psi->flags & PSTORE_FLAGS_FTRACE)
@@ -652,6 +654,10 @@ void pstore_unregister(struct pstore_info *psi)
 	if (psi->flags & PSTORE_FLAGS_DMESG)
 		pstore_unregister_kmsg();
 
+	/* Stop timer and make sure all work has finished. */
+	del_timer_sync(&pstore_timer);
+	flush_work(&pstore_work);
+
 	free_buf_for_compression();
 
 	psinfo = NULL;
@@ -793,9 +799,7 @@ static void pstore_timefunc(struct timer_list *unused)
 		schedule_work(&pstore_work);
 	}
 
-	if (pstore_update_ms >= 0)
-		mod_timer(&pstore_timer,
-			  jiffies + msecs_to_jiffies(pstore_update_ms));
+	pstore_timer_kick();
 }
 
 static void __init pstore_choose_compression(void)
-- 
2.20.1

[PATCH 10/10] pstore: Remove filesystem records when backend is unregistered

[Kees Cook]

If a backend was unloaded without having first removed all its
associated records in pstorefs, subsequent removals would crash while
attempting to call into the now missing backend. Add automatic removal
from the tree in pstore_unregister(), so that no references to the
backend remain.

Reported-by: Luis Henriques <lhenriques@suse.com>
Link: https://lore.kernel.org/lkml/87o8yrmv69.fsf@suse.com
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/pstore/inode.c    | 30 ++++++++++++++++++++++++++++++
 fs/pstore/internal.h |  1 +
 fs/pstore/platform.c |  3 +++
 3 files changed, 34 insertions(+)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index e13482c8e180..499d91a669bb 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -36,6 +36,7 @@ static struct super_block *pstore_sb;
 
 struct pstore_private {
 	struct list_head list;
+	struct dentry *dentry;
 	struct pstore_record *record;
 	size_t total_size;
 };
@@ -306,6 +307,34 @@ struct dentry *psinfo_lock_root(void)
 	return root;
 }
 
+int pstore_put_backend_records(struct pstore_info *psi)
+{
+	struct pstore_private *pos, *tmp;
+	struct dentry *root;
+	int rc = 0;
+
+	root = psinfo_lock_root();
+	if (!root)
+		return 0;
+
+	mutex_lock(&records_list_lock);
+	list_for_each_entry_safe(pos, tmp, &records_list, list) {
+		if (pos->record->psi == psi) {
+			list_del_init(&pos->list);
+			rc = simple_unlink(d_inode(root), pos->dentry);
+			if (WARN_ON(rc))
+				break;
+			d_delete(pos->dentry);
+			dput(pos->dentry);
+		}
+	}
+	mutex_unlock(&records_list_lock);
+
+	inode_unlock(d_inode(root));
+
+	return rc;
+}
+
 /*
  * Make a regular file in the root directory of our file system.
  * Load it up with "size" bytes of data from "buf".
@@ -352,6 +381,7 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
 	if (!dentry)
 		goto fail_private;
 
+	private->dentry = dentry;
 	private->record = record;
 	inode->i_size = private->total_size = size;
 	inode->i_private = private;
diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
index fe5f7ef7323f..8efd72d93b10 100644
--- a/fs/pstore/internal.h
+++ b/fs/pstore/internal.h
@@ -31,6 +31,7 @@ extern void	pstore_set_kmsg_bytes(int);
 extern void	pstore_get_records(int);
 extern void	pstore_get_backend_records(struct pstore_info *psi,
 					   struct dentry *root, int quiet);
+extern int	pstore_put_backend_records(struct pstore_info *psi);
 extern int	pstore_mkfile(struct dentry *root,
 			      struct pstore_record *record);
 extern void	pstore_record_init(struct pstore_record *record,
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 03a17b401533..6fb526187953 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -658,6 +658,9 @@ void pstore_unregister(struct pstore_info *psi)
 	del_timer_sync(&pstore_timer);
 	flush_work(&pstore_work);
 
+	/* Remove all backend records from filesystem tree. */
+	pstore_put_backend_records(psi);
+
 	free_buf_for_compression();
 
 	psinfo = NULL;
-- 
2.20.1