Date: Tue, 20 Feb 2024 20:19:28 +0800
From: Yuanhe Shu <xiangzao@...ux.alibaba.com>
To: Kees Cook <keescook@...omium.org>
Cc: tony.luck@...el.com, gpiccoli@...lia.com, shuah@...nel.org,
 corbet@....net, xlpang@...ux.alibaba.com, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org, linux-hardening@...r.kernel.org,
 linux-kselftest@...r.kernel.org, Xingrui Yi <yixingrui@...ux.alibaba.com>,
 xiangzao@...ux.alibaba.com
Subject: Re: [PATCH 1/3] pstore: add multi-backend support



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@...ux.alibaba.com>
>> Signed-off-by: Yuanhe Shu <xiangzao@...ux.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. :)

Powered by blists - more mailing lists