[PATCH] mark pstore-blk as broken

[PATCH] mark pstore-blk as broken

[Christoph Hellwig]

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

Re: [PATCH] mark pstore-blk as broken

[Kees Cook]

On Tue, Jun 08, 2021 at 06:13:27PM +0200, Christoph Hellwig wrote:
> 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
> 

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

-- 
Kees Cook

Re: [PATCH] mark pstore-blk as broken

[Al Viro]

On Tue, Jun 08, 2021 at 10:34:29AM -0700, Kees Cook wrote:
> > 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"...

Re: [PATCH] mark pstore-blk as broken

[Christoph Hellwig]

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.

Re: [PATCH] mark pstore-blk as broken

[Al Viro]

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