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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210615123118.GA14239@lst.de>
Date:   Tue, 15 Jun 2021 14:31:18 +0200
From:   Christoph Hellwig <hch@....de>
To:     Kees Cook <keescook@...omium.org>
Cc:     Christoph Hellwig <hch@....de>, Al Viro <viro@...iv.linux.org.uk>,
        WeiXiong Liao <gmpy.liaowx@...il.com>, axboe@...nel.dk,
        Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Tony Luck <tony.luck@...el.com>, linux-kernel@...r.kernel.org,
        linux-block@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] pstore/blk: Use the normal block device I/O path

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

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

This needs a __init annotation now.

>
>
>  {
> +	struct pstore_device_info dev = {
> +		.read = psblk_generic_blk_read,
> +		.write = psblk_generic_blk_write,
> +	};

On-stack method tables are a little odd..

> +	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()?

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