LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Christoph Hellwig <hch@lst.de>
To: Kees Cook <keescook@chromium.org>
Cc: Christoph Hellwig <hch@lst.de>,
	WeiXiong Liao <gmpy.liaowx@gmail.com>,
	linux-kernel@vger.kernel.org, Richard Weinberger <richard@nod.at>,
	linux-mtd@lists.infradead.org
Subject: Re: use case for register_pstore_blk?
Date: Mon, 12 Oct 2020 09:02:33 +0200
Message-ID: <20201012070233.GA2770@lst.de> (raw)
In-Reply-To: <202010071147.F6E57A32@keescook>

On Wed, Oct 07, 2020 at 12:17:25PM -0700, Kees Cook wrote:
> Do you mean psblk_generic_blk_read() and psblk_generic_blk_write()?
> These are for writing to the block device... I'm happy to adjust this
> if you can show me the better API. (This was being developed in the
> middle of the iov_iter changes, so perhaps I missed a more appropriate
> way to do things.)

The problem is that you use library function for file systems, and
then call them on an instance you don't own, and you're feeding crap
into them like your own methods.

Now given that as far as I can tell you require 4k aligned reads and
writes anyway, there doesn't seem to be any need for this whole page
cache dance to start with, and you could just do the completely trivial
bios submission.  But if for some reason you need to use the page cache,
that needs to be done through fs/block_dev.c APIs instead of through
the side.

> 
> > and it uses name_to_dev_t which must not be used in new code.
> 
> What?
> 
> include/linux/mount.h:
> extern dev_t name_to_dev_t(const char *name);

It used to be a private API, but then the Chromium folks just exported
it in e6e20a7a5f3f49bfee518d5c6849107398d83912 without consulting any
relevant maintainer.  And now we have this mess :(

> Where did this happen, where was it documented, and what should be used
> instead?

Use blkdev_get_by_path only.  If you look at blkdev_get_by_dev it has
this very explicit comment:

 * Use it ONLY if you really do not have anything better - i.e. when
 * you are behind a truly sucky interface and all you are given is a
 * device number.  _Never_ to be used for internal purposes.  If you
 * ever need it - reconsider your API.


> > It also does not happen to share code with the mtd case.
> 
> What? Yes it does: it explicitly uses the pstore/blk configuration
> callback to get the details configured at boot to identify and configure
> the backing device. This is specifically designed this way to avoid
> repeating the mistake of having per-backing-device configuration that is
> essentially only actually used by the pstore storage layer. i.e. the very
> thing I'm trying to get away from in ramoops, efi-pstore, etc: storage
> configuration is tied to the pstore storage layer (i.e. pstore/blk and
> pstore/zone), not the specific backing device (i.e. MTD, blk, RAM, NVRAM,
> EFI variables, etc).

Sharing the config is trivial.  But it shared nothing in the actual
I/O path, which is entirely different for mtd vs block.  And the sad part
is that with fs/pstore/zone.c you have added the right abstraction, one
that totally makes sense.  But only when used by the block and mtd drivers
directly.  Adding another pointless layer of obsfucation and dead code
that doesn't share anything does not help.

      reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-06 15:52 Christoph Hellwig
2020-10-07  7:13 ` Kees Cook
2020-10-07  7:37   ` Christoph Hellwig
2020-10-07  7:55   ` Christoph Hellwig
2020-10-07  8:37     ` Christoph Hellwig
2020-10-07 18:40       ` Kees Cook
2020-10-07 18:42         ` Christoph Hellwig
2020-10-07 19:17           ` Kees Cook
2020-10-12  7:02             ` Christoph Hellwig [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201012070233.GA2770@lst.de \
    --to=hch@lst.de \
    --cc=gmpy.liaowx@gmail.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=richard@nod.at \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git