Re: [PATCH v2 03/11] pstore/blk: blkoops: support pmsg recorder

[Kees Cook <keescook@xxxxxxxxxxxx>]

On Sun, Mar 22, 2020 at 07:14:37PM +0800, WeiXiong Liao wrote:
> hi Kees Cook,
> 
> On 2020/3/19 AM 2:13, Kees Cook wrote:
> > On Fri, Feb 07, 2020 at 08:25:47PM +0800, WeiXiong Liao wrote:
> >> +config PSTORE_BLKOOPS_PMSG_SIZE
> >> +	int "pmsg size in kbytes for blkoops"
> >> +	depends on PSTORE_BLKOOPS
> >> +	depends on PSTORE_PMSG
> >> +	default 64
> > 
> > Instead of "depends on PSTORE_PMSG", you can do:
> > 
> > 	default 64 if PSTORE_PMSG
> > 	default 0
> > 
> 
> What happens if PSTORE_BLKOOPS_PMSG_SIZE is non-zero while
> PSTORE_PMSG is disabled? The pmsg recorder do not work but pstore/blk
> will always allocate zone for pmsg recorder since pmsg_size is non-zero.
> It waste storage space.

Yeah, true. This gets back to my wanting to have this be more dynamic in
the pstore core. But, yes, for now, you're right.

You can still do this for initialization:

static long pmsg_size = IS_ENABLED(CONFIG_PSTORE_PMSG)
				?  CONFIG_PSTORE_BLKOOPS_PMSG_SIZE
				: -1;

But that'll require logic changes to verify_size(). We can revisit this
after v3.

> >> @@ -611,7 +776,8 @@ static ssize_t blkz_dmesg_read(struct blkz_zone *zone,
> >>  		char *buf = kasprintf(GFP_KERNEL,
> >>  				"%s: Total %d times\n",
> >>  				record->reason == KMSG_DUMP_OOPS ? "Oops" :
> >> -				"Panic", record->count);
> >> +				record->reason == KMSG_DUMP_PANIC ? "Panic" :
> >> +				"Unknown", record->count);
> > 
> > Please use get_reason_str() here.
> > 
> 
> get_reason_str() is marked 'static' on platform.c and pstore/blk only
> support oops
> and panic, it's no need to check more reason number.

I'd still rather identical strings not be scattered around pstore. :) Go
ahead and make get_reason_str() non-static and rename it
pstore_get_reason_str(), EXPORT_SYMBOL(), add to pstore.h etc.

-- 
Kees Cook

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/