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
