On Tue, Jun 15, 2021 at 02:31:18PM +0200, Christoph Hellwig wrote:

quoted

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

quoted

 /*
  * 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.

quoted

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

quoted

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

quoted

+	if (!psblk_file->f_mapping)
+		pr_err("missing f_mapping\n");

Can't ever be true.

quoted

+	else if (!psblk_file->f_mapping->host)
+		pr_err("missing host\n");

Can't ever be true either.

quoted

+	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