From: Christoph Hellwig <firstname.lastname@example.org> To: Kees Cook <email@example.com> Cc: Christoph Hellwig <firstname.lastname@example.org>, WeiXiong Liao <email@example.com>, firstname.lastname@example.org, Richard Weinberger <email@example.com>, firstname.lastname@example.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.
prev parent 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 \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ /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 \ email@example.com 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