Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 2 Dec 2020 11:32:12 -0800
From:   Kees Cook <keescook@chromium.org>
To:     Bhaskara Budiredla <bbudiredla@marvell.com>
Cc:     "ulf.hansson@linaro.org" <ulf.hansson@linaro.org>,
        "ccross@android.com" <ccross@android.com>,
        "tony.luck@intel.com" <tony.luck@intel.com>,
        Sunil Kovvuri Goutham <sgoutham@marvell.com>,
        "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "outgoing2/0000-cover-letter.patch@mx0b-0016f401.pphosted.com" 
        <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
Message-ID: <202012021126.20ACBBD@keescook>
References: <20201123111925.28999-1-bbudiredla@marvell.com>
 <202012011218.3B6566C5@keescook>
 <CY4PR1801MB207097391BD8DE37A0F49102DEF30@CY4PR1801MB2070.namprd18.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CY4PR1801MB207097391BD8DE37A0F49102DEF30@CY4PR1801MB2070.namprd18.prod.outlook.com>
Precedence: bulk

On Wed, Dec 02, 2020 at 06:36:21AM +0000, Bhaskara Budiredla wrote:
> >From: Kees Cook <keescook@chromium.org>
> >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:

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.

> >> +	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.)

-- 
Kees Cook