* [PATCH 1/3] pstore: add multi-backend support
2024-02-05 12:28 [PATCH v2 0/3] pstore: add multi-backend support Yuanhe Shu
@ 2024-02-05 12:28 ` Yuanhe Shu
2024-02-06 8:05 ` kernel test robot
2024-02-06 12:17 ` kernel test robot
0 siblings, 2 replies; 13+ messages in thread
From: Yuanhe Shu @ 2024-02-05 12:28 UTC (permalink / raw)
To: keescook, tony.luck, gpiccoli, shuah, corbet
Cc: xlpang, linux-kernel, linux-doc, linux-hardening, linux-kselftest,
Yuanhe Shu, Xingrui Yi
Currently, pstore supports only one backend open at a time.
Specifically, due to the global variable "psinfo", pstore only accepts
the first registered backend. If a new backend wants to register later,
pstore will simply reject it and return an error. This design forced us
to close existing backend in order to use the new ones.
To enable pstore to support multiple backends, "psinfo" is replaced by
"psinfo_list", a list that holds multiple "psinfo". If multiple backends
are registered with the same frontend, the frontend is reused.
User can specify multiple backends that are allowed to be registered by
module parameter "pstore.backend=" separated by commas or "all" to
enable all available backends. If no pstore.backend was specified,
pstore would accept the first registered backend which is the same as
before.
Signed-off-by: Xingrui Yi <yixingrui@linux.alibaba.com>
Signed-off-by: Yuanhe Shu <xiangzao@linux.alibaba.com>
---
fs/pstore/ftrace.c | 29 +++++-
fs/pstore/inode.c | 19 ++--
fs/pstore/internal.h | 4 +-
fs/pstore/platform.c | 225 ++++++++++++++++++++++++++++-------------
fs/pstore/pmsg.c | 24 ++++-
include/linux/pstore.h | 29 ++++++
6 files changed, 248 insertions(+), 82 deletions(-)
diff --git a/fs/pstore/ftrace.c b/fs/pstore/ftrace.c
index 776cae20af4e..2532a663aa2c 100644
--- a/fs/pstore/ftrace.c
+++ b/fs/pstore/ftrace.c
@@ -23,10 +23,11 @@
/* This doesn't need to be atomic: speed is chosen over correctness here. */
static u64 pstore_ftrace_stamp;
-static void notrace pstore_ftrace_call(unsigned long ip,
+static void notrace pstore_do_ftrace(unsigned long ip,
unsigned long parent_ip,
struct ftrace_ops *op,
- struct ftrace_regs *fregs)
+ struct ftrace_regs *fregs,
+ struct ftrace_info *psinfo)
{
int bit;
unsigned long flags;
@@ -57,6 +58,20 @@ static void notrace pstore_ftrace_call(unsigned long ip,
ftrace_test_recursion_unlock(bit);
}
+static void notrace pstore_ftrace_call(unsigned long ip,
+ unsigned long parent_ip,
+ struct ftrace_ops *op,
+ struct pt_regs *regs)
+{
+ struct pstore_info_list *entry;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, &psback->list_entry, list)
+ if (entry->psi->flags & PSTORE_FLAGS_FTRACE)
+ pstore_do_ftrace(ip, parent_ip, op, regs, entry->psi);
+ rcu_read_unlock();
+}
+
static struct ftrace_ops pstore_ftrace_ops __read_mostly = {
.func = pstore_ftrace_call,
};
@@ -131,8 +146,14 @@ MODULE_PARM_DESC(record_ftrace,
void pstore_register_ftrace(void)
{
- if (!psinfo->write)
- return;
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, &psback->list_entry, list)
+ if (entry->psi->flags & PSTORE_FLAGS_FTRACE)
+ if (!entry->psi->write) {
+ rcu_read_unlock();
+ return;
+ }
+ rcu_read_unlock();
pstore_ftrace_dir = debugfs_create_dir("pstore", NULL);
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index d0d9bfdad30c..bee71c7da995 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -285,7 +285,7 @@ static const struct super_operations pstore_ops = {
.show_options = pstore_show_options,
};
-static struct dentry *psinfo_lock_root(void)
+static struct dentry *psinfo_lock_root(struct pstore_info *psinfo)
{
struct dentry *root;
@@ -309,7 +309,7 @@ int pstore_put_backend_records(struct pstore_info *psi)
struct dentry *root;
int rc = 0;
- root = psinfo_lock_root();
+ root = psinfo_lock_root(psi);
if (!root)
return 0;
@@ -398,21 +398,22 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
* when we are re-scanning the backing store looking to add new
* error records.
*/
-void pstore_get_records(int quiet)
+void pstore_get_records(struct pstore_info *psi, int quiet)
{
struct dentry *root;
- root = psinfo_lock_root();
+ root = psinfo_lock_root(psi);
if (!root)
return;
- pstore_get_backend_records(psinfo, root, quiet);
+ pstore_get_backend_records(psi, root, quiet);
inode_unlock(d_inode(root));
}
static int pstore_fill_super(struct super_block *sb, void *data, int silent)
{
struct inode *inode;
+ struct pstore_info_list *entry;
sb->s_maxbytes = MAX_LFS_FILESIZE;
sb->s_blocksize = PAGE_SIZE;
@@ -437,7 +438,13 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent)
scoped_guard(mutex, &pstore_sb_lock)
pstore_sb = sb;
- pstore_get_records(0);
+ if (!psback)
+ return 0;
+
+ mutex_lock(&psback_lock);
+ list_for_each_entry(entry, &psback->list_entry, list)
+ pstore_get_records(entry->psi, 0);
+ mutex_unlock(&psback_lock);
return 0;
}
diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
index 801d6c0b170c..4b1c7ba27052 100644
--- a/fs/pstore/internal.h
+++ b/fs/pstore/internal.h
@@ -33,10 +33,10 @@ static inline void pstore_register_pmsg(void) {}
static inline void pstore_unregister_pmsg(void) {}
#endif
-extern struct pstore_info *psinfo;
+extern struct pstore_backends *psback;
extern void pstore_set_kmsg_bytes(int);
-extern void pstore_get_records(int);
+extern void pstore_get_records(struct pstore_info *psi, int quiet);
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);
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 03425928d2fb..432a41852a07 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -62,12 +62,12 @@ static void pstore_dowork(struct work_struct *);
static DECLARE_WORK(pstore_work, pstore_dowork);
/*
- * psinfo_lock protects "psinfo" during calls to
+ * psback_lock protects "psback" during calls to
* pstore_register(), pstore_unregister(), and
* the filesystem mount/unmount routines.
*/
-static DEFINE_MUTEX(psinfo_lock);
-struct pstore_info *psinfo;
+DEFINE_MUTEX(psback_lock);
+struct pstore_backends *psback;
static char *backend;
module_param(backend, charp, 0444);
@@ -104,7 +104,7 @@ static void *compress_workspace;
*/
#define DMESG_COMP_PERCENT 60
-static char *big_oops_buf;
+static char *big_oops_buf[PSTORE_BACKEND_NUM];
static size_t max_compressed_size;
void pstore_set_kmsg_bytes(int bytes)
@@ -201,7 +201,7 @@ static int pstore_compress(const void *in, void *out,
return zstream.total_out;
}
-static void allocate_buf_for_compression(void)
+static void allocate_buf_for_compression(struct pstore_info *psinfo, int pos)
{
size_t compressed_size;
char *buf;
@@ -241,21 +241,21 @@ static void allocate_buf_for_compression(void)
}
/* A non-NULL big_oops_buf indicates compression is available. */
- big_oops_buf = buf;
+ big_oops_buf[pos] = buf;
max_compressed_size = compressed_size;
pr_info("Using crash dump compression: %s\n", compress);
}
-static void free_buf_for_compression(void)
+static void free_buf_for_compression(int pos)
{
if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && compress_workspace) {
vfree(compress_workspace);
compress_workspace = NULL;
}
- kvfree(big_oops_buf);
- big_oops_buf = NULL;
+ kvfree(big_oops_buf[pos]);
+ big_oops_buf[pos] = NULL;
max_compressed_size = 0;
}
@@ -274,8 +274,9 @@ void pstore_record_init(struct pstore_record *record,
* callback from kmsg_dump. Save as much as we can (up to kmsg_bytes) from the
* end of the buffer.
*/
-static void pstore_dump(struct kmsg_dumper *dumper,
- enum kmsg_dump_reason reason)
+static void pstore_do_dump(struct kmsg_dumper *dumper,
+ enum kmsg_dump_reason reason,
+ struct pstore_info *psinfo, int pos)
{
struct kmsg_dump_iter iter;
unsigned long total = 0;
@@ -315,7 +316,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
record.part = part;
record.buf = psinfo->buf;
- dst = big_oops_buf ?: psinfo->buf;
+ dst = big_oops_buf[pos] ?: psinfo->buf;
dst_size = max_compressed_size ?: psinfo->bufsize;
/* Write dump header. */
@@ -328,7 +329,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
dst_size, &dump_size))
break;
- if (big_oops_buf) {
+ if (big_oops_buf[pos]) {
zipped_len = pstore_compress(dst, psinfo->buf,
header_size + dump_size,
psinfo->bufsize);
@@ -372,6 +373,19 @@ static void pstore_dump(struct kmsg_dumper *dumper,
}
}
+static void pstore_dump(struct kmsg_dumper *dumper,
+ enum kmsg_dump_reason reason)
+{
+ struct pstore_info_list *entry;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, &psback->list_entry, list)
+ if (entry->psi->flags & PSTORE_FLAGS_DMESG)
+ pstore_do_dump(dumper, reason,
+ entry->psi, entry->index);
+ rcu_read_unlock();
+}
+
static struct kmsg_dumper pstore_dumper = {
.dump = pstore_dump,
};
@@ -390,13 +404,11 @@ static void pstore_unregister_kmsg(void)
}
#ifdef CONFIG_PSTORE_CONSOLE
-static void pstore_console_write(struct console *con, const char *s, unsigned c)
+static void pstore_console_do_write(struct console *con, const char *s,
+ unsigned c, struct pstore_info *psinfo)
{
struct pstore_record record;
- if (!c)
- return;
-
pstore_record_init(&record, psinfo);
record.type = PSTORE_TYPE_CONSOLE;
@@ -405,6 +417,21 @@ static void pstore_console_write(struct console *con, const char *s, unsigned c)
psinfo->write(&record);
}
+static void pstore_console_write(struct console *con, const char *s,
+ unsigned int c)
+{
+ struct pstore_info_list *entry;
+
+ if (!c)
+ return;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, &psback->list_entry, list)
+ if (entry->psi->flags & PSTORE_FLAGS_CONSOLE)
+ pstore_console_do_write(con, s, c, entry->psi);
+ rcu_read_unlock();
+}
+
static struct console pstore_console = {
.write = pstore_console_write,
.index = -1,
@@ -413,7 +440,7 @@ static struct console pstore_console = {
static void pstore_register_console(void)
{
/* Show which backend is going to get console writes. */
- strscpy(pstore_console.name, psinfo->name,
+ strscpy(pstore_console.name, "pstore console",
sizeof(pstore_console.name));
/*
* Always initialize flags here since prior unregister_console()
@@ -464,12 +491,15 @@ static int pstore_write_user_compat(struct pstore_record *record,
*/
int pstore_register(struct pstore_info *psi)
{
+ struct pstore_info_list *entry;
+ struct pstore_info_list *newpsi;
char *new_backend;
- if (backend && strcmp(backend, psi->name)) {
- pr_warn("backend '%s' already in use: ignoring '%s'\n",
- backend, psi->name);
- return -EBUSY;
+ /* backend has to be enabled for going on registering */
+ if (backend && !strstr(backend, psi->name) &&
+ strcmp(backend, "all")) {
+ pr_warn("backend '%s' not enabled\n", psi->name);
+ return -EINVAL;
}
/* Sanity check flags. */
@@ -486,79 +516,118 @@ int pstore_register(struct pstore_info *psi)
return -EINVAL;
}
- new_backend = kstrdup(psi->name, GFP_KERNEL);
- if (!new_backend)
- return -ENOMEM;
-
- mutex_lock(&psinfo_lock);
- if (psinfo) {
- pr_warn("backend '%s' already loaded: ignoring '%s'\n",
- psinfo->name, psi->name);
- mutex_unlock(&psinfo_lock);
- kfree(new_backend);
- return -EBUSY;
+ mutex_lock(&psback_lock);
+
+ /*
+ * If no backend specified, first come first served to
+ * maintain backward compatibility
+ */
+ if (!backend) {
+ pr_warn("no backend enabled, registering backend '%s'\n",
+ psi->name);
+ new_backend = kstrdup(psi->name, GFP_KERNEL);
+ if (!new_backend) {
+ mutex_unlock(&psback_lock);
+ return -ENOMEM;
+ }
+ }
+
+ if (psback) {
+ if (psback->flag == PSTORE_LIST_FULL) {
+ pr_warn("backend registration space is used up: "
+ "ignoring '%s'\n", psi->name);
+ mutex_unlock(&psback_lock);
+ return -EBUSY;
+ }
+ list_for_each_entry(entry, &psback->list_entry, list) {
+ if (strcmp(entry->psi->name, psi->name) == 0) {
+ pr_warn("backend '%s' already loaded\n",
+ psi->name);
+ mutex_unlock(&psback_lock);
+ return -EPERM;
+ }
+ }
+ } else {
+ psback = kzalloc(sizeof(*psback), GFP_KERNEL);
+ INIT_LIST_HEAD(&psback->list_entry);
}
if (!psi->write_user)
psi->write_user = pstore_write_user_compat;
- psinfo = psi;
- mutex_init(&psinfo->read_mutex);
- spin_lock_init(&psinfo->buf_lock);
+ newpsi = kzalloc(sizeof(*newpsi), GFP_KERNEL);
+ newpsi->psi = psi;
+ newpsi->index = ffz(psback->flag);
+ psback->flag |= (1 << newpsi->index);
+
+ mutex_init(&psi->read_mutex);
+ spin_lock_init(&psi->buf_lock);
+
+ if (psi->flags & PSTORE_FLAGS_DMESG &&
+ !psback->front_cnt[PSTORE_TYPE_DMESG])
+ allocate_buf_for_compression(psi, newpsi->index);
- if (psi->flags & PSTORE_FLAGS_DMESG)
- allocate_buf_for_compression();
+ pstore_get_records(psi, 0);
- pstore_get_records(0);
+ list_add_rcu(&newpsi->list, &psback->list_entry);
- if (psi->flags & PSTORE_FLAGS_DMESG) {
- pstore_dumper.max_reason = psinfo->max_reason;
+ if (psi->flags & PSTORE_FLAGS_DMESG &&
+ !psback->front_cnt[PSTORE_TYPE_DMESG]++) {
+ pstore_dumper.max_reason = psi->max_reason;
pstore_register_kmsg();
}
- if (psi->flags & PSTORE_FLAGS_CONSOLE)
+ if (psi->flags & PSTORE_FLAGS_CONSOLE
+ && !psback->front_cnt[PSTORE_TYPE_CONSOLE]++)
pstore_register_console();
- if (psi->flags & PSTORE_FLAGS_FTRACE)
+ if (psi->flags & PSTORE_FLAGS_FTRACE &&
+ !psback->front_cnt[PSTORE_TYPE_FTRACE]++)
pstore_register_ftrace();
- if (psi->flags & PSTORE_FLAGS_PMSG)
+ if (psi->flags & PSTORE_FLAGS_PMSG &&
+ !psback->front_cnt[PSTORE_TYPE_PMSG]++)
pstore_register_pmsg();
/* Start watching for new records, if desired. */
pstore_timer_kick();
/*
- * Update the module parameter backend, so it is visible
+ * When module parameter backend is not specified,
+ * update the module parameter backend, so it is visible
* through /sys/module/pstore/parameters/backend
*/
- backend = new_backend;
+ if (!backend)
+ backend = new_backend;
pr_info("Registered %s as persistent store backend\n", psi->name);
- mutex_unlock(&psinfo_lock);
+ mutex_unlock(&psback_lock);
return 0;
}
EXPORT_SYMBOL_GPL(pstore_register);
void pstore_unregister(struct pstore_info *psi)
{
+ struct pstore_info_list *entry;
/* 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);
+ /* Can not unregister an unenabled backend*/
+ if (WARN_ON(!strstr(backend, psi->name) && strcmp(backend, "all")))
return;
- }
+
+ mutex_lock(&psback_lock);
/* Unregister all callbacks. */
- if (psi->flags & PSTORE_FLAGS_PMSG)
+ if (psi->flags & PSTORE_FLAGS_PMSG &&
+ !--psback->front_cnt[PSTORE_TYPE_PMSG])
pstore_unregister_pmsg();
- if (psi->flags & PSTORE_FLAGS_FTRACE)
+ if (psi->flags & PSTORE_FLAGS_FTRACE &&
+ !--psback->front_cnt[PSTORE_TYPE_FTRACE])
pstore_unregister_ftrace();
- if (psi->flags & PSTORE_FLAGS_CONSOLE)
+ if (psi->flags & PSTORE_FLAGS_CONSOLE &&
+ !--psback->front_cnt[PSTORE_TYPE_CONSOLE])
pstore_unregister_console();
- if (psi->flags & PSTORE_FLAGS_DMESG)
+ if (psi->flags & PSTORE_FLAGS_DMESG &&
+ !--psback->front_cnt[PSTORE_TYPE_DMESG])
pstore_unregister_kmsg();
/* Stop timer and make sure all work has finished. */
@@ -568,19 +637,30 @@ void pstore_unregister(struct pstore_info *psi)
/* Remove all backend records from filesystem tree. */
pstore_put_backend_records(psi);
- free_buf_for_compression();
+ list_for_each_entry(entry, &psback->list_entry, list) {
+ if (entry->psi == psi) {
+ list_del_rcu(&entry->list);
+ psback->flag ^= 1 << entry->index;
+ synchronize_rcu();
+ free_buf_for_compression(entry->index);
+ kfree(entry);
+ break;
+ }
+ }
- psinfo = NULL;
- kfree(backend);
- backend = NULL;
+ if (psback->flag == PSOTRE_LIST_EMPTY) {
+ kfree(psback);
+ psback = NULL;
+ }
pr_info("Unregistered %s as persistent store backend\n", psi->name);
- mutex_unlock(&psinfo_lock);
+ mutex_unlock(&psback_lock);
}
EXPORT_SYMBOL_GPL(pstore_unregister);
static void decompress_record(struct pstore_record *record,
- struct z_stream_s *zstream)
+ struct z_stream_s *zstream,
+ struct pstore_info *psinfo)
{
int ret;
int unzipped_len;
@@ -697,7 +777,7 @@ void pstore_get_backend_records(struct pstore_info *psi,
break;
}
- decompress_record(record, &zstream);
+ decompress_record(record, &zstream, psi);
rc = pstore_mkfile(root, record);
if (rc) {
/* pstore_mkfile() did not take record, so free it. */
@@ -729,7 +809,12 @@ void pstore_get_backend_records(struct pstore_info *psi,
static void pstore_dowork(struct work_struct *work)
{
- pstore_get_records(1);
+ struct pstore_info_list *entry;
+
+ mutex_lock(&psback_lock);
+ list_for_each_entry(entry, &psback->list_entry, list)
+ pstore_get_records(entry->psi, 1);
+ mutex_unlock(&psback_lock);
}
static void pstore_timefunc(struct timer_list *unused)
@@ -745,11 +830,15 @@ static void pstore_timefunc(struct timer_list *unused)
static int __init pstore_init(void)
{
int ret;
+ struct pstore_info_list *entry;
ret = pstore_init_fs();
- if (ret)
- free_buf_for_compression();
-
+ if (ret) {
+ mutex_lock(&psback_lock);
+ list_for_each_entry(entry, &psback->list_entry, list)
+ free_buf_for_compression(entry->index);
+ mutex_unlock(&psback_lock);
+ }
return ret;
}
late_initcall(pstore_init);
diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
index 55f139afa327..9d5b8602e273 100644
--- a/fs/pstore/pmsg.c
+++ b/fs/pstore/pmsg.c
@@ -11,8 +11,9 @@
static DEFINE_MUTEX(pmsg_lock);
-static ssize_t write_pmsg(struct file *file, const char __user *buf,
- size_t count, loff_t *ppos)
+static ssize_t do_write_pmsg(struct file *file, const char __user *buf,
+ size_t count, loff_t *ppos,
+ struct pstore_info *psinfo)
{
struct pstore_record record;
int ret;
@@ -34,6 +35,25 @@ static ssize_t write_pmsg(struct file *file, const char __user *buf,
return ret ? ret : count;
}
+static ssize_t write_pmsg(struct file *file, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ int ret, _ret;
+ struct pstore_info_list *entry;
+
+ mutex_lock(&psback_lock);
+ list_for_each_entry(entry, &psback->list_entry, list) {
+ if (entry->psi->flags & PSTORE_FLAGS_PMSG) {
+ _ret = do_write_pmsg(file, buf, count,
+ ppos, entry->psi);
+ ret = ret > _ret ? ret : _ret;
+ }
+ }
+ mutex_unlock(&psback_lock);
+
+ return ret;
+}
+
static const struct file_operations pmsg_fops = {
.owner = THIS_MODULE,
.llseek = noop_llseek,
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 638507a3c8ff..0d2be20c8929 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -201,6 +201,35 @@ struct pstore_info {
int (*erase)(struct pstore_record *record);
};
+/* Supported multibackends */
+#define PSTORE_MAX_BACKEND_LENGTH 100
+#define PSTORE_BACKEND_NUM 16
+
+#define PSTORE_LIST_FULL (BIT(PSTORE_BACKEND_NUM) - 1)
+#define PSOTRE_LIST_EMPTY 0
+
+extern struct mutex psback_lock;
+
+struct pstore_info_list {
+ struct pstore_info *psi;
+ struct list_head list;
+ int index;
+};
+
+/**
+ * struct pstore_backends - management of pstore backends
+ * @list_entry: entry of pstore backend driver information list
+ * @front_cnt: count of each enabled frontend
+ * @flag: bitmap of enabled pstore backend
+ *
+ */
+
+struct pstore_backends {
+ struct list_head list_entry;
+ int front_cnt[PSTORE_TYPE_MAX];
+ u16 flag;
+};
+
/* Supported frontends */
#define PSTORE_FLAGS_DMESG BIT(0)
#define PSTORE_FLAGS_CONSOLE BIT(1)
--
2.39.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] pstore: add multi-backend support
2024-02-05 12:28 ` [PATCH 1/3] " Yuanhe Shu
@ 2024-02-06 8:05 ` kernel test robot
2024-02-06 8:41 ` Yuanhe Shu
2024-02-06 12:17 ` kernel test robot
1 sibling, 1 reply; 13+ messages in thread
From: kernel test robot @ 2024-02-06 8:05 UTC (permalink / raw)
To: Yuanhe Shu, keescook, tony.luck, gpiccoli, shuah, corbet
Cc: oe-kbuild-all, xlpang, linux-kernel, linux-doc, linux-hardening,
linux-kselftest, Yuanhe Shu, Xingrui Yi
Hi Yuanhe,
kernel test robot noticed the following build errors:
[auto build test ERROR on kees/for-next/pstore]
[also build test ERROR on kees/for-next/kspp shuah-kselftest/next shuah-kselftest/fixes linus/master v6.8-rc3 next-20240205]
[cannot apply to aegl/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Yuanhe-Shu/pstore-add-multi-backend-support/20240205-203438
base: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/pstore
patch link: https://lore.kernel.org/r/20240205122852.7069-2-xiangzao%40linux.alibaba.com
patch subject: [PATCH 1/3] pstore: add multi-backend support
config: csky-randconfig-r071-20240206 (https://download.01.org/0day-ci/archive/20240206/202402061551.EkLBF7yD-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240206/202402061551.EkLBF7yD-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402061551.EkLBF7yD-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
>> fs/pstore/ftrace.c:30:47: warning: 'struct ftrace_info' declared inside parameter list will not be visible outside of this definition or declaration
30 | struct ftrace_info *psinfo)
| ^~~~~~~~~~~
fs/pstore/ftrace.c: In function 'pstore_do_ftrace':
>> fs/pstore/ftrace.c:39:24: error: initialization of 'struct pstore_info *' from incompatible pointer type 'struct ftrace_info *' [-Werror=incompatible-pointer-types]
39 | .psi = psinfo,
| ^~~~~~
fs/pstore/ftrace.c:39:24: note: (near initialization for 'record.psi')
>> fs/pstore/ftrace.c:55:15: error: invalid use of undefined type 'struct ftrace_info'
55 | psinfo->write(&record);
| ^~
fs/pstore/ftrace.c: In function 'pstore_ftrace_call':
>> fs/pstore/ftrace.c:71:61: error: passing argument 4 of 'pstore_do_ftrace' from incompatible pointer type [-Werror=incompatible-pointer-types]
71 | pstore_do_ftrace(ip, parent_ip, op, regs, entry->psi);
| ^~~~
| |
| struct pt_regs *
fs/pstore/ftrace.c:29:60: note: expected 'struct ftrace_regs *' but argument is of type 'struct pt_regs *'
29 | struct ftrace_regs *fregs,
| ~~~~~~~~~~~~~~~~~~~~^~~~~
fs/pstore/ftrace.c:71:72: error: passing argument 5 of 'pstore_do_ftrace' from incompatible pointer type [-Werror=incompatible-pointer-types]
71 | pstore_do_ftrace(ip, parent_ip, op, regs, entry->psi);
| ~~~~~^~~~~
| |
| struct pstore_info *
fs/pstore/ftrace.c:30:60: note: expected 'struct ftrace_info *' but argument is of type 'struct pstore_info *'
30 | struct ftrace_info *psinfo)
| ~~~~~~~~~~~~~~~~~~~~^~~~~~
fs/pstore/ftrace.c: At top level:
>> fs/pstore/ftrace.c:76:19: error: initialization of 'void (*)(long unsigned int, long unsigned int, struct ftrace_ops *, struct ftrace_regs *)' from incompatible pointer type 'void (*)(long unsigned int, long unsigned int, struct ftrace_ops *, struct pt_regs *)' [-Werror=incompatible-pointer-types]
76 | .func = pstore_ftrace_call,
| ^~~~~~~~~~~~~~~~~~
fs/pstore/ftrace.c:76:19: note: (near initialization for 'pstore_ftrace_ops.func')
In file included from include/linux/dcache.h:8,
from include/linux/fs.h:8,
from include/linux/huge_mm.h:8,
from include/linux/mm.h:1095,
from include/linux/kallsyms.h:13,
from include/linux/ftrace.h:13,
from fs/pstore/ftrace.c:14:
fs/pstore/ftrace.c: In function 'pstore_register_ftrace':
>> fs/pstore/ftrace.c:150:33: error: 'entry' undeclared (first use in this function); did you mean 'dentry'?
150 | list_for_each_entry_rcu(entry, &psback->list_entry, list)
| ^~~~~
include/linux/rculist.h:391:14: note: in definition of macro 'list_for_each_entry_rcu'
391 | pos = list_entry_rcu((head)->next, typeof(*pos), member); \
| ^~~
fs/pstore/ftrace.c:150:33: note: each undeclared identifier is reported only once for each function it appears in
150 | list_for_each_entry_rcu(entry, &psback->list_entry, list)
| ^~~~~
include/linux/rculist.h:391:14: note: in definition of macro 'list_for_each_entry_rcu'
391 | pos = list_entry_rcu((head)->next, typeof(*pos), member); \
| ^~~
In file included from include/linux/container_of.h:5,
from include/linux/kernel.h:22,
from fs/pstore/ftrace.c:6:
include/linux/compiler_types.h:376:27: error: expression in static assertion is not an integer
376 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:78:56: note: in definition of macro '__static_assert'
78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
| ^~~~
include/linux/container_of.h:20:9: note: in expansion of macro 'static_assert'
20 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \
| ^~~~~~~~~~~~~
include/linux/container_of.h:20:23: note: in expansion of macro '__same_type'
20 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \
| ^~~~~~~~~~~
include/linux/rculist.h:307:9: note: in expansion of macro 'container_of'
307 | container_of(READ_ONCE(ptr), type, member)
| ^~~~~~~~~~~~
include/linux/rculist.h:391:20: note: in expansion of macro 'list_entry_rcu'
391 | pos = list_entry_rcu((head)->next, typeof(*pos), member); \
| ^~~~~~~~~~~~~~
fs/pstore/ftrace.c:150:9: note: in expansion of macro 'list_for_each_entry_rcu'
150 | list_for_each_entry_rcu(entry, &psback->list_entry, list)
| ^~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler_types.h:376:27: error: expression in static assertion is not an integer
376 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:78:56: note: in definition of macro '__static_assert'
78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
| ^~~~
include/linux/container_of.h:20:9: note: in expansion of macro 'static_assert'
20 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \
| ^~~~~~~~~~~~~
include/linux/container_of.h:20:23: note: in expansion of macro '__same_type'
20 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \
| ^~~~~~~~~~~
include/linux/rculist.h:307:9: note: in expansion of macro 'container_of'
307 | container_of(READ_ONCE(ptr), type, member)
| ^~~~~~~~~~~~
include/linux/rculist.h:393:23: note: in expansion of macro 'list_entry_rcu'
393 | pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
| ^~~~~~~~~~~~~~
fs/pstore/ftrace.c:150:9: note: in expansion of macro 'list_for_each_entry_rcu'
150 | list_for_each_entry_rcu(entry, &psback->list_entry, list)
| ^~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +39 fs/pstore/ftrace.c
fbccdeb8d77d68 Joel Fernandes 2016-10-20 25
7b968e55e3984c Yuanhe Shu 2024-02-05 26 static void notrace pstore_do_ftrace(unsigned long ip,
ebacfd1ece3bfa Anton Vorontsov 2012-11-14 27 unsigned long parent_ip,
ebacfd1ece3bfa Anton Vorontsov 2012-11-14 28 struct ftrace_ops *op,
7b968e55e3984c Yuanhe Shu 2024-02-05 29 struct ftrace_regs *fregs,
7b968e55e3984c Yuanhe Shu 2024-02-05 @30 struct ftrace_info *psinfo)
060287b8c467bf Anton Vorontsov 2012-07-09 31 {
6cdf941871ec9c Steven Rostedt (VMware 2020-11-05 32) int bit;
65f8c95e46a182 Anton Vorontsov 2012-07-17 33 unsigned long flags;
060287b8c467bf Anton Vorontsov 2012-07-09 34 struct pstore_ftrace_record rec = {};
b10b471145f28c Kees Cook 2017-03-05 35 struct pstore_record record = {
b10b471145f28c Kees Cook 2017-03-05 36 .type = PSTORE_TYPE_FTRACE,
b10b471145f28c Kees Cook 2017-03-05 37 .buf = (char *)&rec,
b10b471145f28c Kees Cook 2017-03-05 38 .size = sizeof(rec),
b10b471145f28c Kees Cook 2017-03-05 @39 .psi = psinfo,
b10b471145f28c Kees Cook 2017-03-05 40 };
060287b8c467bf Anton Vorontsov 2012-07-09 41
060287b8c467bf Anton Vorontsov 2012-07-09 42 if (unlikely(oops_in_progress))
060287b8c467bf Anton Vorontsov 2012-07-09 43 return;
060287b8c467bf Anton Vorontsov 2012-07-09 44
773c16705058e9 Steven Rostedt (VMware 2020-11-05 45) bit = ftrace_test_recursion_trylock(ip, parent_ip);
6cdf941871ec9c Steven Rostedt (VMware 2020-11-05 46) if (bit < 0)
6cdf941871ec9c Steven Rostedt (VMware 2020-11-05 47) return;
6cdf941871ec9c Steven Rostedt (VMware 2020-11-05 48)
65f8c95e46a182 Anton Vorontsov 2012-07-17 49 local_irq_save(flags);
65f8c95e46a182 Anton Vorontsov 2012-07-17 50
060287b8c467bf Anton Vorontsov 2012-07-09 51 rec.ip = ip;
060287b8c467bf Anton Vorontsov 2012-07-09 52 rec.parent_ip = parent_ip;
fbccdeb8d77d68 Joel Fernandes 2016-10-20 53 pstore_ftrace_write_timestamp(&rec, pstore_ftrace_stamp++);
060287b8c467bf Anton Vorontsov 2012-07-09 54 pstore_ftrace_encode_cpu(&rec, raw_smp_processor_id());
4c9ec219766a21 Kees Cook 2017-03-05 @55 psinfo->write(&record);
65f8c95e46a182 Anton Vorontsov 2012-07-17 56
65f8c95e46a182 Anton Vorontsov 2012-07-17 57 local_irq_restore(flags);
6cdf941871ec9c Steven Rostedt (VMware 2020-11-05 58) ftrace_test_recursion_unlock(bit);
65f8c95e46a182 Anton Vorontsov 2012-07-17 59 }
65f8c95e46a182 Anton Vorontsov 2012-07-17 60
7b968e55e3984c Yuanhe Shu 2024-02-05 61 static void notrace pstore_ftrace_call(unsigned long ip,
7b968e55e3984c Yuanhe Shu 2024-02-05 62 unsigned long parent_ip,
7b968e55e3984c Yuanhe Shu 2024-02-05 63 struct ftrace_ops *op,
7b968e55e3984c Yuanhe Shu 2024-02-05 64 struct pt_regs *regs)
7b968e55e3984c Yuanhe Shu 2024-02-05 65 {
7b968e55e3984c Yuanhe Shu 2024-02-05 66 struct pstore_info_list *entry;
7b968e55e3984c Yuanhe Shu 2024-02-05 67
7b968e55e3984c Yuanhe Shu 2024-02-05 68 rcu_read_lock();
7b968e55e3984c Yuanhe Shu 2024-02-05 69 list_for_each_entry_rcu(entry, &psback->list_entry, list)
7b968e55e3984c Yuanhe Shu 2024-02-05 70 if (entry->psi->flags & PSTORE_FLAGS_FTRACE)
7b968e55e3984c Yuanhe Shu 2024-02-05 @71 pstore_do_ftrace(ip, parent_ip, op, regs, entry->psi);
7b968e55e3984c Yuanhe Shu 2024-02-05 72 rcu_read_unlock();
7b968e55e3984c Yuanhe Shu 2024-02-05 73 }
7b968e55e3984c Yuanhe Shu 2024-02-05 74
65f8c95e46a182 Anton Vorontsov 2012-07-17 75 static struct ftrace_ops pstore_ftrace_ops __read_mostly = {
65f8c95e46a182 Anton Vorontsov 2012-07-17 @76 .func = pstore_ftrace_call,
65f8c95e46a182 Anton Vorontsov 2012-07-17 77 };
65f8c95e46a182 Anton Vorontsov 2012-07-17 78
65f8c95e46a182 Anton Vorontsov 2012-07-17 79 static DEFINE_MUTEX(pstore_ftrace_lock);
65f8c95e46a182 Anton Vorontsov 2012-07-17 80 static bool pstore_ftrace_enabled;
65f8c95e46a182 Anton Vorontsov 2012-07-17 81
a5d05b07961a2d Uwe Kleine-König 2021-06-10 82 static int pstore_set_ftrace_enabled(bool on)
65f8c95e46a182 Anton Vorontsov 2012-07-17 83 {
65f8c95e46a182 Anton Vorontsov 2012-07-17 84 ssize_t ret;
65f8c95e46a182 Anton Vorontsov 2012-07-17 85
a5d05b07961a2d Uwe Kleine-König 2021-06-10 86 if (on == pstore_ftrace_enabled)
a5d05b07961a2d Uwe Kleine-König 2021-06-10 87 return 0;
65f8c95e46a182 Anton Vorontsov 2012-07-17 88
7a0032f50472c7 Joel Fernandes 2016-11-15 89 if (on) {
7a0032f50472c7 Joel Fernandes 2016-11-15 90 ftrace_ops_set_global_filter(&pstore_ftrace_ops);
65f8c95e46a182 Anton Vorontsov 2012-07-17 91 ret = register_ftrace_function(&pstore_ftrace_ops);
7a0032f50472c7 Joel Fernandes 2016-11-15 92 } else {
65f8c95e46a182 Anton Vorontsov 2012-07-17 93 ret = unregister_ftrace_function(&pstore_ftrace_ops);
7a0032f50472c7 Joel Fernandes 2016-11-15 94 }
7a0032f50472c7 Joel Fernandes 2016-11-15 95
65f8c95e46a182 Anton Vorontsov 2012-07-17 96 if (ret) {
65f8c95e46a182 Anton Vorontsov 2012-07-17 97 pr_err("%s: unable to %sregister ftrace ops: %zd\n",
65f8c95e46a182 Anton Vorontsov 2012-07-17 98 __func__, on ? "" : "un", ret);
a5d05b07961a2d Uwe Kleine-König 2021-06-10 99 } else {
a5d05b07961a2d Uwe Kleine-König 2021-06-10 100 pstore_ftrace_enabled = on;
65f8c95e46a182 Anton Vorontsov 2012-07-17 101 }
65f8c95e46a182 Anton Vorontsov 2012-07-17 102
a5d05b07961a2d Uwe Kleine-König 2021-06-10 103 return ret;
a5d05b07961a2d Uwe Kleine-König 2021-06-10 104 }
a5d05b07961a2d Uwe Kleine-König 2021-06-10 105
a5d05b07961a2d Uwe Kleine-König 2021-06-10 106 static ssize_t pstore_ftrace_knob_write(struct file *f, const char __user *buf,
a5d05b07961a2d Uwe Kleine-König 2021-06-10 107 size_t count, loff_t *ppos)
a5d05b07961a2d Uwe Kleine-König 2021-06-10 108 {
a5d05b07961a2d Uwe Kleine-König 2021-06-10 109 u8 on;
a5d05b07961a2d Uwe Kleine-König 2021-06-10 110 ssize_t ret;
a5d05b07961a2d Uwe Kleine-König 2021-06-10 111
a5d05b07961a2d Uwe Kleine-König 2021-06-10 112 ret = kstrtou8_from_user(buf, count, 2, &on);
a5d05b07961a2d Uwe Kleine-König 2021-06-10 113 if (ret)
a5d05b07961a2d Uwe Kleine-König 2021-06-10 114 return ret;
a5d05b07961a2d Uwe Kleine-König 2021-06-10 115
a5d05b07961a2d Uwe Kleine-König 2021-06-10 116 mutex_lock(&pstore_ftrace_lock);
a5d05b07961a2d Uwe Kleine-König 2021-06-10 117 ret = pstore_set_ftrace_enabled(on);
65f8c95e46a182 Anton Vorontsov 2012-07-17 118 mutex_unlock(&pstore_ftrace_lock);
65f8c95e46a182 Anton Vorontsov 2012-07-17 119
a5d05b07961a2d Uwe Kleine-König 2021-06-10 120 if (ret == 0)
a5d05b07961a2d Uwe Kleine-König 2021-06-10 121 ret = count;
a5d05b07961a2d Uwe Kleine-König 2021-06-10 122
65f8c95e46a182 Anton Vorontsov 2012-07-17 123 return ret;
65f8c95e46a182 Anton Vorontsov 2012-07-17 124 }
65f8c95e46a182 Anton Vorontsov 2012-07-17 125
65f8c95e46a182 Anton Vorontsov 2012-07-17 126 static ssize_t pstore_ftrace_knob_read(struct file *f, char __user *buf,
65f8c95e46a182 Anton Vorontsov 2012-07-17 127 size_t count, loff_t *ppos)
65f8c95e46a182 Anton Vorontsov 2012-07-17 128 {
65f8c95e46a182 Anton Vorontsov 2012-07-17 129 char val[] = { '0' + pstore_ftrace_enabled, '\n' };
65f8c95e46a182 Anton Vorontsov 2012-07-17 130
65f8c95e46a182 Anton Vorontsov 2012-07-17 131 return simple_read_from_buffer(buf, count, ppos, val, sizeof(val));
65f8c95e46a182 Anton Vorontsov 2012-07-17 132 }
65f8c95e46a182 Anton Vorontsov 2012-07-17 133
65f8c95e46a182 Anton Vorontsov 2012-07-17 134 static const struct file_operations pstore_knob_fops = {
65f8c95e46a182 Anton Vorontsov 2012-07-17 135 .open = simple_open,
65f8c95e46a182 Anton Vorontsov 2012-07-17 136 .read = pstore_ftrace_knob_read,
65f8c95e46a182 Anton Vorontsov 2012-07-17 137 .write = pstore_ftrace_knob_write,
65f8c95e46a182 Anton Vorontsov 2012-07-17 138 };
65f8c95e46a182 Anton Vorontsov 2012-07-17 139
ee1d267423a1f8 Geliang Tang 2015-10-20 140 static struct dentry *pstore_ftrace_dir;
ee1d267423a1f8 Geliang Tang 2015-10-20 141
a5d05b07961a2d Uwe Kleine-König 2021-06-10 142 static bool record_ftrace;
a5d05b07961a2d Uwe Kleine-König 2021-06-10 143 module_param(record_ftrace, bool, 0400);
a5d05b07961a2d Uwe Kleine-König 2021-06-10 144 MODULE_PARM_DESC(record_ftrace,
a5d05b07961a2d Uwe Kleine-König 2021-06-10 145 "enable ftrace recording immediately (default: off)");
a5d05b07961a2d Uwe Kleine-König 2021-06-10 146
65f8c95e46a182 Anton Vorontsov 2012-07-17 147 void pstore_register_ftrace(void)
65f8c95e46a182 Anton Vorontsov 2012-07-17 148 {
7b968e55e3984c Yuanhe Shu 2024-02-05 149 rcu_read_lock();
7b968e55e3984c Yuanhe Shu 2024-02-05 @150 list_for_each_entry_rcu(entry, &psback->list_entry, list)
7b968e55e3984c Yuanhe Shu 2024-02-05 151 if (entry->psi->flags & PSTORE_FLAGS_FTRACE)
7b968e55e3984c Yuanhe Shu 2024-02-05 152 if (!entry->psi->write) {
7b968e55e3984c Yuanhe Shu 2024-02-05 153 rcu_read_unlock();
65f8c95e46a182 Anton Vorontsov 2012-07-17 154 return;
7b968e55e3984c Yuanhe Shu 2024-02-05 155 }
7b968e55e3984c Yuanhe Shu 2024-02-05 156 rcu_read_unlock();
65f8c95e46a182 Anton Vorontsov 2012-07-17 157
ee1d267423a1f8 Geliang Tang 2015-10-20 158 pstore_ftrace_dir = debugfs_create_dir("pstore", NULL);
65f8c95e46a182 Anton Vorontsov 2012-07-17 159
a5d05b07961a2d Uwe Kleine-König 2021-06-10 160 pstore_set_ftrace_enabled(record_ftrace);
a5d05b07961a2d Uwe Kleine-König 2021-06-10 161
fa1af7583e8012 Greg Kroah-Hartman 2019-06-12 162 debugfs_create_file("record_ftrace", 0600, pstore_ftrace_dir, NULL,
fa1af7583e8012 Greg Kroah-Hartman 2019-06-12 163 &pstore_knob_fops);
ee1d267423a1f8 Geliang Tang 2015-10-20 164 }
ee1d267423a1f8 Geliang Tang 2015-10-20 165
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] pstore: add multi-backend support
2024-02-06 8:05 ` kernel test robot
@ 2024-02-06 8:41 ` Yuanhe Shu
0 siblings, 0 replies; 13+ messages in thread
From: Yuanhe Shu @ 2024-02-06 8:41 UTC (permalink / raw)
To: lkp
Cc: corbet, gpiccoli, keescook, linux-doc, linux-hardening,
linux-kernel, linux-kselftest, oe-kbuild-all, shuah, tony.luck,
xiangzao, xlpang, yixingrui
Currently, pstore supports only one backend open at a time.
Specifically, due to the global variable "psinfo", pstore only accepts
the first registered backend. If a new backend wants to register later,
pstore will simply reject it and return an error. This design forced us
to close existing backend in order to use the new ones.
To enable pstore to support multiple backends, "psinfo" is replaced by
"psinfo_list", a list that holds multiple "psinfo". If multiple backends
are registered with the same frontend, the frontend is reused.
User can specify multiple backends that are allowed to be registered by
module parameter "pstore.backend=" separated by commas or "all" to
enable all available backends. If no pstore.backend was specified,
pstore would accept the first registered backend which is the same as
before.
Signed-off-by: Xingrui Yi <yixingrui@linux.alibaba.com>
Signed-off-by: Yuanhe Shu <xiangzao@linux.alibaba.com>
---
fs/pstore/ftrace.c | 31 +++++-
fs/pstore/inode.c | 19 ++--
fs/pstore/internal.h | 4 +-
fs/pstore/platform.c | 225 ++++++++++++++++++++++++++++-------------
fs/pstore/pmsg.c | 24 ++++-
include/linux/pstore.h | 29 ++++++
6 files changed, 250 insertions(+), 82 deletions(-)
diff --git a/fs/pstore/ftrace.c b/fs/pstore/ftrace.c
index 776cae20af4e..1238d3946ca1 100644
--- a/fs/pstore/ftrace.c
+++ b/fs/pstore/ftrace.c
@@ -23,10 +23,11 @@
/* This doesn't need to be atomic: speed is chosen over correctness here. */
static u64 pstore_ftrace_stamp;
-static void notrace pstore_ftrace_call(unsigned long ip,
+static void notrace pstore_do_ftrace(unsigned long ip,
unsigned long parent_ip,
struct ftrace_ops *op,
- struct ftrace_regs *fregs)
+ struct ftrace_regs *fregs,
+ struct pstore_info *psinfo)
{
int bit;
unsigned long flags;
@@ -57,6 +58,20 @@ static void notrace pstore_ftrace_call(unsigned long ip,
ftrace_test_recursion_unlock(bit);
}
+static void notrace pstore_ftrace_call(unsigned long ip,
+ unsigned long parent_ip,
+ struct ftrace_ops *op,
+ struct ftrace_regs *regs)
+{
+ struct pstore_info_list *entry;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, &psback->list_entry, list)
+ if (entry->psi->flags & PSTORE_FLAGS_FTRACE)
+ pstore_do_ftrace(ip, parent_ip, op, regs, entry->psi);
+ rcu_read_unlock();
+}
+
static struct ftrace_ops pstore_ftrace_ops __read_mostly = {
.func = pstore_ftrace_call,
};
@@ -131,8 +146,16 @@ MODULE_PARM_DESC(record_ftrace,
void pstore_register_ftrace(void)
{
- if (!psinfo->write)
- return;
+ struct pstore_info_list *entry;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, &psback->list_entry, list)
+ if (entry->psi->flags & PSTORE_FLAGS_FTRACE)
+ if (!entry->psi->write) {
+ rcu_read_unlock();
+ return;
+ }
+ rcu_read_unlock();
pstore_ftrace_dir = debugfs_create_dir("pstore", NULL);
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index d0d9bfdad30c..bee71c7da995 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -285,7 +285,7 @@ static const struct super_operations pstore_ops = {
.show_options = pstore_show_options,
};
-static struct dentry *psinfo_lock_root(void)
+static struct dentry *psinfo_lock_root(struct pstore_info *psinfo)
{
struct dentry *root;
@@ -309,7 +309,7 @@ int pstore_put_backend_records(struct pstore_info *psi)
struct dentry *root;
int rc = 0;
- root = psinfo_lock_root();
+ root = psinfo_lock_root(psi);
if (!root)
return 0;
@@ -398,21 +398,22 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
* when we are re-scanning the backing store looking to add new
* error records.
*/
-void pstore_get_records(int quiet)
+void pstore_get_records(struct pstore_info *psi, int quiet)
{
struct dentry *root;
- root = psinfo_lock_root();
+ root = psinfo_lock_root(psi);
if (!root)
return;
- pstore_get_backend_records(psinfo, root, quiet);
+ pstore_get_backend_records(psi, root, quiet);
inode_unlock(d_inode(root));
}
static int pstore_fill_super(struct super_block *sb, void *data, int silent)
{
struct inode *inode;
+ struct pstore_info_list *entry;
sb->s_maxbytes = MAX_LFS_FILESIZE;
sb->s_blocksize = PAGE_SIZE;
@@ -437,7 +438,13 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent)
scoped_guard(mutex, &pstore_sb_lock)
pstore_sb = sb;
- pstore_get_records(0);
+ if (!psback)
+ return 0;
+
+ mutex_lock(&psback_lock);
+ list_for_each_entry(entry, &psback->list_entry, list)
+ pstore_get_records(entry->psi, 0);
+ mutex_unlock(&psback_lock);
return 0;
}
diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
index 801d6c0b170c..4b1c7ba27052 100644
--- a/fs/pstore/internal.h
+++ b/fs/pstore/internal.h
@@ -33,10 +33,10 @@ static inline void pstore_register_pmsg(void) {}
static inline void pstore_unregister_pmsg(void) {}
#endif
-extern struct pstore_info *psinfo;
+extern struct pstore_backends *psback;
extern void pstore_set_kmsg_bytes(int);
-extern void pstore_get_records(int);
+extern void pstore_get_records(struct pstore_info *psi, int quiet);
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);
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 03425928d2fb..432a41852a07 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -62,12 +62,12 @@ static void pstore_dowork(struct work_struct *);
static DECLARE_WORK(pstore_work, pstore_dowork);
/*
- * psinfo_lock protects "psinfo" during calls to
+ * psback_lock protects "psback" during calls to
* pstore_register(), pstore_unregister(), and
* the filesystem mount/unmount routines.
*/
-static DEFINE_MUTEX(psinfo_lock);
-struct pstore_info *psinfo;
+DEFINE_MUTEX(psback_lock);
+struct pstore_backends *psback;
static char *backend;
module_param(backend, charp, 0444);
@@ -104,7 +104,7 @@ static void *compress_workspace;
*/
#define DMESG_COMP_PERCENT 60
-static char *big_oops_buf;
+static char *big_oops_buf[PSTORE_BACKEND_NUM];
static size_t max_compressed_size;
void pstore_set_kmsg_bytes(int bytes)
@@ -201,7 +201,7 @@ static int pstore_compress(const void *in, void *out,
return zstream.total_out;
}
-static void allocate_buf_for_compression(void)
+static void allocate_buf_for_compression(struct pstore_info *psinfo, int pos)
{
size_t compressed_size;
char *buf;
@@ -241,21 +241,21 @@ static void allocate_buf_for_compression(void)
}
/* A non-NULL big_oops_buf indicates compression is available. */
- big_oops_buf = buf;
+ big_oops_buf[pos] = buf;
max_compressed_size = compressed_size;
pr_info("Using crash dump compression: %s\n", compress);
}
-static void free_buf_for_compression(void)
+static void free_buf_for_compression(int pos)
{
if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && compress_workspace) {
vfree(compress_workspace);
compress_workspace = NULL;
}
- kvfree(big_oops_buf);
- big_oops_buf = NULL;
+ kvfree(big_oops_buf[pos]);
+ big_oops_buf[pos] = NULL;
max_compressed_size = 0;
}
@@ -274,8 +274,9 @@ void pstore_record_init(struct pstore_record *record,
* callback from kmsg_dump. Save as much as we can (up to kmsg_bytes) from the
* end of the buffer.
*/
-static void pstore_dump(struct kmsg_dumper *dumper,
- enum kmsg_dump_reason reason)
+static void pstore_do_dump(struct kmsg_dumper *dumper,
+ enum kmsg_dump_reason reason,
+ struct pstore_info *psinfo, int pos)
{
struct kmsg_dump_iter iter;
unsigned long total = 0;
@@ -315,7 +316,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
record.part = part;
record.buf = psinfo->buf;
- dst = big_oops_buf ?: psinfo->buf;
+ dst = big_oops_buf[pos] ?: psinfo->buf;
dst_size = max_compressed_size ?: psinfo->bufsize;
/* Write dump header. */
@@ -328,7 +329,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
dst_size, &dump_size))
break;
- if (big_oops_buf) {
+ if (big_oops_buf[pos]) {
zipped_len = pstore_compress(dst, psinfo->buf,
header_size + dump_size,
psinfo->bufsize);
@@ -372,6 +373,19 @@ static void pstore_dump(struct kmsg_dumper *dumper,
}
}
+static void pstore_dump(struct kmsg_dumper *dumper,
+ enum kmsg_dump_reason reason)
+{
+ struct pstore_info_list *entry;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, &psback->list_entry, list)
+ if (entry->psi->flags & PSTORE_FLAGS_DMESG)
+ pstore_do_dump(dumper, reason,
+ entry->psi, entry->index);
+ rcu_read_unlock();
+}
+
static struct kmsg_dumper pstore_dumper = {
.dump = pstore_dump,
};
@@ -390,13 +404,11 @@ static void pstore_unregister_kmsg(void)
}
#ifdef CONFIG_PSTORE_CONSOLE
-static void pstore_console_write(struct console *con, const char *s, unsigned c)
+static void pstore_console_do_write(struct console *con, const char *s,
+ unsigned c, struct pstore_info *psinfo)
{
struct pstore_record record;
- if (!c)
- return;
-
pstore_record_init(&record, psinfo);
record.type = PSTORE_TYPE_CONSOLE;
@@ -405,6 +417,21 @@ static void pstore_console_write(struct console *con, const char *s, unsigned c)
psinfo->write(&record);
}
+static void pstore_console_write(struct console *con, const char *s,
+ unsigned int c)
+{
+ struct pstore_info_list *entry;
+
+ if (!c)
+ return;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, &psback->list_entry, list)
+ if (entry->psi->flags & PSTORE_FLAGS_CONSOLE)
+ pstore_console_do_write(con, s, c, entry->psi);
+ rcu_read_unlock();
+}
+
static struct console pstore_console = {
.write = pstore_console_write,
.index = -1,
@@ -413,7 +440,7 @@ static struct console pstore_console = {
static void pstore_register_console(void)
{
/* Show which backend is going to get console writes. */
- strscpy(pstore_console.name, psinfo->name,
+ strscpy(pstore_console.name, "pstore console",
sizeof(pstore_console.name));
/*
* Always initialize flags here since prior unregister_console()
@@ -464,12 +491,15 @@ static int pstore_write_user_compat(struct pstore_record *record,
*/
int pstore_register(struct pstore_info *psi)
{
+ struct pstore_info_list *entry;
+ struct pstore_info_list *newpsi;
char *new_backend;
- if (backend && strcmp(backend, psi->name)) {
- pr_warn("backend '%s' already in use: ignoring '%s'\n",
- backend, psi->name);
- return -EBUSY;
+ /* backend has to be enabled for going on registering */
+ if (backend && !strstr(backend, psi->name) &&
+ strcmp(backend, "all")) {
+ pr_warn("backend '%s' not enabled\n", psi->name);
+ return -EINVAL;
}
/* Sanity check flags. */
@@ -486,79 +516,118 @@ int pstore_register(struct pstore_info *psi)
return -EINVAL;
}
- new_backend = kstrdup(psi->name, GFP_KERNEL);
- if (!new_backend)
- return -ENOMEM;
-
- mutex_lock(&psinfo_lock);
- if (psinfo) {
- pr_warn("backend '%s' already loaded: ignoring '%s'\n",
- psinfo->name, psi->name);
- mutex_unlock(&psinfo_lock);
- kfree(new_backend);
- return -EBUSY;
+ mutex_lock(&psback_lock);
+
+ /*
+ * If no backend specified, first come first served to
+ * maintain backward compatibility
+ */
+ if (!backend) {
+ pr_warn("no backend enabled, registering backend '%s'\n",
+ psi->name);
+ new_backend = kstrdup(psi->name, GFP_KERNEL);
+ if (!new_backend) {
+ mutex_unlock(&psback_lock);
+ return -ENOMEM;
+ }
+ }
+
+ if (psback) {
+ if (psback->flag == PSTORE_LIST_FULL) {
+ pr_warn("backend registration space is used up: "
+ "ignoring '%s'\n", psi->name);
+ mutex_unlock(&psback_lock);
+ return -EBUSY;
+ }
+ list_for_each_entry(entry, &psback->list_entry, list) {
+ if (strcmp(entry->psi->name, psi->name) == 0) {
+ pr_warn("backend '%s' already loaded\n",
+ psi->name);
+ mutex_unlock(&psback_lock);
+ return -EPERM;
+ }
+ }
+ } else {
+ psback = kzalloc(sizeof(*psback), GFP_KERNEL);
+ INIT_LIST_HEAD(&psback->list_entry);
}
if (!psi->write_user)
psi->write_user = pstore_write_user_compat;
- psinfo = psi;
- mutex_init(&psinfo->read_mutex);
- spin_lock_init(&psinfo->buf_lock);
+ newpsi = kzalloc(sizeof(*newpsi), GFP_KERNEL);
+ newpsi->psi = psi;
+ newpsi->index = ffz(psback->flag);
+ psback->flag |= (1 << newpsi->index);
+
+ mutex_init(&psi->read_mutex);
+ spin_lock_init(&psi->buf_lock);
+
+ if (psi->flags & PSTORE_FLAGS_DMESG &&
+ !psback->front_cnt[PSTORE_TYPE_DMESG])
+ allocate_buf_for_compression(psi, newpsi->index);
- if (psi->flags & PSTORE_FLAGS_DMESG)
- allocate_buf_for_compression();
+ pstore_get_records(psi, 0);
- pstore_get_records(0);
+ list_add_rcu(&newpsi->list, &psback->list_entry);
- if (psi->flags & PSTORE_FLAGS_DMESG) {
- pstore_dumper.max_reason = psinfo->max_reason;
+ if (psi->flags & PSTORE_FLAGS_DMESG &&
+ !psback->front_cnt[PSTORE_TYPE_DMESG]++) {
+ pstore_dumper.max_reason = psi->max_reason;
pstore_register_kmsg();
}
- if (psi->flags & PSTORE_FLAGS_CONSOLE)
+ if (psi->flags & PSTORE_FLAGS_CONSOLE
+ && !psback->front_cnt[PSTORE_TYPE_CONSOLE]++)
pstore_register_console();
- if (psi->flags & PSTORE_FLAGS_FTRACE)
+ if (psi->flags & PSTORE_FLAGS_FTRACE &&
+ !psback->front_cnt[PSTORE_TYPE_FTRACE]++)
pstore_register_ftrace();
- if (psi->flags & PSTORE_FLAGS_PMSG)
+ if (psi->flags & PSTORE_FLAGS_PMSG &&
+ !psback->front_cnt[PSTORE_TYPE_PMSG]++)
pstore_register_pmsg();
/* Start watching for new records, if desired. */
pstore_timer_kick();
/*
- * Update the module parameter backend, so it is visible
+ * When module parameter backend is not specified,
+ * update the module parameter backend, so it is visible
* through /sys/module/pstore/parameters/backend
*/
- backend = new_backend;
+ if (!backend)
+ backend = new_backend;
pr_info("Registered %s as persistent store backend\n", psi->name);
- mutex_unlock(&psinfo_lock);
+ mutex_unlock(&psback_lock);
return 0;
}
EXPORT_SYMBOL_GPL(pstore_register);
void pstore_unregister(struct pstore_info *psi)
{
+ struct pstore_info_list *entry;
/* 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);
+ /* Can not unregister an unenabled backend*/
+ if (WARN_ON(!strstr(backend, psi->name) && strcmp(backend, "all")))
return;
- }
+
+ mutex_lock(&psback_lock);
/* Unregister all callbacks. */
- if (psi->flags & PSTORE_FLAGS_PMSG)
+ if (psi->flags & PSTORE_FLAGS_PMSG &&
+ !--psback->front_cnt[PSTORE_TYPE_PMSG])
pstore_unregister_pmsg();
- if (psi->flags & PSTORE_FLAGS_FTRACE)
+ if (psi->flags & PSTORE_FLAGS_FTRACE &&
+ !--psback->front_cnt[PSTORE_TYPE_FTRACE])
pstore_unregister_ftrace();
- if (psi->flags & PSTORE_FLAGS_CONSOLE)
+ if (psi->flags & PSTORE_FLAGS_CONSOLE &&
+ !--psback->front_cnt[PSTORE_TYPE_CONSOLE])
pstore_unregister_console();
- if (psi->flags & PSTORE_FLAGS_DMESG)
+ if (psi->flags & PSTORE_FLAGS_DMESG &&
+ !--psback->front_cnt[PSTORE_TYPE_DMESG])
pstore_unregister_kmsg();
/* Stop timer and make sure all work has finished. */
@@ -568,19 +637,30 @@ void pstore_unregister(struct pstore_info *psi)
/* Remove all backend records from filesystem tree. */
pstore_put_backend_records(psi);
- free_buf_for_compression();
+ list_for_each_entry(entry, &psback->list_entry, list) {
+ if (entry->psi == psi) {
+ list_del_rcu(&entry->list);
+ psback->flag ^= 1 << entry->index;
+ synchronize_rcu();
+ free_buf_for_compression(entry->index);
+ kfree(entry);
+ break;
+ }
+ }
- psinfo = NULL;
- kfree(backend);
- backend = NULL;
+ if (psback->flag == PSOTRE_LIST_EMPTY) {
+ kfree(psback);
+ psback = NULL;
+ }
pr_info("Unregistered %s as persistent store backend\n", psi->name);
- mutex_unlock(&psinfo_lock);
+ mutex_unlock(&psback_lock);
}
EXPORT_SYMBOL_GPL(pstore_unregister);
static void decompress_record(struct pstore_record *record,
- struct z_stream_s *zstream)
+ struct z_stream_s *zstream,
+ struct pstore_info *psinfo)
{
int ret;
int unzipped_len;
@@ -697,7 +777,7 @@ void pstore_get_backend_records(struct pstore_info *psi,
break;
}
- decompress_record(record, &zstream);
+ decompress_record(record, &zstream, psi);
rc = pstore_mkfile(root, record);
if (rc) {
/* pstore_mkfile() did not take record, so free it. */
@@ -729,7 +809,12 @@ void pstore_get_backend_records(struct pstore_info *psi,
static void pstore_dowork(struct work_struct *work)
{
- pstore_get_records(1);
+ struct pstore_info_list *entry;
+
+ mutex_lock(&psback_lock);
+ list_for_each_entry(entry, &psback->list_entry, list)
+ pstore_get_records(entry->psi, 1);
+ mutex_unlock(&psback_lock);
}
static void pstore_timefunc(struct timer_list *unused)
@@ -745,11 +830,15 @@ static void pstore_timefunc(struct timer_list *unused)
static int __init pstore_init(void)
{
int ret;
+ struct pstore_info_list *entry;
ret = pstore_init_fs();
- if (ret)
- free_buf_for_compression();
-
+ if (ret) {
+ mutex_lock(&psback_lock);
+ list_for_each_entry(entry, &psback->list_entry, list)
+ free_buf_for_compression(entry->index);
+ mutex_unlock(&psback_lock);
+ }
return ret;
}
late_initcall(pstore_init);
diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
index 55f139afa327..9d5b8602e273 100644
--- a/fs/pstore/pmsg.c
+++ b/fs/pstore/pmsg.c
@@ -11,8 +11,9 @@
static DEFINE_MUTEX(pmsg_lock);
-static ssize_t write_pmsg(struct file *file, const char __user *buf,
- size_t count, loff_t *ppos)
+static ssize_t do_write_pmsg(struct file *file, const char __user *buf,
+ size_t count, loff_t *ppos,
+ struct pstore_info *psinfo)
{
struct pstore_record record;
int ret;
@@ -34,6 +35,25 @@ static ssize_t write_pmsg(struct file *file, const char __user *buf,
return ret ? ret : count;
}
+static ssize_t write_pmsg(struct file *file, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ int ret, _ret;
+ struct pstore_info_list *entry;
+
+ mutex_lock(&psback_lock);
+ list_for_each_entry(entry, &psback->list_entry, list) {
+ if (entry->psi->flags & PSTORE_FLAGS_PMSG) {
+ _ret = do_write_pmsg(file, buf, count,
+ ppos, entry->psi);
+ ret = ret > _ret ? ret : _ret;
+ }
+ }
+ mutex_unlock(&psback_lock);
+
+ return ret;
+}
+
static const struct file_operations pmsg_fops = {
.owner = THIS_MODULE,
.llseek = noop_llseek,
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 638507a3c8ff..0d2be20c8929 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -201,6 +201,35 @@ struct pstore_info {
int (*erase)(struct pstore_record *record);
};
+/* Supported multibackends */
+#define PSTORE_MAX_BACKEND_LENGTH 100
+#define PSTORE_BACKEND_NUM 16
+
+#define PSTORE_LIST_FULL (BIT(PSTORE_BACKEND_NUM) - 1)
+#define PSOTRE_LIST_EMPTY 0
+
+extern struct mutex psback_lock;
+
+struct pstore_info_list {
+ struct pstore_info *psi;
+ struct list_head list;
+ int index;
+};
+
+/**
+ * struct pstore_backends - management of pstore backends
+ * @list_entry: entry of pstore backend driver information list
+ * @front_cnt: count of each enabled frontend
+ * @flag: bitmap of enabled pstore backend
+ *
+ */
+
+struct pstore_backends {
+ struct list_head list_entry;
+ int front_cnt[PSTORE_TYPE_MAX];
+ u16 flag;
+};
+
/* Supported frontends */
#define PSTORE_FLAGS_DMESG BIT(0)
#define PSTORE_FLAGS_CONSOLE BIT(1)
--
2.39.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] pstore: add multi-backend support
2024-02-05 12:28 ` [PATCH 1/3] " Yuanhe Shu
2024-02-06 8:05 ` kernel test robot
@ 2024-02-06 12:17 ` kernel test robot
1 sibling, 0 replies; 13+ messages in thread
From: kernel test robot @ 2024-02-06 12:17 UTC (permalink / raw)
To: Yuanhe Shu, keescook, tony.luck, gpiccoli, shuah, corbet
Cc: llvm, oe-kbuild-all, xlpang, linux-kernel, linux-doc,
linux-hardening, linux-kselftest, Yuanhe Shu, Xingrui Yi
Hi Yuanhe,
kernel test robot noticed the following build errors:
[auto build test ERROR on kees/for-next/pstore]
[also build test ERROR on kees/for-next/kspp shuah-kselftest/next shuah-kselftest/fixes linus/master v6.8-rc3 next-20240206]
[cannot apply to aegl/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Yuanhe-Shu/pstore-add-multi-backend-support/20240205-203438
base: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/pstore
patch link: https://lore.kernel.org/r/20240205122852.7069-2-xiangzao%40linux.alibaba.com
patch subject: [PATCH 1/3] pstore: add multi-backend support
config: i386-buildonly-randconfig-004-20240206 (https://download.01.org/0day-ci/archive/20240206/202402062036.5FPFKsGI-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240206/202402062036.5FPFKsGI-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402062036.5FPFKsGI-lkp@intel.com/
All errors (new ones prefixed by >>):
fs/pstore/ftrace.c:30:19: warning: declaration of 'struct ftrace_info' will not be visible outside of this function [-Wvisibility]
30 | struct ftrace_info *psinfo)
| ^
>> fs/pstore/ftrace.c:39:10: error: incompatible pointer types initializing 'struct pstore_info *' with an expression of type 'struct ftrace_info *' [-Werror,-Wincompatible-pointer-types]
39 | .psi = psinfo,
| ^~~~~~
>> fs/pstore/ftrace.c:55:8: error: incomplete definition of type 'struct ftrace_info'
55 | psinfo->write(&record);
| ~~~~~~^
fs/pstore/ftrace.c:30:19: note: forward declaration of 'struct ftrace_info'
30 | struct ftrace_info *psinfo)
| ^
>> fs/pstore/ftrace.c:71:40: error: incompatible pointer types passing 'struct pt_regs *' to parameter of type 'struct ftrace_regs *' [-Werror,-Wincompatible-pointer-types]
71 | pstore_do_ftrace(ip, parent_ip, op, regs, entry->psi);
| ^~~~
fs/pstore/ftrace.c:29:32: note: passing argument to parameter 'fregs' here
29 | struct ftrace_regs *fregs,
| ^
>> fs/pstore/ftrace.c:71:46: error: incompatible pointer types passing 'struct pstore_info *' to parameter of type 'struct ftrace_info *' [-Werror,-Wincompatible-pointer-types]
71 | pstore_do_ftrace(ip, parent_ip, op, regs, entry->psi);
| ^~~~~~~~~~
fs/pstore/ftrace.c:30:32: note: passing argument to parameter 'psinfo' here
30 | struct ftrace_info *psinfo)
| ^
>> fs/pstore/ftrace.c:76:10: error: incompatible function pointer types initializing 'ftrace_func_t' (aka 'void (*)(unsigned long, unsigned long, struct ftrace_ops *, struct ftrace_regs *)') with an expression of type 'void (unsigned long, unsigned long, struct ftrace_ops *, struct pt_regs *)' [-Wincompatible-function-pointer-types]
76 | .func = pstore_ftrace_call,
| ^~~~~~~~~~~~~~~~~~
>> fs/pstore/ftrace.c:150:26: error: use of undeclared identifier 'entry'
150 | list_for_each_entry_rcu(entry, &psback->list_entry, list)
| ^
>> fs/pstore/ftrace.c:150:26: error: use of undeclared identifier 'entry'
>> fs/pstore/ftrace.c:150:26: error: use of undeclared identifier 'entry'
>> fs/pstore/ftrace.c:150:26: error: use of undeclared identifier 'entry'
>> fs/pstore/ftrace.c:150:26: error: use of undeclared identifier 'entry'
>> fs/pstore/ftrace.c:150:26: error: use of undeclared identifier 'entry'
>> fs/pstore/ftrace.c:150:26: error: use of undeclared identifier 'entry'
>> fs/pstore/ftrace.c:150:26: error: use of undeclared identifier 'entry'
>> fs/pstore/ftrace.c:150:26: error: use of undeclared identifier 'entry'
>> fs/pstore/ftrace.c:150:26: error: use of undeclared identifier 'entry'
>> fs/pstore/ftrace.c:150:26: error: use of undeclared identifier 'entry'
>> fs/pstore/ftrace.c:150:2: error: operand of type 'void' where arithmetic or pointer type is required
150 | list_for_each_entry_rcu(entry, &psback->list_entry, list)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/rculist.h:393:9: note: expanded from macro 'list_for_each_entry_rcu'
393 | pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/rculist.h:307:2: note: expanded from macro 'list_entry_rcu'
307 | container_of(READ_ONCE(ptr), type, member)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/container_of.h:19:25: note: expanded from macro 'container_of'
19 | void *__mptr = (void *)(ptr); \
| ^~~~~
>> fs/pstore/ftrace.c:150:26: error: use of undeclared identifier 'entry'
150 | list_for_each_entry_rcu(entry, &psback->list_entry, list)
| ^
>> fs/pstore/ftrace.c:150:26: error: use of undeclared identifier 'entry'
fatal error: too many errors emitted, stopping now [-ferror-limit=]
1 warning and 20 errors generated.
vim +39 fs/pstore/ftrace.c
fbccdeb8d77d68 Joel Fernandes 2016-10-20 25
7b968e55e3984c Yuanhe Shu 2024-02-05 26 static void notrace pstore_do_ftrace(unsigned long ip,
ebacfd1ece3bfa Anton Vorontsov 2012-11-14 27 unsigned long parent_ip,
ebacfd1ece3bfa Anton Vorontsov 2012-11-14 28 struct ftrace_ops *op,
7b968e55e3984c Yuanhe Shu 2024-02-05 29 struct ftrace_regs *fregs,
7b968e55e3984c Yuanhe Shu 2024-02-05 30 struct ftrace_info *psinfo)
060287b8c467bf Anton Vorontsov 2012-07-09 31 {
6cdf941871ec9c Steven Rostedt (VMware 2020-11-05 32) int bit;
65f8c95e46a182 Anton Vorontsov 2012-07-17 33 unsigned long flags;
060287b8c467bf Anton Vorontsov 2012-07-09 34 struct pstore_ftrace_record rec = {};
b10b471145f28c Kees Cook 2017-03-05 35 struct pstore_record record = {
b10b471145f28c Kees Cook 2017-03-05 36 .type = PSTORE_TYPE_FTRACE,
b10b471145f28c Kees Cook 2017-03-05 37 .buf = (char *)&rec,
b10b471145f28c Kees Cook 2017-03-05 38 .size = sizeof(rec),
b10b471145f28c Kees Cook 2017-03-05 @39 .psi = psinfo,
b10b471145f28c Kees Cook 2017-03-05 40 };
060287b8c467bf Anton Vorontsov 2012-07-09 41
060287b8c467bf Anton Vorontsov 2012-07-09 42 if (unlikely(oops_in_progress))
060287b8c467bf Anton Vorontsov 2012-07-09 43 return;
060287b8c467bf Anton Vorontsov 2012-07-09 44
773c16705058e9 Steven Rostedt (VMware 2020-11-05 45) bit = ftrace_test_recursion_trylock(ip, parent_ip);
6cdf941871ec9c Steven Rostedt (VMware 2020-11-05 46) if (bit < 0)
6cdf941871ec9c Steven Rostedt (VMware 2020-11-05 47) return;
6cdf941871ec9c Steven Rostedt (VMware 2020-11-05 48)
65f8c95e46a182 Anton Vorontsov 2012-07-17 49 local_irq_save(flags);
65f8c95e46a182 Anton Vorontsov 2012-07-17 50
060287b8c467bf Anton Vorontsov 2012-07-09 51 rec.ip = ip;
060287b8c467bf Anton Vorontsov 2012-07-09 52 rec.parent_ip = parent_ip;
fbccdeb8d77d68 Joel Fernandes 2016-10-20 53 pstore_ftrace_write_timestamp(&rec, pstore_ftrace_stamp++);
060287b8c467bf Anton Vorontsov 2012-07-09 54 pstore_ftrace_encode_cpu(&rec, raw_smp_processor_id());
4c9ec219766a21 Kees Cook 2017-03-05 @55 psinfo->write(&record);
65f8c95e46a182 Anton Vorontsov 2012-07-17 56
65f8c95e46a182 Anton Vorontsov 2012-07-17 57 local_irq_restore(flags);
6cdf941871ec9c Steven Rostedt (VMware 2020-11-05 58) ftrace_test_recursion_unlock(bit);
65f8c95e46a182 Anton Vorontsov 2012-07-17 59 }
65f8c95e46a182 Anton Vorontsov 2012-07-17 60
7b968e55e3984c Yuanhe Shu 2024-02-05 61 static void notrace pstore_ftrace_call(unsigned long ip,
7b968e55e3984c Yuanhe Shu 2024-02-05 62 unsigned long parent_ip,
7b968e55e3984c Yuanhe Shu 2024-02-05 63 struct ftrace_ops *op,
7b968e55e3984c Yuanhe Shu 2024-02-05 64 struct pt_regs *regs)
7b968e55e3984c Yuanhe Shu 2024-02-05 65 {
7b968e55e3984c Yuanhe Shu 2024-02-05 66 struct pstore_info_list *entry;
7b968e55e3984c Yuanhe Shu 2024-02-05 67
7b968e55e3984c Yuanhe Shu 2024-02-05 68 rcu_read_lock();
7b968e55e3984c Yuanhe Shu 2024-02-05 69 list_for_each_entry_rcu(entry, &psback->list_entry, list)
7b968e55e3984c Yuanhe Shu 2024-02-05 70 if (entry->psi->flags & PSTORE_FLAGS_FTRACE)
7b968e55e3984c Yuanhe Shu 2024-02-05 @71 pstore_do_ftrace(ip, parent_ip, op, regs, entry->psi);
7b968e55e3984c Yuanhe Shu 2024-02-05 72 rcu_read_unlock();
7b968e55e3984c Yuanhe Shu 2024-02-05 73 }
7b968e55e3984c Yuanhe Shu 2024-02-05 74
65f8c95e46a182 Anton Vorontsov 2012-07-17 75 static struct ftrace_ops pstore_ftrace_ops __read_mostly = {
65f8c95e46a182 Anton Vorontsov 2012-07-17 @76 .func = pstore_ftrace_call,
65f8c95e46a182 Anton Vorontsov 2012-07-17 77 };
65f8c95e46a182 Anton Vorontsov 2012-07-17 78
65f8c95e46a182 Anton Vorontsov 2012-07-17 79 static DEFINE_MUTEX(pstore_ftrace_lock);
65f8c95e46a182 Anton Vorontsov 2012-07-17 80 static bool pstore_ftrace_enabled;
65f8c95e46a182 Anton Vorontsov 2012-07-17 81
a5d05b07961a2d Uwe Kleine-König 2021-06-10 82 static int pstore_set_ftrace_enabled(bool on)
65f8c95e46a182 Anton Vorontsov 2012-07-17 83 {
65f8c95e46a182 Anton Vorontsov 2012-07-17 84 ssize_t ret;
65f8c95e46a182 Anton Vorontsov 2012-07-17 85
a5d05b07961a2d Uwe Kleine-König 2021-06-10 86 if (on == pstore_ftrace_enabled)
a5d05b07961a2d Uwe Kleine-König 2021-06-10 87 return 0;
65f8c95e46a182 Anton Vorontsov 2012-07-17 88
7a0032f50472c7 Joel Fernandes 2016-11-15 89 if (on) {
7a0032f50472c7 Joel Fernandes 2016-11-15 90 ftrace_ops_set_global_filter(&pstore_ftrace_ops);
65f8c95e46a182 Anton Vorontsov 2012-07-17 91 ret = register_ftrace_function(&pstore_ftrace_ops);
7a0032f50472c7 Joel Fernandes 2016-11-15 92 } else {
65f8c95e46a182 Anton Vorontsov 2012-07-17 93 ret = unregister_ftrace_function(&pstore_ftrace_ops);
7a0032f50472c7 Joel Fernandes 2016-11-15 94 }
7a0032f50472c7 Joel Fernandes 2016-11-15 95
65f8c95e46a182 Anton Vorontsov 2012-07-17 96 if (ret) {
65f8c95e46a182 Anton Vorontsov 2012-07-17 97 pr_err("%s: unable to %sregister ftrace ops: %zd\n",
65f8c95e46a182 Anton Vorontsov 2012-07-17 98 __func__, on ? "" : "un", ret);
a5d05b07961a2d Uwe Kleine-König 2021-06-10 99 } else {
a5d05b07961a2d Uwe Kleine-König 2021-06-10 100 pstore_ftrace_enabled = on;
65f8c95e46a182 Anton Vorontsov 2012-07-17 101 }
65f8c95e46a182 Anton Vorontsov 2012-07-17 102
a5d05b07961a2d Uwe Kleine-König 2021-06-10 103 return ret;
a5d05b07961a2d Uwe Kleine-König 2021-06-10 104 }
a5d05b07961a2d Uwe Kleine-König 2021-06-10 105
a5d05b07961a2d Uwe Kleine-König 2021-06-10 106 static ssize_t pstore_ftrace_knob_write(struct file *f, const char __user *buf,
a5d05b07961a2d Uwe Kleine-König 2021-06-10 107 size_t count, loff_t *ppos)
a5d05b07961a2d Uwe Kleine-König 2021-06-10 108 {
a5d05b07961a2d Uwe Kleine-König 2021-06-10 109 u8 on;
a5d05b07961a2d Uwe Kleine-König 2021-06-10 110 ssize_t ret;
a5d05b07961a2d Uwe Kleine-König 2021-06-10 111
a5d05b07961a2d Uwe Kleine-König 2021-06-10 112 ret = kstrtou8_from_user(buf, count, 2, &on);
a5d05b07961a2d Uwe Kleine-König 2021-06-10 113 if (ret)
a5d05b07961a2d Uwe Kleine-König 2021-06-10 114 return ret;
a5d05b07961a2d Uwe Kleine-König 2021-06-10 115
a5d05b07961a2d Uwe Kleine-König 2021-06-10 116 mutex_lock(&pstore_ftrace_lock);
a5d05b07961a2d Uwe Kleine-König 2021-06-10 117 ret = pstore_set_ftrace_enabled(on);
65f8c95e46a182 Anton Vorontsov 2012-07-17 118 mutex_unlock(&pstore_ftrace_lock);
65f8c95e46a182 Anton Vorontsov 2012-07-17 119
a5d05b07961a2d Uwe Kleine-König 2021-06-10 120 if (ret == 0)
a5d05b07961a2d Uwe Kleine-König 2021-06-10 121 ret = count;
a5d05b07961a2d Uwe Kleine-König 2021-06-10 122
65f8c95e46a182 Anton Vorontsov 2012-07-17 123 return ret;
65f8c95e46a182 Anton Vorontsov 2012-07-17 124 }
65f8c95e46a182 Anton Vorontsov 2012-07-17 125
65f8c95e46a182 Anton Vorontsov 2012-07-17 126 static ssize_t pstore_ftrace_knob_read(struct file *f, char __user *buf,
65f8c95e46a182 Anton Vorontsov 2012-07-17 127 size_t count, loff_t *ppos)
65f8c95e46a182 Anton Vorontsov 2012-07-17 128 {
65f8c95e46a182 Anton Vorontsov 2012-07-17 129 char val[] = { '0' + pstore_ftrace_enabled, '\n' };
65f8c95e46a182 Anton Vorontsov 2012-07-17 130
65f8c95e46a182 Anton Vorontsov 2012-07-17 131 return simple_read_from_buffer(buf, count, ppos, val, sizeof(val));
65f8c95e46a182 Anton Vorontsov 2012-07-17 132 }
65f8c95e46a182 Anton Vorontsov 2012-07-17 133
65f8c95e46a182 Anton Vorontsov 2012-07-17 134 static const struct file_operations pstore_knob_fops = {
65f8c95e46a182 Anton Vorontsov 2012-07-17 135 .open = simple_open,
65f8c95e46a182 Anton Vorontsov 2012-07-17 136 .read = pstore_ftrace_knob_read,
65f8c95e46a182 Anton Vorontsov 2012-07-17 137 .write = pstore_ftrace_knob_write,
65f8c95e46a182 Anton Vorontsov 2012-07-17 138 };
65f8c95e46a182 Anton Vorontsov 2012-07-17 139
ee1d267423a1f8 Geliang Tang 2015-10-20 140 static struct dentry *pstore_ftrace_dir;
ee1d267423a1f8 Geliang Tang 2015-10-20 141
a5d05b07961a2d Uwe Kleine-König 2021-06-10 142 static bool record_ftrace;
a5d05b07961a2d Uwe Kleine-König 2021-06-10 143 module_param(record_ftrace, bool, 0400);
a5d05b07961a2d Uwe Kleine-König 2021-06-10 144 MODULE_PARM_DESC(record_ftrace,
a5d05b07961a2d Uwe Kleine-König 2021-06-10 145 "enable ftrace recording immediately (default: off)");
a5d05b07961a2d Uwe Kleine-König 2021-06-10 146
65f8c95e46a182 Anton Vorontsov 2012-07-17 147 void pstore_register_ftrace(void)
65f8c95e46a182 Anton Vorontsov 2012-07-17 148 {
7b968e55e3984c Yuanhe Shu 2024-02-05 149 rcu_read_lock();
7b968e55e3984c Yuanhe Shu 2024-02-05 @150 list_for_each_entry_rcu(entry, &psback->list_entry, list)
7b968e55e3984c Yuanhe Shu 2024-02-05 151 if (entry->psi->flags & PSTORE_FLAGS_FTRACE)
7b968e55e3984c Yuanhe Shu 2024-02-05 152 if (!entry->psi->write) {
7b968e55e3984c Yuanhe Shu 2024-02-05 153 rcu_read_unlock();
65f8c95e46a182 Anton Vorontsov 2012-07-17 154 return;
7b968e55e3984c Yuanhe Shu 2024-02-05 155 }
7b968e55e3984c Yuanhe Shu 2024-02-05 156 rcu_read_unlock();
65f8c95e46a182 Anton Vorontsov 2012-07-17 157
ee1d267423a1f8 Geliang Tang 2015-10-20 158 pstore_ftrace_dir = debugfs_create_dir("pstore", NULL);
65f8c95e46a182 Anton Vorontsov 2012-07-17 159
a5d05b07961a2d Uwe Kleine-König 2021-06-10 160 pstore_set_ftrace_enabled(record_ftrace);
a5d05b07961a2d Uwe Kleine-König 2021-06-10 161
fa1af7583e8012 Greg Kroah-Hartman 2019-06-12 162 debugfs_create_file("record_ftrace", 0600, pstore_ftrace_dir, NULL,
fa1af7583e8012 Greg Kroah-Hartman 2019-06-12 163 &pstore_knob_fops);
ee1d267423a1f8 Geliang Tang 2015-10-20 164 }
ee1d267423a1f8 Geliang Tang 2015-10-20 165
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 0/3] pstore: add multi-backend support
@ 2024-02-07 2:19 Yuanhe Shu
2024-02-07 2:19 ` [PATCH 1/3] " Yuanhe Shu
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Yuanhe Shu @ 2024-02-07 2:19 UTC (permalink / raw)
To: keescook, tony.luck, gpiccoli, shuah, corbet
Cc: xlpang, linux-kernel, linux-doc, linux-hardening, linux-kselftest,
Yuanhe Shu
I have been steadily working but struggled to find a seamlessly
integrated way to implement tty frontend until Guilherme inspired me
that multi-backend and tty frontend are actually two separate entities.
This submission presents the 3rd iteration of my efforts, listing
notable changes form the v1:
1. pstore.backend no longer acts as "registered backend", but "backends
eligible for registration".
2. drop subdir since it will break user space
3. drop tty frontend since I haven't yet devised a satisfactory
implementation strategy
Changes from v2:
1. Fix ftrace.c build error as I did not compile with
CONFIG_PSTORE_FTRACE.
A heartfelt thank you to Kees and Guilherme for your suggestions.
I firmly believe that a tty frontend is crucial for kdump debugging,
and I am still dedicating effort to develop one. Hope in the future I
can accomplish it with deeper comprehension with tty driver :)
Yuanhe Shu (3):
pstore: add multi-backend support
Documentation: adjust pstore backend related document
tools/testing: adjust pstore backend related selftest
Documentation/ABI/testing/pstore | 8 +-
.../admin-guide/kernel-parameters.txt | 4 +-
fs/pstore/ftrace.c | 31 ++-
fs/pstore/inode.c | 19 +-
fs/pstore/internal.h | 4 +-
fs/pstore/platform.c | 225 ++++++++++++------
fs/pstore/pmsg.c | 24 +-
include/linux/pstore.h | 29 +++
tools/testing/selftests/pstore/common_tests | 8 +-
.../selftests/pstore/pstore_post_reboot_tests | 65 ++---
tools/testing/selftests/pstore/pstore_tests | 2 +-
11 files changed, 295 insertions(+), 124 deletions(-)
--
2.39.3
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] pstore: add multi-backend support
2024-02-07 2:19 [PATCH v3 0/3] pstore: add multi-backend support Yuanhe Shu
@ 2024-02-07 2:19 ` Yuanhe Shu
2024-02-07 12:48 ` Kees Cook
2024-02-07 2:19 ` [PATCH 2/3] Documentation: adjust pstore backend related document Yuanhe Shu
2024-02-07 2:19 ` [PATCH 3/3] tools/testing: adjust pstore backend related selftest Yuanhe Shu
2 siblings, 1 reply; 13+ messages in thread
From: Yuanhe Shu @ 2024-02-07 2:19 UTC (permalink / raw)
To: keescook, tony.luck, gpiccoli, shuah, corbet
Cc: xlpang, linux-kernel, linux-doc, linux-hardening, linux-kselftest,
Yuanhe Shu, Xingrui Yi
Currently, pstore supports only one backend open at a time.
Specifically, due to the global variable "psinfo", pstore only accepts
the first registered backend. If a new backend wants to register later,
pstore will simply reject it and return an error. This design forced us
to close existing backend in order to use the new ones.
To enable pstore to support multiple backends, "psinfo" is replaced by
"psinfo_list", a list that holds multiple "psinfo". If multiple backends
are registered with the same frontend, the frontend is reused.
User can specify multiple backends that are allowed to be registered by
module parameter "pstore.backend=" separated by commas or "all" to
enable all available backends. If no pstore.backend was specified,
pstore would accept the first registered backend which is the same as
before.
Signed-off-by: Xingrui Yi <yixingrui@linux.alibaba.com>
Signed-off-by: Yuanhe Shu <xiangzao@linux.alibaba.com>
---
fs/pstore/ftrace.c | 31 +++++-
fs/pstore/inode.c | 19 ++--
fs/pstore/internal.h | 4 +-
fs/pstore/platform.c | 225 ++++++++++++++++++++++++++++-------------
fs/pstore/pmsg.c | 24 ++++-
include/linux/pstore.h | 29 ++++++
6 files changed, 250 insertions(+), 82 deletions(-)
diff --git a/fs/pstore/ftrace.c b/fs/pstore/ftrace.c
index 776cae20af4e..49b9c8532dab 100644
--- a/fs/pstore/ftrace.c
+++ b/fs/pstore/ftrace.c
@@ -23,10 +23,11 @@
/* This doesn't need to be atomic: speed is chosen over correctness here. */
static u64 pstore_ftrace_stamp;
-static void notrace pstore_ftrace_call(unsigned long ip,
+static void notrace pstore_do_ftrace(unsigned long ip,
unsigned long parent_ip,
struct ftrace_ops *op,
- struct ftrace_regs *fregs)
+ struct ftrace_regs *fregs,
+ struct pstore_info *psinfo)
{
int bit;
unsigned long flags;
@@ -57,6 +58,20 @@ static void notrace pstore_ftrace_call(unsigned long ip,
ftrace_test_recursion_unlock(bit);
}
+static void notrace pstore_ftrace_call(unsigned long ip,
+ unsigned long parent_ip,
+ struct ftrace_ops *op,
+ struct ftrace_regs *fregs)
+{
+ struct pstore_info_list *entry;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, &psback->list_entry, list)
+ if (entry->psi->flags & PSTORE_FLAGS_FTRACE)
+ pstore_do_ftrace(ip, parent_ip, op, fregs, entry->psi);
+ rcu_read_unlock();
+}
+
static struct ftrace_ops pstore_ftrace_ops __read_mostly = {
.func = pstore_ftrace_call,
};
@@ -131,8 +146,16 @@ MODULE_PARM_DESC(record_ftrace,
void pstore_register_ftrace(void)
{
- if (!psinfo->write)
- return;
+ struct pstore_info_list *entry;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, &psback->list_entry, list)
+ if (entry->psi->flags & PSTORE_FLAGS_FTRACE)
+ if (!entry->psi->write) {
+ rcu_read_unlock();
+ return;
+ }
+ rcu_read_unlock();
pstore_ftrace_dir = debugfs_create_dir("pstore", NULL);
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index d0d9bfdad30c..bee71c7da995 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -285,7 +285,7 @@ static const struct super_operations pstore_ops = {
.show_options = pstore_show_options,
};
-static struct dentry *psinfo_lock_root(void)
+static struct dentry *psinfo_lock_root(struct pstore_info *psinfo)
{
struct dentry *root;
@@ -309,7 +309,7 @@ int pstore_put_backend_records(struct pstore_info *psi)
struct dentry *root;
int rc = 0;
- root = psinfo_lock_root();
+ root = psinfo_lock_root(psi);
if (!root)
return 0;
@@ -398,21 +398,22 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
* when we are re-scanning the backing store looking to add new
* error records.
*/
-void pstore_get_records(int quiet)
+void pstore_get_records(struct pstore_info *psi, int quiet)
{
struct dentry *root;
- root = psinfo_lock_root();
+ root = psinfo_lock_root(psi);
if (!root)
return;
- pstore_get_backend_records(psinfo, root, quiet);
+ pstore_get_backend_records(psi, root, quiet);
inode_unlock(d_inode(root));
}
static int pstore_fill_super(struct super_block *sb, void *data, int silent)
{
struct inode *inode;
+ struct pstore_info_list *entry;
sb->s_maxbytes = MAX_LFS_FILESIZE;
sb->s_blocksize = PAGE_SIZE;
@@ -437,7 +438,13 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent)
scoped_guard(mutex, &pstore_sb_lock)
pstore_sb = sb;
- pstore_get_records(0);
+ if (!psback)
+ return 0;
+
+ mutex_lock(&psback_lock);
+ list_for_each_entry(entry, &psback->list_entry, list)
+ pstore_get_records(entry->psi, 0);
+ mutex_unlock(&psback_lock);
return 0;
}
diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
index 801d6c0b170c..4b1c7ba27052 100644
--- a/fs/pstore/internal.h
+++ b/fs/pstore/internal.h
@@ -33,10 +33,10 @@ static inline void pstore_register_pmsg(void) {}
static inline void pstore_unregister_pmsg(void) {}
#endif
-extern struct pstore_info *psinfo;
+extern struct pstore_backends *psback;
extern void pstore_set_kmsg_bytes(int);
-extern void pstore_get_records(int);
+extern void pstore_get_records(struct pstore_info *psi, int quiet);
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);
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 03425928d2fb..432a41852a07 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -62,12 +62,12 @@ static void pstore_dowork(struct work_struct *);
static DECLARE_WORK(pstore_work, pstore_dowork);
/*
- * psinfo_lock protects "psinfo" during calls to
+ * psback_lock protects "psback" during calls to
* pstore_register(), pstore_unregister(), and
* the filesystem mount/unmount routines.
*/
-static DEFINE_MUTEX(psinfo_lock);
-struct pstore_info *psinfo;
+DEFINE_MUTEX(psback_lock);
+struct pstore_backends *psback;
static char *backend;
module_param(backend, charp, 0444);
@@ -104,7 +104,7 @@ static void *compress_workspace;
*/
#define DMESG_COMP_PERCENT 60
-static char *big_oops_buf;
+static char *big_oops_buf[PSTORE_BACKEND_NUM];
static size_t max_compressed_size;
void pstore_set_kmsg_bytes(int bytes)
@@ -201,7 +201,7 @@ static int pstore_compress(const void *in, void *out,
return zstream.total_out;
}
-static void allocate_buf_for_compression(void)
+static void allocate_buf_for_compression(struct pstore_info *psinfo, int pos)
{
size_t compressed_size;
char *buf;
@@ -241,21 +241,21 @@ static void allocate_buf_for_compression(void)
}
/* A non-NULL big_oops_buf indicates compression is available. */
- big_oops_buf = buf;
+ big_oops_buf[pos] = buf;
max_compressed_size = compressed_size;
pr_info("Using crash dump compression: %s\n", compress);
}
-static void free_buf_for_compression(void)
+static void free_buf_for_compression(int pos)
{
if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && compress_workspace) {
vfree(compress_workspace);
compress_workspace = NULL;
}
- kvfree(big_oops_buf);
- big_oops_buf = NULL;
+ kvfree(big_oops_buf[pos]);
+ big_oops_buf[pos] = NULL;
max_compressed_size = 0;
}
@@ -274,8 +274,9 @@ void pstore_record_init(struct pstore_record *record,
* callback from kmsg_dump. Save as much as we can (up to kmsg_bytes) from the
* end of the buffer.
*/
-static void pstore_dump(struct kmsg_dumper *dumper,
- enum kmsg_dump_reason reason)
+static void pstore_do_dump(struct kmsg_dumper *dumper,
+ enum kmsg_dump_reason reason,
+ struct pstore_info *psinfo, int pos)
{
struct kmsg_dump_iter iter;
unsigned long total = 0;
@@ -315,7 +316,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
record.part = part;
record.buf = psinfo->buf;
- dst = big_oops_buf ?: psinfo->buf;
+ dst = big_oops_buf[pos] ?: psinfo->buf;
dst_size = max_compressed_size ?: psinfo->bufsize;
/* Write dump header. */
@@ -328,7 +329,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
dst_size, &dump_size))
break;
- if (big_oops_buf) {
+ if (big_oops_buf[pos]) {
zipped_len = pstore_compress(dst, psinfo->buf,
header_size + dump_size,
psinfo->bufsize);
@@ -372,6 +373,19 @@ static void pstore_dump(struct kmsg_dumper *dumper,
}
}
+static void pstore_dump(struct kmsg_dumper *dumper,
+ enum kmsg_dump_reason reason)
+{
+ struct pstore_info_list *entry;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, &psback->list_entry, list)
+ if (entry->psi->flags & PSTORE_FLAGS_DMESG)
+ pstore_do_dump(dumper, reason,
+ entry->psi, entry->index);
+ rcu_read_unlock();
+}
+
static struct kmsg_dumper pstore_dumper = {
.dump = pstore_dump,
};
@@ -390,13 +404,11 @@ static void pstore_unregister_kmsg(void)
}
#ifdef CONFIG_PSTORE_CONSOLE
-static void pstore_console_write(struct console *con, const char *s, unsigned c)
+static void pstore_console_do_write(struct console *con, const char *s,
+ unsigned c, struct pstore_info *psinfo)
{
struct pstore_record record;
- if (!c)
- return;
-
pstore_record_init(&record, psinfo);
record.type = PSTORE_TYPE_CONSOLE;
@@ -405,6 +417,21 @@ static void pstore_console_write(struct console *con, const char *s, unsigned c)
psinfo->write(&record);
}
+static void pstore_console_write(struct console *con, const char *s,
+ unsigned int c)
+{
+ struct pstore_info_list *entry;
+
+ if (!c)
+ return;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, &psback->list_entry, list)
+ if (entry->psi->flags & PSTORE_FLAGS_CONSOLE)
+ pstore_console_do_write(con, s, c, entry->psi);
+ rcu_read_unlock();
+}
+
static struct console pstore_console = {
.write = pstore_console_write,
.index = -1,
@@ -413,7 +440,7 @@ static struct console pstore_console = {
static void pstore_register_console(void)
{
/* Show which backend is going to get console writes. */
- strscpy(pstore_console.name, psinfo->name,
+ strscpy(pstore_console.name, "pstore console",
sizeof(pstore_console.name));
/*
* Always initialize flags here since prior unregister_console()
@@ -464,12 +491,15 @@ static int pstore_write_user_compat(struct pstore_record *record,
*/
int pstore_register(struct pstore_info *psi)
{
+ struct pstore_info_list *entry;
+ struct pstore_info_list *newpsi;
char *new_backend;
- if (backend && strcmp(backend, psi->name)) {
- pr_warn("backend '%s' already in use: ignoring '%s'\n",
- backend, psi->name);
- return -EBUSY;
+ /* backend has to be enabled for going on registering */
+ if (backend && !strstr(backend, psi->name) &&
+ strcmp(backend, "all")) {
+ pr_warn("backend '%s' not enabled\n", psi->name);
+ return -EINVAL;
}
/* Sanity check flags. */
@@ -486,79 +516,118 @@ int pstore_register(struct pstore_info *psi)
return -EINVAL;
}
- new_backend = kstrdup(psi->name, GFP_KERNEL);
- if (!new_backend)
- return -ENOMEM;
-
- mutex_lock(&psinfo_lock);
- if (psinfo) {
- pr_warn("backend '%s' already loaded: ignoring '%s'\n",
- psinfo->name, psi->name);
- mutex_unlock(&psinfo_lock);
- kfree(new_backend);
- return -EBUSY;
+ mutex_lock(&psback_lock);
+
+ /*
+ * If no backend specified, first come first served to
+ * maintain backward compatibility
+ */
+ if (!backend) {
+ pr_warn("no backend enabled, registering backend '%s'\n",
+ psi->name);
+ new_backend = kstrdup(psi->name, GFP_KERNEL);
+ if (!new_backend) {
+ mutex_unlock(&psback_lock);
+ return -ENOMEM;
+ }
+ }
+
+ if (psback) {
+ if (psback->flag == PSTORE_LIST_FULL) {
+ pr_warn("backend registration space is used up: "
+ "ignoring '%s'\n", psi->name);
+ mutex_unlock(&psback_lock);
+ return -EBUSY;
+ }
+ list_for_each_entry(entry, &psback->list_entry, list) {
+ if (strcmp(entry->psi->name, psi->name) == 0) {
+ pr_warn("backend '%s' already loaded\n",
+ psi->name);
+ mutex_unlock(&psback_lock);
+ return -EPERM;
+ }
+ }
+ } else {
+ psback = kzalloc(sizeof(*psback), GFP_KERNEL);
+ INIT_LIST_HEAD(&psback->list_entry);
}
if (!psi->write_user)
psi->write_user = pstore_write_user_compat;
- psinfo = psi;
- mutex_init(&psinfo->read_mutex);
- spin_lock_init(&psinfo->buf_lock);
+ newpsi = kzalloc(sizeof(*newpsi), GFP_KERNEL);
+ newpsi->psi = psi;
+ newpsi->index = ffz(psback->flag);
+ psback->flag |= (1 << newpsi->index);
+
+ mutex_init(&psi->read_mutex);
+ spin_lock_init(&psi->buf_lock);
+
+ if (psi->flags & PSTORE_FLAGS_DMESG &&
+ !psback->front_cnt[PSTORE_TYPE_DMESG])
+ allocate_buf_for_compression(psi, newpsi->index);
- if (psi->flags & PSTORE_FLAGS_DMESG)
- allocate_buf_for_compression();
+ pstore_get_records(psi, 0);
- pstore_get_records(0);
+ list_add_rcu(&newpsi->list, &psback->list_entry);
- if (psi->flags & PSTORE_FLAGS_DMESG) {
- pstore_dumper.max_reason = psinfo->max_reason;
+ if (psi->flags & PSTORE_FLAGS_DMESG &&
+ !psback->front_cnt[PSTORE_TYPE_DMESG]++) {
+ pstore_dumper.max_reason = psi->max_reason;
pstore_register_kmsg();
}
- if (psi->flags & PSTORE_FLAGS_CONSOLE)
+ if (psi->flags & PSTORE_FLAGS_CONSOLE
+ && !psback->front_cnt[PSTORE_TYPE_CONSOLE]++)
pstore_register_console();
- if (psi->flags & PSTORE_FLAGS_FTRACE)
+ if (psi->flags & PSTORE_FLAGS_FTRACE &&
+ !psback->front_cnt[PSTORE_TYPE_FTRACE]++)
pstore_register_ftrace();
- if (psi->flags & PSTORE_FLAGS_PMSG)
+ if (psi->flags & PSTORE_FLAGS_PMSG &&
+ !psback->front_cnt[PSTORE_TYPE_PMSG]++)
pstore_register_pmsg();
/* Start watching for new records, if desired. */
pstore_timer_kick();
/*
- * Update the module parameter backend, so it is visible
+ * When module parameter backend is not specified,
+ * update the module parameter backend, so it is visible
* through /sys/module/pstore/parameters/backend
*/
- backend = new_backend;
+ if (!backend)
+ backend = new_backend;
pr_info("Registered %s as persistent store backend\n", psi->name);
- mutex_unlock(&psinfo_lock);
+ mutex_unlock(&psback_lock);
return 0;
}
EXPORT_SYMBOL_GPL(pstore_register);
void pstore_unregister(struct pstore_info *psi)
{
+ struct pstore_info_list *entry;
/* 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);
+ /* Can not unregister an unenabled backend*/
+ if (WARN_ON(!strstr(backend, psi->name) && strcmp(backend, "all")))
return;
- }
+
+ mutex_lock(&psback_lock);
/* Unregister all callbacks. */
- if (psi->flags & PSTORE_FLAGS_PMSG)
+ if (psi->flags & PSTORE_FLAGS_PMSG &&
+ !--psback->front_cnt[PSTORE_TYPE_PMSG])
pstore_unregister_pmsg();
- if (psi->flags & PSTORE_FLAGS_FTRACE)
+ if (psi->flags & PSTORE_FLAGS_FTRACE &&
+ !--psback->front_cnt[PSTORE_TYPE_FTRACE])
pstore_unregister_ftrace();
- if (psi->flags & PSTORE_FLAGS_CONSOLE)
+ if (psi->flags & PSTORE_FLAGS_CONSOLE &&
+ !--psback->front_cnt[PSTORE_TYPE_CONSOLE])
pstore_unregister_console();
- if (psi->flags & PSTORE_FLAGS_DMESG)
+ if (psi->flags & PSTORE_FLAGS_DMESG &&
+ !--psback->front_cnt[PSTORE_TYPE_DMESG])
pstore_unregister_kmsg();
/* Stop timer and make sure all work has finished. */
@@ -568,19 +637,30 @@ void pstore_unregister(struct pstore_info *psi)
/* Remove all backend records from filesystem tree. */
pstore_put_backend_records(psi);
- free_buf_for_compression();
+ list_for_each_entry(entry, &psback->list_entry, list) {
+ if (entry->psi == psi) {
+ list_del_rcu(&entry->list);
+ psback->flag ^= 1 << entry->index;
+ synchronize_rcu();
+ free_buf_for_compression(entry->index);
+ kfree(entry);
+ break;
+ }
+ }
- psinfo = NULL;
- kfree(backend);
- backend = NULL;
+ if (psback->flag == PSOTRE_LIST_EMPTY) {
+ kfree(psback);
+ psback = NULL;
+ }
pr_info("Unregistered %s as persistent store backend\n", psi->name);
- mutex_unlock(&psinfo_lock);
+ mutex_unlock(&psback_lock);
}
EXPORT_SYMBOL_GPL(pstore_unregister);
static void decompress_record(struct pstore_record *record,
- struct z_stream_s *zstream)
+ struct z_stream_s *zstream,
+ struct pstore_info *psinfo)
{
int ret;
int unzipped_len;
@@ -697,7 +777,7 @@ void pstore_get_backend_records(struct pstore_info *psi,
break;
}
- decompress_record(record, &zstream);
+ decompress_record(record, &zstream, psi);
rc = pstore_mkfile(root, record);
if (rc) {
/* pstore_mkfile() did not take record, so free it. */
@@ -729,7 +809,12 @@ void pstore_get_backend_records(struct pstore_info *psi,
static void pstore_dowork(struct work_struct *work)
{
- pstore_get_records(1);
+ struct pstore_info_list *entry;
+
+ mutex_lock(&psback_lock);
+ list_for_each_entry(entry, &psback->list_entry, list)
+ pstore_get_records(entry->psi, 1);
+ mutex_unlock(&psback_lock);
}
static void pstore_timefunc(struct timer_list *unused)
@@ -745,11 +830,15 @@ static void pstore_timefunc(struct timer_list *unused)
static int __init pstore_init(void)
{
int ret;
+ struct pstore_info_list *entry;
ret = pstore_init_fs();
- if (ret)
- free_buf_for_compression();
-
+ if (ret) {
+ mutex_lock(&psback_lock);
+ list_for_each_entry(entry, &psback->list_entry, list)
+ free_buf_for_compression(entry->index);
+ mutex_unlock(&psback_lock);
+ }
return ret;
}
late_initcall(pstore_init);
diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
index 55f139afa327..9d5b8602e273 100644
--- a/fs/pstore/pmsg.c
+++ b/fs/pstore/pmsg.c
@@ -11,8 +11,9 @@
static DEFINE_MUTEX(pmsg_lock);
-static ssize_t write_pmsg(struct file *file, const char __user *buf,
- size_t count, loff_t *ppos)
+static ssize_t do_write_pmsg(struct file *file, const char __user *buf,
+ size_t count, loff_t *ppos,
+ struct pstore_info *psinfo)
{
struct pstore_record record;
int ret;
@@ -34,6 +35,25 @@ static ssize_t write_pmsg(struct file *file, const char __user *buf,
return ret ? ret : count;
}
+static ssize_t write_pmsg(struct file *file, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ int ret, _ret;
+ struct pstore_info_list *entry;
+
+ mutex_lock(&psback_lock);
+ list_for_each_entry(entry, &psback->list_entry, list) {
+ if (entry->psi->flags & PSTORE_FLAGS_PMSG) {
+ _ret = do_write_pmsg(file, buf, count,
+ ppos, entry->psi);
+ ret = ret > _ret ? ret : _ret;
+ }
+ }
+ mutex_unlock(&psback_lock);
+
+ return ret;
+}
+
static const struct file_operations pmsg_fops = {
.owner = THIS_MODULE,
.llseek = noop_llseek,
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 638507a3c8ff..0d2be20c8929 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -201,6 +201,35 @@ struct pstore_info {
int (*erase)(struct pstore_record *record);
};
+/* Supported multibackends */
+#define PSTORE_MAX_BACKEND_LENGTH 100
+#define PSTORE_BACKEND_NUM 16
+
+#define PSTORE_LIST_FULL (BIT(PSTORE_BACKEND_NUM) - 1)
+#define PSOTRE_LIST_EMPTY 0
+
+extern struct mutex psback_lock;
+
+struct pstore_info_list {
+ struct pstore_info *psi;
+ struct list_head list;
+ int index;
+};
+
+/**
+ * struct pstore_backends - management of pstore backends
+ * @list_entry: entry of pstore backend driver information list
+ * @front_cnt: count of each enabled frontend
+ * @flag: bitmap of enabled pstore backend
+ *
+ */
+
+struct pstore_backends {
+ struct list_head list_entry;
+ int front_cnt[PSTORE_TYPE_MAX];
+ u16 flag;
+};
+
/* Supported frontends */
#define PSTORE_FLAGS_DMESG BIT(0)
#define PSTORE_FLAGS_CONSOLE BIT(1)
--
2.39.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] Documentation: adjust pstore backend related document
2024-02-07 2:19 [PATCH v3 0/3] pstore: add multi-backend support Yuanhe Shu
2024-02-07 2:19 ` [PATCH 1/3] " Yuanhe Shu
@ 2024-02-07 2:19 ` Yuanhe Shu
2024-02-07 12:51 ` Kees Cook
2024-02-07 2:19 ` [PATCH 3/3] tools/testing: adjust pstore backend related selftest Yuanhe Shu
2 siblings, 1 reply; 13+ messages in thread
From: Yuanhe Shu @ 2024-02-07 2:19 UTC (permalink / raw)
To: keescook, tony.luck, gpiccoli, shuah, corbet
Cc: xlpang, linux-kernel, linux-doc, linux-hardening, linux-kselftest,
Yuanhe Shu
Pstore now supports multiple backends, adjust related document.
Signed-off-by: Yuanhe Shu <xiangzao@linux.alibaba.com>
---
Documentation/ABI/testing/pstore | 8 ++++----
Documentation/admin-guide/kernel-parameters.txt | 4 +++-
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/Documentation/ABI/testing/pstore b/Documentation/ABI/testing/pstore
index d3cff4a7ee10..2cd67b502f11 100644
--- a/Documentation/ABI/testing/pstore
+++ b/Documentation/ABI/testing/pstore
@@ -41,7 +41,7 @@ Description: Generic interface to platform dependent persistent storage.
persistent storage until at least this amount is reached.
Default is 10 Kbytes.
- Pstore only supports one backend at a time. If multiple
- backends are available, the preferred backend may be
- set by passing the pstore.backend= argument to the kernel at
- boot time.
+ Pstore now supports multiple backends at a time. If multiple
+ backends are available, the registrable backends may be
+ set by passing the pstore.backend= argument1, argument2...
+ or pstore.backend= all to the kernel at boot time.
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 31b3a25680d0..a8a109b822a9 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4748,7 +4748,9 @@
[HW,MOUSE] Controls Logitech smartscroll autorepeat.
0 = disabled, 1 = enabled (default).
- pstore.backend= Specify the name of the pstore backend to use
+ pstore.backend=backend1,...,backendN
+ Specify the names of the pstore backends to use
+ or =all for all available backends
pti= [X86-64] Control Page Table Isolation of user and
kernel address spaces. Disabling this feature
--
2.39.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] tools/testing: adjust pstore backend related selftest
2024-02-07 2:19 [PATCH v3 0/3] pstore: add multi-backend support Yuanhe Shu
2024-02-07 2:19 ` [PATCH 1/3] " Yuanhe Shu
2024-02-07 2:19 ` [PATCH 2/3] Documentation: adjust pstore backend related document Yuanhe Shu
@ 2024-02-07 2:19 ` Yuanhe Shu
2024-02-07 12:53 ` Kees Cook
2 siblings, 1 reply; 13+ messages in thread
From: Yuanhe Shu @ 2024-02-07 2:19 UTC (permalink / raw)
To: keescook, tony.luck, gpiccoli, shuah, corbet
Cc: xlpang, linux-kernel, linux-doc, linux-hardening, linux-kselftest,
Yuanhe Shu
Pstore now supports multiple backends, the module parameter
pstore.backend varies from 'registered backend' to 'backends that are
allowed to register'. Adjust selftests to match the change.
Signed-off-by: Yuanhe Shu <xiangzao@linux.alibaba.com>
---
tools/testing/selftests/pstore/common_tests | 8 +--
.../selftests/pstore/pstore_post_reboot_tests | 65 ++++++++++---------
tools/testing/selftests/pstore/pstore_tests | 2 +-
3 files changed, 38 insertions(+), 37 deletions(-)
diff --git a/tools/testing/selftests/pstore/common_tests b/tools/testing/selftests/pstore/common_tests
index 4509f0cc9c91..497e6fc3215f 100755
--- a/tools/testing/selftests/pstore/common_tests
+++ b/tools/testing/selftests/pstore/common_tests
@@ -27,9 +27,9 @@ show_result() { # result_value
}
check_files_exist() { # type of pstorefs file
- if [ -e ${1}-${backend}-0 ]; then
+ if [ -e ${1}-${2}-0 ]; then
prlog "ok"
- for f in `ls ${1}-${backend}-*`; do
+ for f in `ls ${1}-${2}-*`; do
prlog -e "\t${f}"
done
else
@@ -74,9 +74,9 @@ prlog "=== Pstore unit tests (`basename $0`) ==="
prlog "UUID="$UUID
prlog -n "Checking pstore backend is registered ... "
-backend=`cat /sys/module/pstore/parameters/backend`
+backends=$(dmesg | sed -n 's/.*pstore: Registered \(.*\) as persistent store backend.*/\1/p')
show_result $?
-prlog -e "\tbackend=${backend}"
+prlog -e "\tbackends="$backends
prlog -e "\tcmdline=`cat /proc/cmdline`"
if [ $rc -ne 0 ]; then
exit 1
diff --git a/tools/testing/selftests/pstore/pstore_post_reboot_tests b/tools/testing/selftests/pstore/pstore_post_reboot_tests
index d6da5e86efbf..9e40ccb9c918 100755
--- a/tools/testing/selftests/pstore/pstore_post_reboot_tests
+++ b/tools/testing/selftests/pstore/pstore_post_reboot_tests
@@ -36,45 +36,46 @@ else
fi
cd ${mount_point}
+for backend in ${backends}; do
+ prlog -n "Checking ${backend}-dmesg files exist in pstore filesystem ... "
+ check_files_exist dmesg ${backend}
-prlog -n "Checking dmesg files exist in pstore filesystem ... "
-check_files_exist dmesg
+ prlog -n "Checking ${backend}-console files exist in pstore filesystem ... "
+ check_files_exist console ${backend}
-prlog -n "Checking console files exist in pstore filesystem ... "
-check_files_exist console
+ prlog -n "Checking ${backend}-pmsg files exist in pstore filesystem ... "
+ check_files_exist pmsg ${backend}
-prlog -n "Checking pmsg files exist in pstore filesystem ... "
-check_files_exist pmsg
+ prlog -n "Checking ${backend}-dmesg files contain oops end marker"
+ grep_end_trace() {
+ grep -q "\---\[ end trace" $1
+ }
+ files=`ls dmesg-${backend}-*`
+ operate_files $? "$files" grep_end_trace
-prlog -n "Checking dmesg files contain oops end marker"
-grep_end_trace() {
- grep -q "\---\[ end trace" $1
-}
-files=`ls dmesg-${backend}-*`
-operate_files $? "$files" grep_end_trace
+ prlog -n "Checking ${backend}-console file contains oops end marker ... "
+ grep -q "\---\[ end trace" console-${backend}-0
+ show_result $?
-prlog -n "Checking console file contains oops end marker ... "
-grep -q "\---\[ end trace" console-${backend}-0
-show_result $?
-
-prlog -n "Checking pmsg file properly keeps the content written before crash ... "
-prev_uuid=`cat $TOP_DIR/prev_uuid`
-if [ $? -eq 0 ]; then
- nr_matched=`grep -c "$TEST_STRING_PATTERN" pmsg-${backend}-0`
- if [ $nr_matched -eq 1 ]; then
- grep -q "$TEST_STRING_PATTERN"$prev_uuid pmsg-${backend}-0
- show_result $?
+ prlog -n "Checking ${backend}-pmsg file properly keeps the content written before crash ... "
+ prev_uuid=`cat $TOP_DIR/prev_uuid`
+ if [ $? -eq 0 ]; then
+ nr_matched=`grep -c "$TEST_STRING_PATTERN" pmsg-${backend}-0`
+ if [ $nr_matched -eq 1 ]; then
+ grep -q "$TEST_STRING_PATTERN"$prev_uuid pmsg-${backend}-0
+ show_result $?
+ else
+ prlog "FAIL"
+ rc=1
+ fi
else
- prlog "FAIL"
- rc=1
+ prlog "FAIL"
+ rc=1
fi
-else
- prlog "FAIL"
- rc=1
-fi
-prlog -n "Removing all files in pstore filesystem "
-files=`ls *-${backend}-*`
-operate_files $? "$files" rm
+ prlog -n "Removing all ${backend} files in pstore filesystem "
+ files=`ls *-${backend}-*`
+ operate_files $? "$files" rm
+done
exit $rc
diff --git a/tools/testing/selftests/pstore/pstore_tests b/tools/testing/selftests/pstore/pstore_tests
index 2aa9a3852a84..f4665a8c77dc 100755
--- a/tools/testing/selftests/pstore/pstore_tests
+++ b/tools/testing/selftests/pstore/pstore_tests
@@ -10,7 +10,7 @@
. ./common_tests
prlog -n "Checking pstore console is registered ... "
-dmesg | grep -Eq "console \[(pstore|${backend})"
+dmesg | grep -Eq "console \[(pstore console)"
show_result $?
prlog -n "Checking /dev/pmsg0 exists ... "
--
2.39.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] pstore: add multi-backend support
2024-02-07 2:19 ` [PATCH 1/3] " Yuanhe Shu
@ 2024-02-07 12:48 ` Kees Cook
2024-02-20 12:19 ` Yuanhe Shu
0 siblings, 1 reply; 13+ messages in thread
From: Kees Cook @ 2024-02-07 12:48 UTC (permalink / raw)
To: Yuanhe Shu
Cc: tony.luck, gpiccoli, shuah, corbet, xlpang, linux-kernel,
linux-doc, linux-hardening, linux-kselftest, Xingrui Yi
On Wed, Feb 07, 2024 at 10:19:19AM +0800, Yuanhe Shu wrote:
> Currently, pstore supports only one backend open at a time.
> Specifically, due to the global variable "psinfo", pstore only accepts
> the first registered backend. If a new backend wants to register later,
> pstore will simply reject it and return an error. This design forced us
> to close existing backend in order to use the new ones.
>
> To enable pstore to support multiple backends, "psinfo" is replaced by
> "psinfo_list", a list that holds multiple "psinfo". If multiple backends
> are registered with the same frontend, the frontend is reused.
>
> User can specify multiple backends that are allowed to be registered by
> module parameter "pstore.backend=" separated by commas or "all" to
> enable all available backends. If no pstore.backend was specified,
> pstore would accept the first registered backend which is the same as
> before.
>
> Signed-off-by: Xingrui Yi <yixingrui@linux.alibaba.com>
> Signed-off-by: Yuanhe Shu <xiangzao@linux.alibaba.com>
> ---
> fs/pstore/ftrace.c | 31 +++++-
> fs/pstore/inode.c | 19 ++--
> fs/pstore/internal.h | 4 +-
> fs/pstore/platform.c | 225 ++++++++++++++++++++++++++++-------------
> fs/pstore/pmsg.c | 24 ++++-
> include/linux/pstore.h | 29 ++++++
> 6 files changed, 250 insertions(+), 82 deletions(-)
>
> diff --git a/fs/pstore/ftrace.c b/fs/pstore/ftrace.c
> index 776cae20af4e..49b9c8532dab 100644
> --- a/fs/pstore/ftrace.c
> +++ b/fs/pstore/ftrace.c
> @@ -23,10 +23,11 @@
> /* This doesn't need to be atomic: speed is chosen over correctness here. */
> static u64 pstore_ftrace_stamp;
>
> -static void notrace pstore_ftrace_call(unsigned long ip,
> +static void notrace pstore_do_ftrace(unsigned long ip,
> unsigned long parent_ip,
> struct ftrace_ops *op,
> - struct ftrace_regs *fregs)
> + struct ftrace_regs *fregs,
> + struct pstore_info *psinfo)
> {
> int bit;
> unsigned long flags;
> @@ -57,6 +58,20 @@ static void notrace pstore_ftrace_call(unsigned long ip,
> ftrace_test_recursion_unlock(bit);
> }
>
> +static void notrace pstore_ftrace_call(unsigned long ip,
> + unsigned long parent_ip,
> + struct ftrace_ops *op,
> + struct ftrace_regs *fregs)
> +{
> + struct pstore_info_list *entry;
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(entry, &psback->list_entry, list)
> + if (entry->psi->flags & PSTORE_FLAGS_FTRACE)
> + pstore_do_ftrace(ip, parent_ip, op, fregs, entry->psi);
> + rcu_read_unlock();
Just to make sure I understand the purpose of the RCU here is to deal
with having a backend get unregistered while something is walking this
list? And to do so without holding the global psback_lock?
> +}
> +
> static struct ftrace_ops pstore_ftrace_ops __read_mostly = {
> .func = pstore_ftrace_call,
> };
> @@ -131,8 +146,16 @@ MODULE_PARM_DESC(record_ftrace,
>
> void pstore_register_ftrace(void)
> {
> - if (!psinfo->write)
> - return;
> + struct pstore_info_list *entry;
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(entry, &psback->list_entry, list)
> + if (entry->psi->flags & PSTORE_FLAGS_FTRACE)
> + if (!entry->psi->write) {
> + rcu_read_unlock();
> + return;
> + }
> + rcu_read_unlock();
>
> pstore_ftrace_dir = debugfs_create_dir("pstore", NULL);
>
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index d0d9bfdad30c..bee71c7da995 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -285,7 +285,7 @@ static const struct super_operations pstore_ops = {
> .show_options = pstore_show_options,
> };
>
> -static struct dentry *psinfo_lock_root(void)
> +static struct dentry *psinfo_lock_root(struct pstore_info *psinfo)
> {
> struct dentry *root;
>
> @@ -309,7 +309,7 @@ int pstore_put_backend_records(struct pstore_info *psi)
> struct dentry *root;
> int rc = 0;
>
> - root = psinfo_lock_root();
> + root = psinfo_lock_root(psi);
> if (!root)
> return 0;
>
> @@ -398,21 +398,22 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
> * when we are re-scanning the backing store looking to add new
> * error records.
> */
> -void pstore_get_records(int quiet)
> +void pstore_get_records(struct pstore_info *psi, int quiet)
> {
> struct dentry *root;
>
> - root = psinfo_lock_root();
> + root = psinfo_lock_root(psi);
> if (!root)
> return;
>
> - pstore_get_backend_records(psinfo, root, quiet);
> + pstore_get_backend_records(psi, root, quiet);
> inode_unlock(d_inode(root));
> }
>
> static int pstore_fill_super(struct super_block *sb, void *data, int silent)
> {
> struct inode *inode;
> + struct pstore_info_list *entry;
>
> sb->s_maxbytes = MAX_LFS_FILESIZE;
> sb->s_blocksize = PAGE_SIZE;
> @@ -437,7 +438,13 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent)
> scoped_guard(mutex, &pstore_sb_lock)
> pstore_sb = sb;
>
> - pstore_get_records(0);
> + if (!psback)
> + return 0;
> +
> + mutex_lock(&psback_lock);
> + list_for_each_entry(entry, &psback->list_entry, list)
> + pstore_get_records(entry->psi, 0);
> + mutex_unlock(&psback_lock);
>
> return 0;
> }
> diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
> index 801d6c0b170c..4b1c7ba27052 100644
> --- a/fs/pstore/internal.h
> +++ b/fs/pstore/internal.h
> @@ -33,10 +33,10 @@ static inline void pstore_register_pmsg(void) {}
> static inline void pstore_unregister_pmsg(void) {}
> #endif
>
> -extern struct pstore_info *psinfo;
> +extern struct pstore_backends *psback;
>
> extern void pstore_set_kmsg_bytes(int);
> -extern void pstore_get_records(int);
> +extern void pstore_get_records(struct pstore_info *psi, int quiet);
> 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);
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 03425928d2fb..432a41852a07 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -62,12 +62,12 @@ static void pstore_dowork(struct work_struct *);
> static DECLARE_WORK(pstore_work, pstore_dowork);
>
> /*
> - * psinfo_lock protects "psinfo" during calls to
> + * psback_lock protects "psback" during calls to
> * pstore_register(), pstore_unregister(), and
> * the filesystem mount/unmount routines.
> */
> -static DEFINE_MUTEX(psinfo_lock);
> -struct pstore_info *psinfo;
> +DEFINE_MUTEX(psback_lock);
> +struct pstore_backends *psback;
>
> static char *backend;
> module_param(backend, charp, 0444);
> @@ -104,7 +104,7 @@ static void *compress_workspace;
> */
> #define DMESG_COMP_PERCENT 60
>
> -static char *big_oops_buf;
> +static char *big_oops_buf[PSTORE_BACKEND_NUM];
I think big_oops_buf should live in the pstore_info_list struct, then
no indexes are needed to be tracked.
> static size_t max_compressed_size;
>
> void pstore_set_kmsg_bytes(int bytes)
> @@ -201,7 +201,7 @@ static int pstore_compress(const void *in, void *out,
> return zstream.total_out;
> }
>
> -static void allocate_buf_for_compression(void)
> +static void allocate_buf_for_compression(struct pstore_info *psinfo, int pos)
> {
> size_t compressed_size;
> char *buf;
> @@ -241,21 +241,21 @@ static void allocate_buf_for_compression(void)
> }
>
> /* A non-NULL big_oops_buf indicates compression is available. */
> - big_oops_buf = buf;
> + big_oops_buf[pos] = buf;
e.g.: entry->big_oops_buf = buf;
> max_compressed_size = compressed_size;
And should this be included as well, or are we just constantly setting
this to the same value?
>
> pr_info("Using crash dump compression: %s\n", compress);
> }
>
> -static void free_buf_for_compression(void)
> +static void free_buf_for_compression(int pos)
Instead of "pos" this would take the ps_info.
> {
> if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && compress_workspace) {
> vfree(compress_workspace);
> compress_workspace = NULL;
> }
>
> - kvfree(big_oops_buf);
> - big_oops_buf = NULL;
> + kvfree(big_oops_buf[pos]);
> + big_oops_buf[pos] = NULL;
> max_compressed_size = 0;
> }
>
> @@ -274,8 +274,9 @@ void pstore_record_init(struct pstore_record *record,
> * callback from kmsg_dump. Save as much as we can (up to kmsg_bytes) from the
> * end of the buffer.
> */
> -static void pstore_dump(struct kmsg_dumper *dumper,
> - enum kmsg_dump_reason reason)
> +static void pstore_do_dump(struct kmsg_dumper *dumper,
> + enum kmsg_dump_reason reason,
> + struct pstore_info *psinfo, int pos)
> {
> struct kmsg_dump_iter iter;
> unsigned long total = 0;
> @@ -315,7 +316,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> record.part = part;
> record.buf = psinfo->buf;
>
> - dst = big_oops_buf ?: psinfo->buf;
> + dst = big_oops_buf[pos] ?: psinfo->buf;
> dst_size = max_compressed_size ?: psinfo->bufsize;
>
> /* Write dump header. */
> @@ -328,7 +329,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> dst_size, &dump_size))
> break;
>
> - if (big_oops_buf) {
> + if (big_oops_buf[pos]) {
> zipped_len = pstore_compress(dst, psinfo->buf,
> header_size + dump_size,
> psinfo->bufsize);
> @@ -372,6 +373,19 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> }
> }
>
> +static void pstore_dump(struct kmsg_dumper *dumper,
> + enum kmsg_dump_reason reason)
> +{
> + struct pstore_info_list *entry;
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(entry, &psback->list_entry, list)
> + if (entry->psi->flags & PSTORE_FLAGS_DMESG)
> + pstore_do_dump(dumper, reason,
> + entry->psi, entry->index);
Then no need for index here.
> + rcu_read_unlock();
> +}
> +
> static struct kmsg_dumper pstore_dumper = {
> .dump = pstore_dump,
> };
> @@ -390,13 +404,11 @@ static void pstore_unregister_kmsg(void)
> }
>
> #ifdef CONFIG_PSTORE_CONSOLE
> -static void pstore_console_write(struct console *con, const char *s, unsigned c)
> +static void pstore_console_do_write(struct console *con, const char *s,
> + unsigned c, struct pstore_info *psinfo)
> {
> struct pstore_record record;
>
> - if (!c)
> - return;
> -
> pstore_record_init(&record, psinfo);
> record.type = PSTORE_TYPE_CONSOLE;
>
> @@ -405,6 +417,21 @@ static void pstore_console_write(struct console *con, const char *s, unsigned c)
> psinfo->write(&record);
> }
>
> +static void pstore_console_write(struct console *con, const char *s,
> + unsigned int c)
> +{
> + struct pstore_info_list *entry;
> +
> + if (!c)
> + return;
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(entry, &psback->list_entry, list)
> + if (entry->psi->flags & PSTORE_FLAGS_CONSOLE)
> + pstore_console_do_write(con, s, c, entry->psi);
> + rcu_read_unlock();
> +}
> +
> static struct console pstore_console = {
> .write = pstore_console_write,
> .index = -1,
> @@ -413,7 +440,7 @@ static struct console pstore_console = {
> static void pstore_register_console(void)
> {
> /* Show which backend is going to get console writes. */
> - strscpy(pstore_console.name, psinfo->name,
> + strscpy(pstore_console.name, "pstore console",
> sizeof(pstore_console.name));
A nice-to-have for this might be to include all the backend names
included, but that is probably overkill, so this is fine.
> /*
> * Always initialize flags here since prior unregister_console()
> @@ -464,12 +491,15 @@ static int pstore_write_user_compat(struct pstore_record *record,
> */
> int pstore_register(struct pstore_info *psi)
> {
> + struct pstore_info_list *entry;
> + struct pstore_info_list *newpsi;
> char *new_backend;
>
> - if (backend && strcmp(backend, psi->name)) {
> - pr_warn("backend '%s' already in use: ignoring '%s'\n",
> - backend, psi->name);
> - return -EBUSY;
> + /* backend has to be enabled for going on registering */
> + if (backend && !strstr(backend, psi->name) &&
I think I'd like a helper to do the "is this name in the list?", as that
would be more robust. e.g. if we ever had a "ram" backend and a "cram"
backend, and someone used "backend=cram", suddenly both "cram" and "ram"
match in the code above.
> + strcmp(backend, "all")) {
style preference, can you make this strcmp be:
strcmp(backend, "all") != 0
it seems redundant, but it helps readers remember that "strcmp() == 0"
is "matches".
> + pr_warn("backend '%s' not enabled\n", psi->name);
nit, I think a more clear error might be something like:
"backend '%s' ignored: not present in pstore.backend=...\n"
which gives people a little hint about what's wrong.
> + return -EINVAL;
> }
>
> /* Sanity check flags. */
> @@ -486,79 +516,118 @@ int pstore_register(struct pstore_info *psi)
> return -EINVAL;
> }
>
> - new_backend = kstrdup(psi->name, GFP_KERNEL);
> - if (!new_backend)
> - return -ENOMEM;
> -
> - mutex_lock(&psinfo_lock);
> - if (psinfo) {
> - pr_warn("backend '%s' already loaded: ignoring '%s'\n",
> - psinfo->name, psi->name);
> - mutex_unlock(&psinfo_lock);
> - kfree(new_backend);
> - return -EBUSY;
> + mutex_lock(&psback_lock);
> +
> + /*
> + * If no backend specified, first come first served to
> + * maintain backward compatibility
> + */
> + if (!backend) {
> + pr_warn("no backend enabled, registering backend '%s'\n",
> + psi->name);
I'd move this to after the if below, so you can't get this message if it
were going to just immediately fail. And a nit on language. I think this
would be more clear:
"pstore.backend=... not specified; registering first available: '%s'\n"
> + new_backend = kstrdup(psi->name, GFP_KERNEL);
> + if (!new_backend) {
> + mutex_unlock(&psback_lock);
> + return -ENOMEM;
> + }
> + }
> +
> + if (psback) {
> + if (psback->flag == PSTORE_LIST_FULL) {
> + pr_warn("backend registration space is used up: "
> + "ignoring '%s'\n", psi->name);
> + mutex_unlock(&psback_lock);
> + return -EBUSY;
> + }
All of this flag/index stuff can go away with big_oops_buf moved.
> + list_for_each_entry(entry, &psback->list_entry, list) {
> + if (strcmp(entry->psi->name, psi->name) == 0) {
> + pr_warn("backend '%s' already loaded\n",
> + psi->name);
"backend '%s' already loaded; not loading it again\n"
> + mutex_unlock(&psback_lock);
> + return -EPERM;
> + }
> + }
> + } else {
> + psback = kzalloc(sizeof(*psback), GFP_KERNEL);
> + INIT_LIST_HEAD(&psback->list_entry);
> }
>
> if (!psi->write_user)
> psi->write_user = pstore_write_user_compat;
> - psinfo = psi;
> - mutex_init(&psinfo->read_mutex);
> - spin_lock_init(&psinfo->buf_lock);
> + newpsi = kzalloc(sizeof(*newpsi), GFP_KERNEL);
> + newpsi->psi = psi;
> + newpsi->index = ffz(psback->flag);
> + psback->flag |= (1 << newpsi->index);
> +
> + mutex_init(&psi->read_mutex);
> + spin_lock_init(&psi->buf_lock);
> +
> + if (psi->flags & PSTORE_FLAGS_DMESG &&
> + !psback->front_cnt[PSTORE_TYPE_DMESG])
> + allocate_buf_for_compression(psi, newpsi->index);
Can't we just ignore the front_cnt? The only reason to have
big_oops_buf is if the backend supports PSTORE_FLAGS_DMESG, yes?
>
> - if (psi->flags & PSTORE_FLAGS_DMESG)
> - allocate_buf_for_compression();
> + pstore_get_records(psi, 0);
>
> - pstore_get_records(0);
> + list_add_rcu(&newpsi->list, &psback->list_entry);
>
> - if (psi->flags & PSTORE_FLAGS_DMESG) {
> - pstore_dumper.max_reason = psinfo->max_reason;
> + if (psi->flags & PSTORE_FLAGS_DMESG &&
> + !psback->front_cnt[PSTORE_TYPE_DMESG]++) {
> + pstore_dumper.max_reason = psi->max_reason;
> pstore_register_kmsg();
> }
Hmm... does this mean we need multiple pstore_dump instances? (i.e. like
big_oops_buf) The backend configures the max_reason, so we'll need to
register multiple kmsg handlers to have different max_reasons. Right now
this just means "last registered backend wins" which would be
surprising.
Let's add a pstore_dumper instance to pstore_info_list, along with
big_oops_buf. All of these globals are really per-backend instances now.
> - if (psi->flags & PSTORE_FLAGS_CONSOLE)
> + if (psi->flags & PSTORE_FLAGS_CONSOLE
> + && !psback->front_cnt[PSTORE_TYPE_CONSOLE]++)
> pstore_register_console();
> - if (psi->flags & PSTORE_FLAGS_FTRACE)
> + if (psi->flags & PSTORE_FLAGS_FTRACE &&
> + !psback->front_cnt[PSTORE_TYPE_FTRACE]++)
> pstore_register_ftrace();
> - if (psi->flags & PSTORE_FLAGS_PMSG)
> + if (psi->flags & PSTORE_FLAGS_PMSG &&
> + !psback->front_cnt[PSTORE_TYPE_PMSG]++)
> pstore_register_pmsg();
>
> /* Start watching for new records, if desired. */
> pstore_timer_kick();
>
> /*
> - * Update the module parameter backend, so it is visible
> + * When module parameter backend is not specified,
> + * update the module parameter backend, so it is visible
> * through /sys/module/pstore/parameters/backend
> */
> - backend = new_backend;
> + if (!backend)
> + backend = new_backend;
>
> pr_info("Registered %s as persistent store backend\n", psi->name);
>
> - mutex_unlock(&psinfo_lock);
> + mutex_unlock(&psback_lock);
> return 0;
> }
> EXPORT_SYMBOL_GPL(pstore_register);
>
> void pstore_unregister(struct pstore_info *psi)
> {
> + struct pstore_info_list *entry;
> /* 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);
> + /* Can not unregister an unenabled backend*/
> + if (WARN_ON(!strstr(backend, psi->name) && strcmp(backend, "all")))
> return;
> - }
> +
> + mutex_lock(&psback_lock);
>
> /* Unregister all callbacks. */
> - if (psi->flags & PSTORE_FLAGS_PMSG)
> + if (psi->flags & PSTORE_FLAGS_PMSG &&
> + !--psback->front_cnt[PSTORE_TYPE_PMSG])
> pstore_unregister_pmsg();
> - if (psi->flags & PSTORE_FLAGS_FTRACE)
> + if (psi->flags & PSTORE_FLAGS_FTRACE &&
> + !--psback->front_cnt[PSTORE_TYPE_FTRACE])
> pstore_unregister_ftrace();
> - if (psi->flags & PSTORE_FLAGS_CONSOLE)
> + if (psi->flags & PSTORE_FLAGS_CONSOLE &&
> + !--psback->front_cnt[PSTORE_TYPE_CONSOLE])
> pstore_unregister_console();
> - if (psi->flags & PSTORE_FLAGS_DMESG)
> + if (psi->flags & PSTORE_FLAGS_DMESG &&
> + !--psback->front_cnt[PSTORE_TYPE_DMESG])
> pstore_unregister_kmsg();
>
> /* Stop timer and make sure all work has finished. */
> @@ -568,19 +637,30 @@ void pstore_unregister(struct pstore_info *psi)
> /* Remove all backend records from filesystem tree. */
> pstore_put_backend_records(psi);
>
> - free_buf_for_compression();
> + list_for_each_entry(entry, &psback->list_entry, list) {
Since you're using list_del below, doesn't this above need to use the
_safe version of the list walker?
> + if (entry->psi == psi) {
> + list_del_rcu(&entry->list);
> + psback->flag ^= 1 << entry->index;
> + synchronize_rcu();
Do we want the synchronize here? I think it might be better to do the
list removal first, and then outside of the psback_lock we can do it and
finalize the cleanup.
> + free_buf_for_compression(entry->index);
> + kfree(entry);
> + break;
> + }
> + }
>
> - psinfo = NULL;
> - kfree(backend);
> - backend = NULL;
> + if (psback->flag == PSOTRE_LIST_EMPTY) {
> + kfree(psback);
> + psback = NULL;
> + }
>
> pr_info("Unregistered %s as persistent store backend\n", psi->name);
As this might help with debugging, can you move this pr_info to near the
top of the function (and say "Unregistering '%s' ...") so any weird
problems reported to dmesg will show up _after_ this report of intent.
:)
> - mutex_unlock(&psinfo_lock);
> + mutex_unlock(&psback_lock);
> }
> EXPORT_SYMBOL_GPL(pstore_unregister);
>
> static void decompress_record(struct pstore_record *record,
> - struct z_stream_s *zstream)
> + struct z_stream_s *zstream,
> + struct pstore_info *psinfo)
> {
> int ret;
> int unzipped_len;
> @@ -697,7 +777,7 @@ void pstore_get_backend_records(struct pstore_info *psi,
> break;
> }
>
> - decompress_record(record, &zstream);
> + decompress_record(record, &zstream, psi);
> rc = pstore_mkfile(root, record);
> if (rc) {
> /* pstore_mkfile() did not take record, so free it. */
> @@ -729,7 +809,12 @@ void pstore_get_backend_records(struct pstore_info *psi,
>
> static void pstore_dowork(struct work_struct *work)
> {
> - pstore_get_records(1);
> + struct pstore_info_list *entry;
> +
> + mutex_lock(&psback_lock);
> + list_for_each_entry(entry, &psback->list_entry, list)
> + pstore_get_records(entry->psi, 1);
> + mutex_unlock(&psback_lock);
> }
>
> static void pstore_timefunc(struct timer_list *unused)
> @@ -745,11 +830,15 @@ static void pstore_timefunc(struct timer_list *unused)
> static int __init pstore_init(void)
> {
> int ret;
> + struct pstore_info_list *entry;
>
> ret = pstore_init_fs();
> - if (ret)
> - free_buf_for_compression();
> -
> + if (ret) {
> + mutex_lock(&psback_lock);
> + list_for_each_entry(entry, &psback->list_entry, list)
> + free_buf_for_compression(entry->index);
> + mutex_unlock(&psback_lock);
At init, we'll have no list, yes?
> + }
> return ret;
> }
> late_initcall(pstore_init);
> diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
> index 55f139afa327..9d5b8602e273 100644
> --- a/fs/pstore/pmsg.c
> +++ b/fs/pstore/pmsg.c
> @@ -11,8 +11,9 @@
>
> static DEFINE_MUTEX(pmsg_lock);
>
> -static ssize_t write_pmsg(struct file *file, const char __user *buf,
> - size_t count, loff_t *ppos)
> +static ssize_t do_write_pmsg(struct file *file, const char __user *buf,
> + size_t count, loff_t *ppos,
> + struct pstore_info *psinfo)
> {
> struct pstore_record record;
> int ret;
> @@ -34,6 +35,25 @@ static ssize_t write_pmsg(struct file *file, const char __user *buf,
> return ret ? ret : count;
> }
>
> +static ssize_t write_pmsg(struct file *file, const char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + int ret, _ret;
> + struct pstore_info_list *entry;
> +
> + mutex_lock(&psback_lock);
> + list_for_each_entry(entry, &psback->list_entry, list) {
> + if (entry->psi->flags & PSTORE_FLAGS_PMSG) {
> + _ret = do_write_pmsg(file, buf, count,
> + ppos, entry->psi);
> + ret = ret > _ret ? ret : _ret;
I'm not sure this "ret" handling is correct. What's your goal with it?
I think we need to report either the max seen _ret, or the first
negative return value.
(This reminds me of LSM stacking.)
so:
ssize_t err = 0;
ssize_t written = 0;
ssize_t ret;
for each {
ret = do_write_pmsg();
if (!err && ret < 0)
err = ret;
written = max(ret, written);
}
...
return err ? err : written;
So we'll return an error if we hit one, otherwise the longest scan of
written bytes.
This does run the risk of breaking backends that will split up writes,
but I don't think that's a problem in reality yet.
For example, if "count" was 100, and a backend wrote 10, pstore_write()
will return 10, and userspace will turn around and try to write the next
90 bytes. If we want to support it, we'll need to do the retry within
the above code, but I think that's overkill currently. Maybe just leave
a comment about it for now.
> + }
> + }
> + mutex_unlock(&psback_lock);
> +
> + return ret;
> +}
> +
> static const struct file_operations pmsg_fops = {
> .owner = THIS_MODULE,
> .llseek = noop_llseek,
> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> index 638507a3c8ff..0d2be20c8929 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -201,6 +201,35 @@ struct pstore_info {
> int (*erase)(struct pstore_record *record);
> };
>
> +/* Supported multibackends */
> +#define PSTORE_MAX_BACKEND_LENGTH 100
> +#define PSTORE_BACKEND_NUM 16
> +
> +#define PSTORE_LIST_FULL (BIT(PSTORE_BACKEND_NUM) - 1)
> +#define PSOTRE_LIST_EMPTY 0
> +
> +extern struct mutex psback_lock;
> +
> +struct pstore_info_list {
> + struct pstore_info *psi;
> + struct list_head list;
> + int index;
> +};
> +
> +/**
> + * struct pstore_backends - management of pstore backends
> + * @list_entry: entry of pstore backend driver information list
> + * @front_cnt: count of each enabled frontend
> + * @flag: bitmap of enabled pstore backend
> + *
> + */
> +
> +struct pstore_backends {
> + struct list_head list_entry;
> + int front_cnt[PSTORE_TYPE_MAX];
> + u16 flag;
> +};
I think all of this can just live in pstore_info_list, yes?
> +
> /* Supported frontends */
> #define PSTORE_FLAGS_DMESG BIT(0)
> #define PSTORE_FLAGS_CONSOLE BIT(1)
> --
> 2.39.3
>
This is continuing to look good! Thanks for the work. :)
--
Kees Cook
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] Documentation: adjust pstore backend related document
2024-02-07 2:19 ` [PATCH 2/3] Documentation: adjust pstore backend related document Yuanhe Shu
@ 2024-02-07 12:51 ` Kees Cook
0 siblings, 0 replies; 13+ messages in thread
From: Kees Cook @ 2024-02-07 12:51 UTC (permalink / raw)
To: Yuanhe Shu
Cc: tony.luck, gpiccoli, shuah, corbet, xlpang, linux-kernel,
linux-doc, linux-hardening, linux-kselftest
On Wed, Feb 07, 2024 at 10:19:20AM +0800, Yuanhe Shu wrote:
> Pstore now supports multiple backends, adjust related document.
>
> Signed-off-by: Yuanhe Shu <xiangzao@linux.alibaba.com>
> ---
> Documentation/ABI/testing/pstore | 8 ++++----
> Documentation/admin-guide/kernel-parameters.txt | 4 +++-
> 2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/ABI/testing/pstore b/Documentation/ABI/testing/pstore
> index d3cff4a7ee10..2cd67b502f11 100644
> --- a/Documentation/ABI/testing/pstore
> +++ b/Documentation/ABI/testing/pstore
> @@ -41,7 +41,7 @@ Description: Generic interface to platform dependent persistent storage.
> persistent storage until at least this amount is reached.
> Default is 10 Kbytes.
>
> - Pstore only supports one backend at a time. If multiple
> - backends are available, the preferred backend may be
> - set by passing the pstore.backend= argument to the kernel at
> - boot time.
> + Pstore now supports multiple backends at a time. If multiple
I'd drop "now"
> + backends are available, the registrable backends may be
> + set by passing the pstore.backend= argument1, argument2...
> + or pstore.backend= all to the kernel at boot time.
I think dropping the cases of " " after the "=" and "," above would make
this more readable (and syntactically correct).
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 31b3a25680d0..a8a109b822a9 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4748,7 +4748,9 @@
> [HW,MOUSE] Controls Logitech smartscroll autorepeat.
> 0 = disabled, 1 = enabled (default).
>
> - pstore.backend= Specify the name of the pstore backend to use
> + pstore.backend=backend1,...,backendN
> + Specify the names of the pstore backends to use
> + or =all for all available backends
>
> pti= [X86-64] Control Page Table Isolation of user and
> kernel address spaces. Disabling this feature
> --
> 2.39.3
>
Otherwise looks good.
--
Kees Cook
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] tools/testing: adjust pstore backend related selftest
2024-02-07 2:19 ` [PATCH 3/3] tools/testing: adjust pstore backend related selftest Yuanhe Shu
@ 2024-02-07 12:53 ` Kees Cook
2024-02-20 12:31 ` Yuanhe Shu
0 siblings, 1 reply; 13+ messages in thread
From: Kees Cook @ 2024-02-07 12:53 UTC (permalink / raw)
To: Yuanhe Shu
Cc: tony.luck, gpiccoli, shuah, corbet, xlpang, linux-kernel,
linux-doc, linux-hardening, linux-kselftest
On Wed, Feb 07, 2024 at 10:19:21AM +0800, Yuanhe Shu wrote:
> Pstore now supports multiple backends, the module parameter
> pstore.backend varies from 'registered backend' to 'backends that are
> allowed to register'. Adjust selftests to match the change.
>
> Signed-off-by: Yuanhe Shu <xiangzao@linux.alibaba.com>
> ---
> tools/testing/selftests/pstore/common_tests | 8 +--
> .../selftests/pstore/pstore_post_reboot_tests | 65 ++++++++++---------
> tools/testing/selftests/pstore/pstore_tests | 2 +-
> 3 files changed, 38 insertions(+), 37 deletions(-)
>
> diff --git a/tools/testing/selftests/pstore/common_tests b/tools/testing/selftests/pstore/common_tests
> index 4509f0cc9c91..497e6fc3215f 100755
> --- a/tools/testing/selftests/pstore/common_tests
> +++ b/tools/testing/selftests/pstore/common_tests
> @@ -27,9 +27,9 @@ show_result() { # result_value
> }
>
> check_files_exist() { # type of pstorefs file
> - if [ -e ${1}-${backend}-0 ]; then
> + if [ -e ${1}-${2}-0 ]; then
> prlog "ok"
> - for f in `ls ${1}-${backend}-*`; do
> + for f in `ls ${1}-${2}-*`; do
> prlog -e "\t${f}"
> done
> else
> @@ -74,9 +74,9 @@ prlog "=== Pstore unit tests (`basename $0`) ==="
> prlog "UUID="$UUID
>
> prlog -n "Checking pstore backend is registered ... "
> -backend=`cat /sys/module/pstore/parameters/backend`
> +backends=$(dmesg | sed -n 's/.*pstore: Registered \(.*\) as persistent store backend.*/\1/p')
> show_result $?
> -prlog -e "\tbackend=${backend}"
> +prlog -e "\tbackends="$backends
Missing trailing "? Also, doesn't this end up printing multiple lines?
Perhaps, like LSM stacking, we need a /sys/module entry for the list of
backends, comma separated?
> prlog -e "\tcmdline=`cat /proc/cmdline`"
> if [ $rc -ne 0 ]; then
> exit 1
> diff --git a/tools/testing/selftests/pstore/pstore_post_reboot_tests b/tools/testing/selftests/pstore/pstore_post_reboot_tests
> index d6da5e86efbf..9e40ccb9c918 100755
> --- a/tools/testing/selftests/pstore/pstore_post_reboot_tests
> +++ b/tools/testing/selftests/pstore/pstore_post_reboot_tests
> @@ -36,45 +36,46 @@ else
> fi
>
> cd ${mount_point}
> +for backend in ${backends}; do
> + prlog -n "Checking ${backend}-dmesg files exist in pstore filesystem ... "
> + check_files_exist dmesg ${backend}
>
> -prlog -n "Checking dmesg files exist in pstore filesystem ... "
> -check_files_exist dmesg
> + prlog -n "Checking ${backend}-console files exist in pstore filesystem ... "
> + check_files_exist console ${backend}
>
> -prlog -n "Checking console files exist in pstore filesystem ... "
> -check_files_exist console
> + prlog -n "Checking ${backend}-pmsg files exist in pstore filesystem ... "
> + check_files_exist pmsg ${backend}
>
> -prlog -n "Checking pmsg files exist in pstore filesystem ... "
> -check_files_exist pmsg
> + prlog -n "Checking ${backend}-dmesg files contain oops end marker"
> + grep_end_trace() {
> + grep -q "\---\[ end trace" $1
> + }
> + files=`ls dmesg-${backend}-*`
> + operate_files $? "$files" grep_end_trace
>
> -prlog -n "Checking dmesg files contain oops end marker"
> -grep_end_trace() {
> - grep -q "\---\[ end trace" $1
> -}
> -files=`ls dmesg-${backend}-*`
> -operate_files $? "$files" grep_end_trace
> + prlog -n "Checking ${backend}-console file contains oops end marker ... "
> + grep -q "\---\[ end trace" console-${backend}-0
> + show_result $?
>
> -prlog -n "Checking console file contains oops end marker ... "
> -grep -q "\---\[ end trace" console-${backend}-0
> -show_result $?
> -
> -prlog -n "Checking pmsg file properly keeps the content written before crash ... "
> -prev_uuid=`cat $TOP_DIR/prev_uuid`
> -if [ $? -eq 0 ]; then
> - nr_matched=`grep -c "$TEST_STRING_PATTERN" pmsg-${backend}-0`
> - if [ $nr_matched -eq 1 ]; then
> - grep -q "$TEST_STRING_PATTERN"$prev_uuid pmsg-${backend}-0
> - show_result $?
> + prlog -n "Checking ${backend}-pmsg file properly keeps the content written before crash ... "
> + prev_uuid=`cat $TOP_DIR/prev_uuid`
> + if [ $? -eq 0 ]; then
> + nr_matched=`grep -c "$TEST_STRING_PATTERN" pmsg-${backend}-0`
> + if [ $nr_matched -eq 1 ]; then
> + grep -q "$TEST_STRING_PATTERN"$prev_uuid pmsg-${backend}-0
> + show_result $?
> + else
> + prlog "FAIL"
> + rc=1
> + fi
> else
> - prlog "FAIL"
> - rc=1
> + prlog "FAIL"
> + rc=1
> fi
> -else
> - prlog "FAIL"
> - rc=1
> -fi
>
> -prlog -n "Removing all files in pstore filesystem "
> -files=`ls *-${backend}-*`
> -operate_files $? "$files" rm
> + prlog -n "Removing all ${backend} files in pstore filesystem "
> + files=`ls *-${backend}-*`
> + operate_files $? "$files" rm
> +done
>
> exit $rc
> diff --git a/tools/testing/selftests/pstore/pstore_tests b/tools/testing/selftests/pstore/pstore_tests
> index 2aa9a3852a84..f4665a8c77dc 100755
> --- a/tools/testing/selftests/pstore/pstore_tests
> +++ b/tools/testing/selftests/pstore/pstore_tests
> @@ -10,7 +10,7 @@
> . ./common_tests
>
> prlog -n "Checking pstore console is registered ... "
> -dmesg | grep -Eq "console \[(pstore|${backend})"
> +dmesg | grep -Eq "console \[(pstore console)"
> show_result $?
>
> prlog -n "Checking /dev/pmsg0 exists ... "
> --
> 2.39.3
>
Otherwise seems ok
--
Kees Cook
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] pstore: add multi-backend support
2024-02-07 12:48 ` Kees Cook
@ 2024-02-20 12:19 ` Yuanhe Shu
0 siblings, 0 replies; 13+ messages in thread
From: Yuanhe Shu @ 2024-02-20 12:19 UTC (permalink / raw)
To: Kees Cook
Cc: tony.luck, gpiccoli, shuah, corbet, xlpang, linux-kernel,
linux-doc, linux-hardening, linux-kselftest, Xingrui Yi, xiangzao
On 2024/2/7 20:48, Kees Cook wrote:
> On Wed, Feb 07, 2024 at 10:19:19AM +0800, Yuanhe Shu wrote:
>> Currently, pstore supports only one backend open at a time.
>> Specifically, due to the global variable "psinfo", pstore only accepts
>> the first registered backend. If a new backend wants to register later,
>> pstore will simply reject it and return an error. This design forced us
>> to close existing backend in order to use the new ones.
>>
>> To enable pstore to support multiple backends, "psinfo" is replaced by
>> "psinfo_list", a list that holds multiple "psinfo". If multiple backends
>> are registered with the same frontend, the frontend is reused.
>>
>> User can specify multiple backends that are allowed to be registered by
>> module parameter "pstore.backend=" separated by commas or "all" to
>> enable all available backends. If no pstore.backend was specified,
>> pstore would accept the first registered backend which is the same as
>> before.
>>
>> Signed-off-by: Xingrui Yi <yixingrui@linux.alibaba.com>
>> Signed-off-by: Yuanhe Shu <xiangzao@linux.alibaba.com>
>> ---
>> fs/pstore/ftrace.c | 31 +++++-
>> fs/pstore/inode.c | 19 ++--
>> fs/pstore/internal.h | 4 +-
>> fs/pstore/platform.c | 225 ++++++++++++++++++++++++++++-------------
>> fs/pstore/pmsg.c | 24 ++++-
>> include/linux/pstore.h | 29 ++++++
>> 6 files changed, 250 insertions(+), 82 deletions(-)
>>
>> diff --git a/fs/pstore/ftrace.c b/fs/pstore/ftrace.c
>> index 776cae20af4e..49b9c8532dab 100644
>> --- a/fs/pstore/ftrace.c
>> +++ b/fs/pstore/ftrace.c
>> @@ -23,10 +23,11 @@
>> /* This doesn't need to be atomic: speed is chosen over correctness here. */
>> static u64 pstore_ftrace_stamp;
>>
>> -static void notrace pstore_ftrace_call(unsigned long ip,
>> +static void notrace pstore_do_ftrace(unsigned long ip,
>> unsigned long parent_ip,
>> struct ftrace_ops *op,
>> - struct ftrace_regs *fregs)
>> + struct ftrace_regs *fregs,
>> + struct pstore_info *psinfo)
>> {
>> int bit;
>> unsigned long flags;
>> @@ -57,6 +58,20 @@ static void notrace pstore_ftrace_call(unsigned long ip,
>> ftrace_test_recursion_unlock(bit);
>> }
>>
>> +static void notrace pstore_ftrace_call(unsigned long ip,
>> + unsigned long parent_ip,
>> + struct ftrace_ops *op,
>> + struct ftrace_regs *fregs)
>> +{
>> + struct pstore_info_list *entry;
>> +
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(entry, &psback->list_entry, list)
>> + if (entry->psi->flags & PSTORE_FLAGS_FTRACE)
>> + pstore_do_ftrace(ip, parent_ip, op, fregs, entry->psi);
>> + rcu_read_unlock();
>
> Just to make sure I understand the purpose of the RCU here is to deal
> with having a backend get unregistered while something is walking this
> list? And to do so without holding the global psback_lock?
>
Yes, the purpose of the RCU here is the same as you said. We worry about
global mutex would be somehow inefficient as list add/del is relatively
low-frequency compared to just walking the list and do some
pstore_xxx_call. Thus operations on list, e.g. add/del, and those would
be might_sleep, are protected by global mutex. The other list walking
operations are protected by rcu lock.
Sorry, but we forgot pstore_xxx_call, e.g. pstore_ftrace_call, would
call all the backend in the list. The unregistered backend should not do
pstore_xxx_call. I will replace all the RCU lock with global psback_lock.
>> +}
>> +
>> static struct ftrace_ops pstore_ftrace_ops __read_mostly = {
>> .func = pstore_ftrace_call,
>> };
>> @@ -131,8 +146,16 @@ MODULE_PARM_DESC(record_ftrace,
>>
>> void pstore_register_ftrace(void)
>> {
>> - if (!psinfo->write)
>> - return;
>> + struct pstore_info_list *entry;
>> +
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(entry, &psback->list_entry, list)
>> + if (entry->psi->flags & PSTORE_FLAGS_FTRACE)
>> + if (!entry->psi->write) {
>> + rcu_read_unlock();
>> + return;
>> + }
>> + rcu_read_unlock();
>>
>> pstore_ftrace_dir = debugfs_create_dir("pstore", NULL);
>>
>> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
>> index d0d9bfdad30c..bee71c7da995 100644
>> --- a/fs/pstore/inode.c
>> +++ b/fs/pstore/inode.c
>> @@ -285,7 +285,7 @@ static const struct super_operations pstore_ops = {
>> .show_options = pstore_show_options,
>> };
>>
>> -static struct dentry *psinfo_lock_root(void)
>> +static struct dentry *psinfo_lock_root(struct pstore_info *psinfo)
>> {
>> struct dentry *root;
>>
>> @@ -309,7 +309,7 @@ int pstore_put_backend_records(struct pstore_info *psi)
>> struct dentry *root;
>> int rc = 0;
>>
>> - root = psinfo_lock_root();
>> + root = psinfo_lock_root(psi);
>> if (!root)
>> return 0;
>>
>> @@ -398,21 +398,22 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
>> * when we are re-scanning the backing store looking to add new
>> * error records.
>> */
>> -void pstore_get_records(int quiet)
>> +void pstore_get_records(struct pstore_info *psi, int quiet)
>> {
>> struct dentry *root;
>>
>> - root = psinfo_lock_root();
>> + root = psinfo_lock_root(psi);
>> if (!root)
>> return;
>>
>> - pstore_get_backend_records(psinfo, root, quiet);
>> + pstore_get_backend_records(psi, root, quiet);
>> inode_unlock(d_inode(root));
>> }
>>
>> static int pstore_fill_super(struct super_block *sb, void *data, int silent)
>> {
>> struct inode *inode;
>> + struct pstore_info_list *entry;
>>
>> sb->s_maxbytes = MAX_LFS_FILESIZE;
>> sb->s_blocksize = PAGE_SIZE;
>> @@ -437,7 +438,13 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent)
>> scoped_guard(mutex, &pstore_sb_lock)
>> pstore_sb = sb;
>>
>> - pstore_get_records(0);
>> + if (!psback)
>> + return 0;
>> +
>> + mutex_lock(&psback_lock);
>> + list_for_each_entry(entry, &psback->list_entry, list)
>> + pstore_get_records(entry->psi, 0);
>> + mutex_unlock(&psback_lock);
>>
>> return 0;
>> }
>> diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
>> index 801d6c0b170c..4b1c7ba27052 100644
>> --- a/fs/pstore/internal.h
>> +++ b/fs/pstore/internal.h
>> @@ -33,10 +33,10 @@ static inline void pstore_register_pmsg(void) {}
>> static inline void pstore_unregister_pmsg(void) {}
>> #endif
>>
>> -extern struct pstore_info *psinfo;
>> +extern struct pstore_backends *psback;
>>
>> extern void pstore_set_kmsg_bytes(int);
>> -extern void pstore_get_records(int);
>> +extern void pstore_get_records(struct pstore_info *psi, int quiet);
>> 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);
>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
>> index 03425928d2fb..432a41852a07 100644
>> --- a/fs/pstore/platform.c
>> +++ b/fs/pstore/platform.c
>> @@ -62,12 +62,12 @@ static void pstore_dowork(struct work_struct *);
>> static DECLARE_WORK(pstore_work, pstore_dowork);
>>
>> /*
>> - * psinfo_lock protects "psinfo" during calls to
>> + * psback_lock protects "psback" during calls to
>> * pstore_register(), pstore_unregister(), and
>> * the filesystem mount/unmount routines.
>> */
>> -static DEFINE_MUTEX(psinfo_lock);
>> -struct pstore_info *psinfo;
>> +DEFINE_MUTEX(psback_lock);
>> +struct pstore_backends *psback;
>>
>> static char *backend;
>> module_param(backend, charp, 0444);
>> @@ -104,7 +104,7 @@ static void *compress_workspace;
>> */
>> #define DMESG_COMP_PERCENT 60
>>
>> -static char *big_oops_buf;
>> +static char *big_oops_buf[PSTORE_BACKEND_NUM];
>
> I think big_oops_buf should live in the pstore_info_list struct, then
> no indexes are needed to be tracked.
>
>> static size_t max_compressed_size;
>>
>> void pstore_set_kmsg_bytes(int bytes)
>> @@ -201,7 +201,7 @@ static int pstore_compress(const void *in, void *out,
>> return zstream.total_out;
>> }
>>
>> -static void allocate_buf_for_compression(void)
>> +static void allocate_buf_for_compression(struct pstore_info *psinfo, int pos)
>> {
>> size_t compressed_size;
>> char *buf;
>> @@ -241,21 +241,21 @@ static void allocate_buf_for_compression(void)
>> }
>>
>> /* A non-NULL big_oops_buf indicates compression is available. */
>> - big_oops_buf = buf;
>> + big_oops_buf[pos] = buf;
>
> e.g.: entry->big_oops_buf = buf;
>
>> max_compressed_size = compressed_size;
>
> And should this be included as well, or are we just constantly setting
> this to the same value?
>
Maybe we should include max_compressed_size as well? If one backend is
unregistered, free_buf_for_compression is called to set
max_compressed_size to 0, and dst_size in pstore_do_dump fall to
psinfo->bufsize which is different from the origin max_compressed_size
(which is (psinfo->bufsize * 100) / DMESG_COMP_PERCENT).
>>
>> pr_info("Using crash dump compression: %s\n", compress);
>> }
>>
>> -static void free_buf_for_compression(void)
>> +static void free_buf_for_compression(int pos)
>
> Instead of "pos" this would take the ps_info.
>
>> {
>> if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && compress_workspace) {
>> vfree(compress_workspace);
>> compress_workspace = NULL;
>> }
>>
>> - kvfree(big_oops_buf);
>> - big_oops_buf = NULL;
>> + kvfree(big_oops_buf[pos]);
>> + big_oops_buf[pos] = NULL;
>> max_compressed_size = 0;
>> }
>>
>> @@ -274,8 +274,9 @@ void pstore_record_init(struct pstore_record *record,
>> * callback from kmsg_dump. Save as much as we can (up to kmsg_bytes) from the
>> * end of the buffer.
>> */
>> -static void pstore_dump(struct kmsg_dumper *dumper,
>> - enum kmsg_dump_reason reason)
>> +static void pstore_do_dump(struct kmsg_dumper *dumper,
>> + enum kmsg_dump_reason reason,
>> + struct pstore_info *psinfo, int pos)
>> {
>> struct kmsg_dump_iter iter;
>> unsigned long total = 0;
>> @@ -315,7 +316,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>> record.part = part;
>> record.buf = psinfo->buf;
>>
>> - dst = big_oops_buf ?: psinfo->buf;
>> + dst = big_oops_buf[pos] ?: psinfo->buf;
>> dst_size = max_compressed_size ?: psinfo->bufsize;
>>
>> /* Write dump header. */
>> @@ -328,7 +329,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>> dst_size, &dump_size))
>> break;
>>
>> - if (big_oops_buf) {
>> + if (big_oops_buf[pos]) {
>> zipped_len = pstore_compress(dst, psinfo->buf,
>> header_size + dump_size,
>> psinfo->bufsize);
>> @@ -372,6 +373,19 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>> }
>> }
>>
>> +static void pstore_dump(struct kmsg_dumper *dumper,
>> + enum kmsg_dump_reason reason)
>> +{
>> + struct pstore_info_list *entry;
>> +
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(entry, &psback->list_entry, list)
>> + if (entry->psi->flags & PSTORE_FLAGS_DMESG)
>> + pstore_do_dump(dumper, reason,
>> + entry->psi, entry->index);
>
> Then no need for index here.
>
>> + rcu_read_unlock();
>> +}
>> +
>> static struct kmsg_dumper pstore_dumper = {
>> .dump = pstore_dump,
>> };
>> @@ -390,13 +404,11 @@ static void pstore_unregister_kmsg(void)
>> }
>>
>> #ifdef CONFIG_PSTORE_CONSOLE
>> -static void pstore_console_write(struct console *con, const char *s, unsigned c)
>> +static void pstore_console_do_write(struct console *con, const char *s,
>> + unsigned c, struct pstore_info *psinfo)
>> {
>> struct pstore_record record;
>>
>> - if (!c)
>> - return;
>> -
>> pstore_record_init(&record, psinfo);
>> record.type = PSTORE_TYPE_CONSOLE;
>>
>> @@ -405,6 +417,21 @@ static void pstore_console_write(struct console *con, const char *s, unsigned c)
>> psinfo->write(&record);
>> }
>>
>> +static void pstore_console_write(struct console *con, const char *s,
>> + unsigned int c)
>> +{
>> + struct pstore_info_list *entry;
>> +
>> + if (!c)
>> + return;
>> +
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(entry, &psback->list_entry, list)
>> + if (entry->psi->flags & PSTORE_FLAGS_CONSOLE)
>> + pstore_console_do_write(con, s, c, entry->psi);
>> + rcu_read_unlock();
>> +}
>> +
>> static struct console pstore_console = {
>> .write = pstore_console_write,
>> .index = -1,
>> @@ -413,7 +440,7 @@ static struct console pstore_console = {
>> static void pstore_register_console(void)
>> {
>> /* Show which backend is going to get console writes. */
>> - strscpy(pstore_console.name, psinfo->name,
>> + strscpy(pstore_console.name, "pstore console",
>> sizeof(pstore_console.name));
>
> A nice-to-have for this might be to include all the backend names
> included, but that is probably overkill, so this is fine.
>
>> /*
>> * Always initialize flags here since prior unregister_console()
>> @@ -464,12 +491,15 @@ static int pstore_write_user_compat(struct pstore_record *record,
>> */
>> int pstore_register(struct pstore_info *psi)
>> {
>> + struct pstore_info_list *entry;
>> + struct pstore_info_list *newpsi;
>> char *new_backend;
>>
>> - if (backend && strcmp(backend, psi->name)) {
>> - pr_warn("backend '%s' already in use: ignoring '%s'\n",
>> - backend, psi->name);
>> - return -EBUSY;
>> + /* backend has to be enabled for going on registering */
>> + if (backend && !strstr(backend, psi->name) &&
>
> I think I'd like a helper to do the "is this name in the list?", as that
> would be more robust. e.g. if we ever had a "ram" backend and a "cram"
> backend, and someone used "backend=cram", suddenly both "cram" and "ram"
> match in the code above.
>
>> + strcmp(backend, "all")) {
>
> style preference, can you make this strcmp be:
>
> strcmp(backend, "all") != 0
>
> it seems redundant, but it helps readers remember that "strcmp() == 0"
> is "matches".
>
>
>> + pr_warn("backend '%s' not enabled\n", psi->name);
>
> nit, I think a more clear error might be something like:
>
> "backend '%s' ignored: not present in pstore.backend=...\n"
>
> which gives people a little hint about what's wrong.
>
>> + return -EINVAL;
>> }
>>
>> /* Sanity check flags. */
>> @@ -486,79 +516,118 @@ int pstore_register(struct pstore_info *psi)
>> return -EINVAL;
>> }
>>
>> - new_backend = kstrdup(psi->name, GFP_KERNEL);
>> - if (!new_backend)
>> - return -ENOMEM;
>> -
>> - mutex_lock(&psinfo_lock);
>> - if (psinfo) {
>> - pr_warn("backend '%s' already loaded: ignoring '%s'\n",
>> - psinfo->name, psi->name);
>> - mutex_unlock(&psinfo_lock);
>> - kfree(new_backend);
>> - return -EBUSY;
>> + mutex_lock(&psback_lock);
>> +
>> + /*
>> + * If no backend specified, first come first served to
>> + * maintain backward compatibility
>> + */
>> + if (!backend) {
>> + pr_warn("no backend enabled, registering backend '%s'\n",
>> + psi->name);
>
> I'd move this to after the if below, so you can't get this message if it
> were going to just immediately fail. And a nit on language. I think this
> would be more clear:
>
> "pstore.backend=... not specified; registering first available: '%s'\n"
>
>> + new_backend = kstrdup(psi->name, GFP_KERNEL);
>> + if (!new_backend) {
>> + mutex_unlock(&psback_lock);
>> + return -ENOMEM;
>> + }
>> + }
>> +
>> + if (psback) {
>> + if (psback->flag == PSTORE_LIST_FULL) {
>> + pr_warn("backend registration space is used up: "
>> + "ignoring '%s'\n", psi->name);
>> + mutex_unlock(&psback_lock);
>> + return -EBUSY;
>> + }
>
> All of this flag/index stuff can go away with big_oops_buf moved.
>
>> + list_for_each_entry(entry, &psback->list_entry, list) {
>> + if (strcmp(entry->psi->name, psi->name) == 0) {
>> + pr_warn("backend '%s' already loaded\n",
>> + psi->name);
>
> "backend '%s' already loaded; not loading it again\n"
>
>> + mutex_unlock(&psback_lock);
>> + return -EPERM;
>> + }
>> + }
>> + } else {
>> + psback = kzalloc(sizeof(*psback), GFP_KERNEL);
>> + INIT_LIST_HEAD(&psback->list_entry);
>> }
>>
>> if (!psi->write_user)
>> psi->write_user = pstore_write_user_compat;
>> - psinfo = psi;
>> - mutex_init(&psinfo->read_mutex);
>> - spin_lock_init(&psinfo->buf_lock);
>> + newpsi = kzalloc(sizeof(*newpsi), GFP_KERNEL);
>> + newpsi->psi = psi;
>> + newpsi->index = ffz(psback->flag);
>> + psback->flag |= (1 << newpsi->index);
>> +
>> + mutex_init(&psi->read_mutex);
>> + spin_lock_init(&psi->buf_lock);
>> +
>> + if (psi->flags & PSTORE_FLAGS_DMESG &&
>> + !psback->front_cnt[PSTORE_TYPE_DMESG])
>> + allocate_buf_for_compression(psi, newpsi->index);
>
> Can't we just ignore the front_cnt? The only reason to have
> big_oops_buf is if the backend supports PSTORE_FLAGS_DMESG, yes?
>
>>
>> - if (psi->flags & PSTORE_FLAGS_DMESG)
>> - allocate_buf_for_compression();
>> + pstore_get_records(psi, 0);
>>
>> - pstore_get_records(0);
>> + list_add_rcu(&newpsi->list, &psback->list_entry);
>>
>> - if (psi->flags & PSTORE_FLAGS_DMESG) {
>> - pstore_dumper.max_reason = psinfo->max_reason;
>> + if (psi->flags & PSTORE_FLAGS_DMESG &&
>> + !psback->front_cnt[PSTORE_TYPE_DMESG]++) {
>> + pstore_dumper.max_reason = psi->max_reason;
>> pstore_register_kmsg();
>> }
>
> Hmm... does this mean we need multiple pstore_dump instances? (i.e. like
> big_oops_buf) The backend configures the max_reason, so we'll need to
> register multiple kmsg handlers to have different max_reasons. Right now
> this just means "last registered backend wins" which would be
> surprising.
>
> Let's add a pstore_dumper instance to pstore_info_list, along with
> big_oops_buf. All of these globals are really per-backend instances now.
>
>> - if (psi->flags & PSTORE_FLAGS_CONSOLE)
>> + if (psi->flags & PSTORE_FLAGS_CONSOLE
>> + && !psback->front_cnt[PSTORE_TYPE_CONSOLE]++)
>> pstore_register_console();
>> - if (psi->flags & PSTORE_FLAGS_FTRACE)
>> + if (psi->flags & PSTORE_FLAGS_FTRACE &&
>> + !psback->front_cnt[PSTORE_TYPE_FTRACE]++)
>> pstore_register_ftrace();
>> - if (psi->flags & PSTORE_FLAGS_PMSG)
>> + if (psi->flags & PSTORE_FLAGS_PMSG &&
>> + !psback->front_cnt[PSTORE_TYPE_PMSG]++)
>> pstore_register_pmsg();
>>
>> /* Start watching for new records, if desired. */
>> pstore_timer_kick();
>>
>> /*
>> - * Update the module parameter backend, so it is visible
>> + * When module parameter backend is not specified,
>> + * update the module parameter backend, so it is visible
>> * through /sys/module/pstore/parameters/backend
>> */
>> - backend = new_backend;
>> + if (!backend)
>> + backend = new_backend;
>>
>> pr_info("Registered %s as persistent store backend\n", psi->name);
>>
>> - mutex_unlock(&psinfo_lock);
>> + mutex_unlock(&psback_lock);
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(pstore_register);
>>
>> void pstore_unregister(struct pstore_info *psi)
>> {
>> + struct pstore_info_list *entry;
>> /* 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);
>> + /* Can not unregister an unenabled backend*/
>> + if (WARN_ON(!strstr(backend, psi->name) && strcmp(backend, "all")))
>> return;
>> - }
>> +
>> + mutex_lock(&psback_lock);
>>
>> /* Unregister all callbacks. */
>> - if (psi->flags & PSTORE_FLAGS_PMSG)
>> + if (psi->flags & PSTORE_FLAGS_PMSG &&
>> + !--psback->front_cnt[PSTORE_TYPE_PMSG])
>> pstore_unregister_pmsg();
>> - if (psi->flags & PSTORE_FLAGS_FTRACE)
>> + if (psi->flags & PSTORE_FLAGS_FTRACE &&
>> + !--psback->front_cnt[PSTORE_TYPE_FTRACE])
>> pstore_unregister_ftrace();
>> - if (psi->flags & PSTORE_FLAGS_CONSOLE)
>> + if (psi->flags & PSTORE_FLAGS_CONSOLE &&
>> + !--psback->front_cnt[PSTORE_TYPE_CONSOLE])
>> pstore_unregister_console();
>> - if (psi->flags & PSTORE_FLAGS_DMESG)
>> + if (psi->flags & PSTORE_FLAGS_DMESG &&
>> + !--psback->front_cnt[PSTORE_TYPE_DMESG])
>> pstore_unregister_kmsg();
>>
>> /* Stop timer and make sure all work has finished. */
>> @@ -568,19 +637,30 @@ void pstore_unregister(struct pstore_info *psi)
>> /* Remove all backend records from filesystem tree. */
>> pstore_put_backend_records(psi);
>>
>> - free_buf_for_compression();
>> + list_for_each_entry(entry, &psback->list_entry, list) {
>
> Since you're using list_del below, doesn't this above need to use the
> _safe version of the list walker?
>
>> + if (entry->psi == psi) {
>> + list_del_rcu(&entry->list);
>> + psback->flag ^= 1 << entry->index;
>> + synchronize_rcu();
>
> Do we want the synchronize here? I think it might be better to do the
> list removal first, and then outside of the psback_lock we can do it and
> finalize the cleanup.
>
>> + free_buf_for_compression(entry->index);
>> + kfree(entry);
>> + break;
>> + }
>> + }
>>
>> - psinfo = NULL;
>> - kfree(backend);
>> - backend = NULL;
>> + if (psback->flag == PSOTRE_LIST_EMPTY) {
>> + kfree(psback);
>> + psback = NULL;
>> + }
>>
>> pr_info("Unregistered %s as persistent store backend\n", psi->name);
>
> As this might help with debugging, can you move this pr_info to near the
> top of the function (and say "Unregistering '%s' ...") so any weird
> problems reported to dmesg will show up _after_ this report of intent.
> :)
>
>> - mutex_unlock(&psinfo_lock);
>> + mutex_unlock(&psback_lock);
>> }
>> EXPORT_SYMBOL_GPL(pstore_unregister);
>>
>> static void decompress_record(struct pstore_record *record,
>> - struct z_stream_s *zstream)
>> + struct z_stream_s *zstream,
>> + struct pstore_info *psinfo)
>> {
>> int ret;
>> int unzipped_len;
>> @@ -697,7 +777,7 @@ void pstore_get_backend_records(struct pstore_info *psi,
>> break;
>> }
>>
>> - decompress_record(record, &zstream);
>> + decompress_record(record, &zstream, psi);
>> rc = pstore_mkfile(root, record);
>> if (rc) {
>> /* pstore_mkfile() did not take record, so free it. */
>> @@ -729,7 +809,12 @@ void pstore_get_backend_records(struct pstore_info *psi,
>>
>> static void pstore_dowork(struct work_struct *work)
>> {
>> - pstore_get_records(1);
>> + struct pstore_info_list *entry;
>> +
>> + mutex_lock(&psback_lock);
>> + list_for_each_entry(entry, &psback->list_entry, list)
>> + pstore_get_records(entry->psi, 1);
>> + mutex_unlock(&psback_lock);
>> }
>>
>> static void pstore_timefunc(struct timer_list *unused)
>> @@ -745,11 +830,15 @@ static void pstore_timefunc(struct timer_list *unused)
>> static int __init pstore_init(void)
>> {
>> int ret;
>> + struct pstore_info_list *entry;
>>
>> ret = pstore_init_fs();
>> - if (ret)
>> - free_buf_for_compression();
>> -
>> + if (ret) {
>> + mutex_lock(&psback_lock);
>> + list_for_each_entry(entry, &psback->list_entry, list)
>> + free_buf_for_compression(entry->index);
>> + mutex_unlock(&psback_lock);
>
> At init, we'll have no list, yes?
>
Yes, here we don't have list and no backend is registered.
Thus no need to do the free?
>> + }
>> return ret;
>> }
>> late_initcall(pstore_init);
>> diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
>> index 55f139afa327..9d5b8602e273 100644
>> --- a/fs/pstore/pmsg.c
>> +++ b/fs/pstore/pmsg.c
>> @@ -11,8 +11,9 @@
>>
>> static DEFINE_MUTEX(pmsg_lock);
>>
>> -static ssize_t write_pmsg(struct file *file, const char __user *buf,
>> - size_t count, loff_t *ppos)
>> +static ssize_t do_write_pmsg(struct file *file, const char __user *buf,
>> + size_t count, loff_t *ppos,
>> + struct pstore_info *psinfo)
>> {
>> struct pstore_record record;
>> int ret;
>> @@ -34,6 +35,25 @@ static ssize_t write_pmsg(struct file *file, const char __user *buf,
>> return ret ? ret : count;
>> }
>>
>> +static ssize_t write_pmsg(struct file *file, const char __user *buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int ret, _ret;
>> + struct pstore_info_list *entry;
>> +
>> + mutex_lock(&psback_lock);
>> + list_for_each_entry(entry, &psback->list_entry, list) {
>> + if (entry->psi->flags & PSTORE_FLAGS_PMSG) {
>> + _ret = do_write_pmsg(file, buf, count,
>> + ppos, entry->psi);
>> + ret = ret > _ret ? ret : _ret;
>
> I'm not sure this "ret" handling is correct. What's your goal with it?
>
> I think we need to report either the max seen _ret, or the first
> negative return value.
>
> (This reminds me of LSM stacking.)
>
> so:
>
> ssize_t err = 0;
> ssize_t written = 0;
> ssize_t ret;
>
> for each {
> ret = do_write_pmsg();
> if (!err && ret < 0)
> err = ret;
> written = max(ret, written);
> }
> ...
> return err ? err : written;
>
> So we'll return an error if we hit one, otherwise the longest scan of
> written bytes.
>
> This does run the risk of breaking backends that will split up writes,
> but I don't think that's a problem in reality yet.
>
> For example, if "count" was 100, and a backend wrote 10, pstore_write()
> will return 10, and userspace will turn around and try to write the next
> 90 bytes. If we want to support it, we'll need to do the retry within
> the above code, but I think that's overkill currently. Maybe just leave
> a comment about it for now.
>
Our goal was to return the max seen _ret but missed the situation that
it would fail to return a negative value.
>> + }
>> + }
>> + mutex_unlock(&psback_lock);
>> +
>> + return ret;
>> +}
>> +
>> static const struct file_operations pmsg_fops = {
>> .owner = THIS_MODULE,
>> .llseek = noop_llseek,
>> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
>> index 638507a3c8ff..0d2be20c8929 100644
>> --- a/include/linux/pstore.h
>> +++ b/include/linux/pstore.h
>> @@ -201,6 +201,35 @@ struct pstore_info {
>> int (*erase)(struct pstore_record *record);
>> };
>>
>> +/* Supported multibackends */
>> +#define PSTORE_MAX_BACKEND_LENGTH 100
>> +#define PSTORE_BACKEND_NUM 16
>> +
>> +#define PSTORE_LIST_FULL (BIT(PSTORE_BACKEND_NUM) - 1)
>> +#define PSOTRE_LIST_EMPTY 0
>> +
>> +extern struct mutex psback_lock;
>> +
>> +struct pstore_info_list {
>> + struct pstore_info *psi;
>> + struct list_head list;
>> + int index;
>> +};
>> +
>> +/**
>> + * struct pstore_backends - management of pstore backends
>> + * @list_entry: entry of pstore backend driver information list
>> + * @front_cnt: count of each enabled frontend
>> + * @flag: bitmap of enabled pstore backend
>> + *
>> + */
>> +
>> +struct pstore_backends {
>> + struct list_head list_entry;
>> + int front_cnt[PSTORE_TYPE_MAX];
>> + u16 flag;
>> +};
>
> I think all of this can just live in pstore_info_list, yes?
>
Sorry but I don't quite understand what 'all of this' stands for.
After big_oops_buf moved into pstore_info_list, those PSTORE_XXX
are not needed anymore. And index in pstore_info_list is removed,
big_oops_buf, pstore_dumper are added(and maybe max_compressed_size).
flag in pstore_backends is removed. Now pstore_info_list holds
per-backend variable and pstore_backends holds the backend list.
Then what could be live in pstore_info_list?
>> +
>> /* Supported frontends */
>> #define PSTORE_FLAGS_DMESG BIT(0)
>> #define PSTORE_FLAGS_CONSOLE BIT(1)
>> --
>> 2.39.3
>>
>
> This is continuing to look good! Thanks for the work. :)
>
I should be the one to appreciate your review,
thank you so much and we do see it is getting better. :)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] tools/testing: adjust pstore backend related selftest
2024-02-07 12:53 ` Kees Cook
@ 2024-02-20 12:31 ` Yuanhe Shu
0 siblings, 0 replies; 13+ messages in thread
From: Yuanhe Shu @ 2024-02-20 12:31 UTC (permalink / raw)
To: Kees Cook
Cc: tony.luck, gpiccoli, shuah, corbet, xlpang, linux-kernel,
linux-doc, linux-hardening, linux-kselftest, xiangzao
On 2024/2/7 20:53, Kees Cook wrote:
> On Wed, Feb 07, 2024 at 10:19:21AM +0800, Yuanhe Shu wrote:
>> Pstore now supports multiple backends, the module parameter
>> pstore.backend varies from 'registered backend' to 'backends that are
>> allowed to register'. Adjust selftests to match the change.
>>
>> Signed-off-by: Yuanhe Shu <xiangzao@linux.alibaba.com>
>> ---
>> tools/testing/selftests/pstore/common_tests | 8 +--
>> .../selftests/pstore/pstore_post_reboot_tests | 65 ++++++++++---------
>> tools/testing/selftests/pstore/pstore_tests | 2 +-
>> 3 files changed, 38 insertions(+), 37 deletions(-)
>>
>> diff --git a/tools/testing/selftests/pstore/common_tests b/tools/testing/selftests/pstore/common_tests
>> index 4509f0cc9c91..497e6fc3215f 100755
>> --- a/tools/testing/selftests/pstore/common_tests
>> +++ b/tools/testing/selftests/pstore/common_tests
>> @@ -27,9 +27,9 @@ show_result() { # result_value
>> }
>>
>> check_files_exist() { # type of pstorefs file
>> - if [ -e ${1}-${backend}-0 ]; then
>> + if [ -e ${1}-${2}-0 ]; then
>> prlog "ok"
>> - for f in `ls ${1}-${backend}-*`; do
>> + for f in `ls ${1}-${2}-*`; do
>> prlog -e "\t${f}"
>> done
>> else
>> @@ -74,9 +74,9 @@ prlog "=== Pstore unit tests (`basename $0`) ==="
>> prlog "UUID="$UUID
>>
>> prlog -n "Checking pstore backend is registered ... "
>> -backend=`cat /sys/module/pstore/parameters/backend`
>> +backends=$(dmesg | sed -n 's/.*pstore: Registered \(.*\) as persistent store backend.*/\1/p')
>> show_result $?
>> -prlog -e "\tbackend=${backend}"
>> +prlog -e "\tbackends="$backends
>
> Missing trailing "? Also, doesn't this end up printing multiple lines?
> Perhaps, like LSM stacking, we need a /sys/module entry for the list of
> backends, comma separated?
>
To avoid printing multiple lines here we move $backends out of "" then
it will print one single line backend names seperated by white space.
Yes, I also referred to LSM stacking and wondering if we need a module
parameter to indicate which backends are registered at present. It would
be nice for users to know which pstore backends are registered and
selftest could take it for test easily. But I am worried about it would
be confusing for users that there is a parameter pstore.backend to
indicate which backends are allowed to be registered and another
parameter to indicate which backends are registered now. At first the
naming is a question. What is your advice?
>> prlog -e "\tcmdline=`cat /proc/cmdline`"
>> if [ $rc -ne 0 ]; then
>> exit 1
>> diff --git a/tools/testing/selftests/pstore/pstore_post_reboot_tests b/tools/testing/selftests/pstore/pstore_post_reboot_tests
>> index d6da5e86efbf..9e40ccb9c918 100755
>> --- a/tools/testing/selftests/pstore/pstore_post_reboot_tests
>> +++ b/tools/testing/selftests/pstore/pstore_post_reboot_tests
>> @@ -36,45 +36,46 @@ else
>> fi
>>
>> cd ${mount_point}
>> +for backend in ${backends}; do
>> + prlog -n "Checking ${backend}-dmesg files exist in pstore filesystem ... "
>> + check_files_exist dmesg ${backend}
>>
>> -prlog -n "Checking dmesg files exist in pstore filesystem ... "
>> -check_files_exist dmesg
>> + prlog -n "Checking ${backend}-console files exist in pstore filesystem ... "
>> + check_files_exist console ${backend}
>>
>> -prlog -n "Checking console files exist in pstore filesystem ... "
>> -check_files_exist console
>> + prlog -n "Checking ${backend}-pmsg files exist in pstore filesystem ... "
>> + check_files_exist pmsg ${backend}
>>
>> -prlog -n "Checking pmsg files exist in pstore filesystem ... "
>> -check_files_exist pmsg
>> + prlog -n "Checking ${backend}-dmesg files contain oops end marker"
>> + grep_end_trace() {
>> + grep -q "\---\[ end trace" $1
>> + }
>> + files=`ls dmesg-${backend}-*`
>> + operate_files $? "$files" grep_end_trace
>>
>> -prlog -n "Checking dmesg files contain oops end marker"
>> -grep_end_trace() {
>> - grep -q "\---\[ end trace" $1
>> -}
>> -files=`ls dmesg-${backend}-*`
>> -operate_files $? "$files" grep_end_trace
>> + prlog -n "Checking ${backend}-console file contains oops end marker ... "
>> + grep -q "\---\[ end trace" console-${backend}-0
>> + show_result $?
>>
>> -prlog -n "Checking console file contains oops end marker ... "
>> -grep -q "\---\[ end trace" console-${backend}-0
>> -show_result $?
>> -
>> -prlog -n "Checking pmsg file properly keeps the content written before crash ... "
>> -prev_uuid=`cat $TOP_DIR/prev_uuid`
>> -if [ $? -eq 0 ]; then
>> - nr_matched=`grep -c "$TEST_STRING_PATTERN" pmsg-${backend}-0`
>> - if [ $nr_matched -eq 1 ]; then
>> - grep -q "$TEST_STRING_PATTERN"$prev_uuid pmsg-${backend}-0
>> - show_result $?
>> + prlog -n "Checking ${backend}-pmsg file properly keeps the content written before crash ... "
>> + prev_uuid=`cat $TOP_DIR/prev_uuid`
>> + if [ $? -eq 0 ]; then
>> + nr_matched=`grep -c "$TEST_STRING_PATTERN" pmsg-${backend}-0`
>> + if [ $nr_matched -eq 1 ]; then
>> + grep -q "$TEST_STRING_PATTERN"$prev_uuid pmsg-${backend}-0
>> + show_result $?
>> + else
>> + prlog "FAIL"
>> + rc=1
>> + fi
>> else
>> - prlog "FAIL"
>> - rc=1
>> + prlog "FAIL"
>> + rc=1
>> fi
>> -else
>> - prlog "FAIL"
>> - rc=1
>> -fi
>>
>> -prlog -n "Removing all files in pstore filesystem "
>> -files=`ls *-${backend}-*`
>> -operate_files $? "$files" rm
>> + prlog -n "Removing all ${backend} files in pstore filesystem "
>> + files=`ls *-${backend}-*`
>> + operate_files $? "$files" rm
>> +done
>>
>> exit $rc
>> diff --git a/tools/testing/selftests/pstore/pstore_tests b/tools/testing/selftests/pstore/pstore_tests
>> index 2aa9a3852a84..f4665a8c77dc 100755
>> --- a/tools/testing/selftests/pstore/pstore_tests
>> +++ b/tools/testing/selftests/pstore/pstore_tests
>> @@ -10,7 +10,7 @@
>> . ./common_tests
>>
>> prlog -n "Checking pstore console is registered ... "
>> -dmesg | grep -Eq "console \[(pstore|${backend})"
>> +dmesg | grep -Eq "console \[(pstore console)"
>> show_result $?
>>
>> prlog -n "Checking /dev/pmsg0 exists ... "
>> --
>> 2.39.3
>>
>
> Otherwise seems ok
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-02-20 12:31 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-07 2:19 [PATCH v3 0/3] pstore: add multi-backend support Yuanhe Shu
2024-02-07 2:19 ` [PATCH 1/3] " Yuanhe Shu
2024-02-07 12:48 ` Kees Cook
2024-02-20 12:19 ` Yuanhe Shu
2024-02-07 2:19 ` [PATCH 2/3] Documentation: adjust pstore backend related document Yuanhe Shu
2024-02-07 12:51 ` Kees Cook
2024-02-07 2:19 ` [PATCH 3/3] tools/testing: adjust pstore backend related selftest Yuanhe Shu
2024-02-07 12:53 ` Kees Cook
2024-02-20 12:31 ` Yuanhe Shu
-- strict thread matches above, loose matches on Subject: below --
2024-02-05 12:28 [PATCH v2 0/3] pstore: add multi-backend support Yuanhe Shu
2024-02-05 12:28 ` [PATCH 1/3] " Yuanhe Shu
2024-02-06 8:05 ` kernel test robot
2024-02-06 8:41 ` Yuanhe Shu
2024-02-06 12:17 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).