| Message ID | 20210608161327.1537919-1-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mark pstore-blk as broken | expand |
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/
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"...
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: > > 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.
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
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(+)