Re: [PATCH] pstore/blk: Use the normal block device I/O path

[Kees Cook <keescook@chromium.org>]

On Tue, Jun 15, 2021 at 02:31:18PM +0200, Christoph Hellwig wrote:
> > -	if (!dev || !dev->total_size || !dev->read || !dev->write)
> > +	if (!dev || !dev->total_size || !dev->read || !dev->write) {
> > +		if (!dev)
> > +			pr_err("NULL device info\n");
> > +		else {
> > +			if (!dev->total_size)
> > +				pr_err("zero sized device\n");
> > +			if (!dev->read)
> > +				pr_err("no read handler for device\n");
> > +			if (!dev->write)
> > +				pr_err("no write handler for device\n");
> > +		}
> >  		return -EINVAL;
> > +	}
> 
> This is completely unrelated and should be a separate patch.  And it
> also looks rather strange, I'd at very least split the dev check out
> and return early without the weird compound statement, but would probably
> handle each one separate.  All assuming that we really need all these
> debug printks.

Agreed -- I've moved it to a separate patch.

> >  /*
> >   * This takes its configuration only from the module parameters now.
> >   */
> >  static int __register_pstore_blk(void)
> 
> This needs a __init annotation now.

Ah yes, good point. I've rearranged things to avoid this.

> >  {
> > +	struct pstore_device_info dev = {
> > +		.read = psblk_generic_blk_read,
> > +		.write = psblk_generic_blk_write,
> > +	};
> 
> On-stack method tables are a little odd..

struct pstore_device_info is mainly an argument passing structure, not
an ops structure. There is some weird over-engineering here, which I'll
fix up in a follow-up patch.

> > +	if (!__is_defined(MODULE)) {
> 
> This looks a little weird.  Can we define a rapper for this in config.h
> that is a little more self-explanatory, e.g. in_module()?

I've adjusted this.

> 
> > +	if (!psblk_file->f_mapping)
> > +		pr_err("missing f_mapping\n");
> 
> Can't ever be true.
> 
> > +	else if (!psblk_file->f_mapping->host)
> > +		pr_err("missing host\n");
> 
> Can't ever be true either.
> 
> > +	else if (!I_BDEV(psblk_file->f_mapping->host))
> > +		pr_err("missing I_BDEV\n");
> > +	else if (!I_BDEV(psblk_file->f_mapping->host)->bd_inode)
> > +		pr_err("missing bd_inode\n");
> 
> І_BDEV just does pointer arithmetics, so it can't ever return NULL.
> And there are no block device inodes without bd_inode either.  And
> all of this is per definition present for open S_ISBLK inodes.

Okay, good. There were a lot of dead-ends in here, and after the removal
of i_bdev, it wasn't obvious which things were going to exist. I've
removed all these checks.

One thing I noticed it that it seems we're missing a global helper for
"->f_mapping->host", which I see repeated a lot. (There is a local
helper bdev_file_inode().)

$ git grep '\bf_mapping->host\b' | wc -l
149


Thanks for the review! I'll send a v2 series.

-- 
Kees Cook