[PATCH] mark pstore-blk as broken

pstore-blk just pokes directly into the pagecache for the block
device without going through the file operations for that by faking
up it's own file operations that do not match the block device ones.

As this breaks the control of the block layer of it's page cache,
and even now just works by accident only the best thing is to just
disable this driver.

Fixes: 17639f67c1d6 ("pstore/blk: Introduce backend for block devices")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/pstore/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
index 8adabde685f1..328da35da390 100644
--- a/fs/pstore/Kconfig
+++ b/fs/pstore/Kconfig

@@ -173,6 +173,7 @@ config PSTORE_BLK
 	tristate "Log panic/oops to a block device"
 	depends on PSTORE
 	depends on BLOCK
+	depends on BROKEN
 	select PSTORE_ZONE
 	default n
 	help

-- 
2.30.2

On Tue, Jun 08, 2021 at 06:13:27PM +0200, Christoph Hellwig wrote:

NAK, please answer my concerns about your patches instead:
https://lore.kernel.org/lkml/202012011149.5650B9796@keescook/

-- 
Kees Cook

On Tue, Jun 08, 2021 at 10:34:29AM -0700, Kees Cook wrote:

quoted

diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
index 8adabde685f1..328da35da390 100644
--- a/fs/pstore/Kconfig
+++ b/fs/pstore/Kconfig

@@ -173,6 +173,7 @@ config PSTORE_BLK
 	tristate "Log panic/oops to a block device"
 	depends on PSTORE
 	depends on BLOCK
+	depends on BROKEN
 	select PSTORE_ZONE
 	default n
 	help

-- 
2.30.2

NAK, please answer my concerns about your patches instead:
https://lore.kernel.org/lkml/202012011149.5650B9796@keescook/

	How about concerns about the code in question having gotten
into the kernel in the first place?  Quality aside (that's a separate
conversation, probably for tomorrow), just what happens if that thing
is triggered by the code that happens to hold a page on block device
locked?  AFAICS, __generic_file_write_iter() will cheerfully deadlock...

	Kees, may I ask you where had that thing been discussed back
then?  All I can see is linux-kernel, and that's "archived by", not
"discussed on"...

On Tue, Jun 08, 2021 at 10:34:29AM -0700, Kees Cook wrote:

NAK, please answer my concerns about your patches instead:
https://lore.kernel.org/lkml/202012011149.5650B9796@keescook/

No.  This code pokes into block layer internals with all kinds of issues
and without any signoff from the relevant parties.  We just can't keep it
around.

On Mon, Jun 14, 2021 at 09:07:12AM +0200, Christoph Hellwig wrote:

On Tue, Jun 08, 2021 at 10:34:29AM -0700, Kees Cook wrote:

quoted

NAK, please answer my concerns about your patches instead:
https://lore.kernel.org/lkml/202012011149.5650B9796@keescook/

No.  This code pokes into block layer internals with all kinds of issues
and without any signoff from the relevant parties.  We just can't keep it
around.

There's a much more interesting question about that code: seeing that
psblk_generic_blk_write() contains this
        /* Console/Ftrace backend may handle buffer until flush dirty zones */
	if (in_interrupt() || irqs_disabled())
		return -EBUSY;
just what are the locking conditions guaranteed to that thing?
Because if it's ever called with one of the destination pages
held locked by the caller, we are fucked.  It won't get caught
by that test.

That really should've been discussed back when the entire thing
got merged; at the absolute least we need the locking environment
documented.