linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mark pstore-blk as broken
@ 2021-06-08 16:13 Christoph Hellwig
  2021-06-08 17:34 ` Kees Cook
  0 siblings, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2021-06-08 16:13 UTC (permalink / raw)
  To: axboe
  Cc: keescook, anton, ccross, tony.luck, gmpy.liaowx, linux-block,
	linux-fsdevel, linux-kernel

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] mark pstore-blk as broken
  2021-06-08 16:13 [PATCH] mark pstore-blk as broken Christoph Hellwig
@ 2021-06-08 17:34 ` Kees Cook
  2021-06-14  1:17   ` Al Viro
  2021-06-14  7:07   ` Christoph Hellwig
  0 siblings, 2 replies; 5+ messages in thread
From: Kees Cook @ 2021-06-08 17:34 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, anton, ccross, tony.luck, gmpy.liaowx, linux-block,
	linux-fsdevel, linux-kernel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] mark pstore-blk as broken
  2021-06-08 17:34 ` Kees Cook
@ 2021-06-14  1:17   ` Al Viro
  2021-06-14  7:07   ` Christoph Hellwig
  1 sibling, 0 replies; 5+ messages in thread
From: Al Viro @ 2021-06-14  1:17 UTC (permalink / raw)
  To: Kees Cook
  Cc: Christoph Hellwig, axboe, anton, ccross, tony.luck, gmpy.liaowx,
	linux-block, linux-fsdevel, linux-kernel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] mark pstore-blk as broken
  2021-06-08 17:34 ` Kees Cook
  2021-06-14  1:17   ` Al Viro
@ 2021-06-14  7:07   ` Christoph Hellwig
  2021-06-14 17:33     ` Al Viro
  1 sibling, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2021-06-14  7:07 UTC (permalink / raw)
  To: Kees Cook
  Cc: Christoph Hellwig, axboe, anton, ccross, tony.luck, gmpy.liaowx,
	linux-block, linux-fsdevel, linux-kernel

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] mark pstore-blk as broken
  2021-06-14  7:07   ` Christoph Hellwig
@ 2021-06-14 17:33     ` Al Viro
  0 siblings, 0 replies; 5+ messages in thread
From: Al Viro @ 2021-06-14 17:33 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Kees Cook, axboe, anton, ccross, tony.luck, gmpy.liaowx,
	linux-block, linux-fsdevel, linux-kernel

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-06-14 17:33 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-08 16:13 [PATCH] mark pstore-blk as broken Christoph Hellwig
2021-06-08 17:34 ` Kees Cook
2021-06-14  1:17   ` Al Viro
2021-06-14  7:07   ` Christoph Hellwig
2021-06-14 17:33     ` Al Viro

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).