RE: [EXT] Re: [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk

[Bhaskara Budiredla]

>-----Original Message-----
>From: Kees Cook <keescook@xxxxxxxxxxxx>
>Sent: Thursday, December 3, 2020 1:02 AM
>To: Bhaskara Budiredla <bbudiredla@xxxxxxxxxxx>
>Cc: ulf.hansson@xxxxxxxxxx; ccross@xxxxxxxxxxx; tony.luck@xxxxxxxxx; Sunil
>Kovvuri Goutham <sgoutham@xxxxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx;
>linux-kernel@xxxxxxxxxxxxxxx; outgoing2/0000-cover-letter.patch@mx0b-
>0016f401.pphosted.com
>Subject: Re: [EXT] Re: [PATCH v2 1/2] mmc: Support kmsg dumper based on
>pstore/blk
>
>On Wed, Dec 02, 2020 at 06:36:21AM +0000, Bhaskara Budiredla wrote:
>> >From: Kees Cook <keescook@xxxxxxxxxxxx> On Mon, Nov 23, 2020 at
>> >04:49:24PM +0530, Bhaskara Budiredla wrote:
>> >Why isn't this just written as:
>> >
>> >config MMC_PSTORE
>> >	bool "Log panic/oops to a MMC buffer"
>> >	depends on MMC_BLOCK
>> >	select PSTORE_BLK
>> >	help
>> >	  This option will let you create platform backend to store kmsg
>> >	  crash dumps to a user specified MMC device. This is primarily
>> >	  based on pstore/blk.
>> >
>>
>> The idea was to compile MMC_PSTORE as part of MMC_BLOCK driver,
>> provided it is optionally enabled.
>> The above arrangement compiles MMC_PSTORE as module: if
>> (CONFIG_MMC_PSTORE_BACKEND == y && CONFIG_MMC_BLOCK == m)
>> as static:     if (CONFIG_MMC_PSTORE_BACKEND == y &&
>CONFIG_MMC_BLOCK == y)
>
>Ah, okay. If it's a tri-state, wouldn't it track CONFIG_MMC_BLOCK's state? As
>in, does this work:
>

Yes, it's a tri-state but not compiled as a separate driver. 

>config MMC_PSTORE
>	tristate "Log panic/oops to a MMC buffer"
>	depends on MMC_BLOCK
>	select PSTORE_BLK
>	help
>	  This option will let you create platform backend to store kmsg
>	  crash dumps to a user specified MMC device. This is primarily
>	  based on pstore/blk.
>

No, this will cause problems for MMC_PSTORE=m and MMC_BLOCK=y
MMC_PSTORE automatically have to be selected as module or static
based on MMC_BLOCK selection. There were couple of function calls
from the code in MMC_BLOCK to MMC_PSTORE. Uffe prefers them
to be unconditional calls (as per discussion in v1).  

>> >> +	if (strncmp(cxt->dev_name, disk_name, strlen(disk_name)))
>> >> +		return;
>> >
>> >Why isn't this just strcmp()?
>>
>> The mmc disk name (disk_name) doesn't include the partition number.
>> strncmp is restricted to something like /dev/mmcblk0, it doesn't cover full
>/dev/mmcblk0pn.
>> The partition number check is carried out in the next statement.
>
>Okay, gotcha; thanks!
>
>> >> +	dev->flags = PSTORE_FLAGS_DMESG;
>> >
>> >Can't this support more than just DMESG? I don't see anything specific to
>that.
>> >This is using pstore/zone ultimately, which can support whatever
>> >frontends it needs to.
>>
>> Yes, as of now the support is only for DMESG. We will extend this to
>> other frontends on need basis.
>
>Okay -- I assume this has mostly to do with not having erasure (below).
>
>> >> +	dev->erase = NULL;
>> >
>> >No way to remove the records?
>>
>> Yes, at this time, no removal of records.
>
>Okay. (I think this might be worth mentioning in docs somewhere.)
>

Would it be sufficient to add corresponding notes to the commit message? 

>--
>Kees Cook