Messages in this thread | | | Date | Tue, 1 Dec 2020 11:52:09 -0800 | From | Kees Cook <> | Subject | Re: [PATCH 8/9] pstore/blk: use the normal block device I/O path |
| |
On Fri, Oct 16, 2020 at 03:20:46PM +0200, Christoph Hellwig wrote: > Stop poking into block layer internals and just open the block device > file an use kernel_read and kernel_write on it. Note that this means > the transformation from name_to_dev_t can't be used anymore, and proper > block device file names must be used. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > [...] > + > +#ifdef MODULE > +static int __init pstore_blk_init(void) > +{ > + return __pstore_blk_init(blkdev); > +} > late_initcall(pstore_blk_init); > > static void __exit pstore_blk_exit(void) > { > - if (!psblk_bdev) > + if (!psblk_file) > return; > unregister_pstore_device(&pstore_blk_zone_ops); > - blkdev_put(psblk_bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL); > + fput(psblk_file); > } > module_exit(pstore_blk_exit); > +#else /* MODULE */ > +/* > + * During early boot the real root file system hasn't been mounted yet, > + * and not device nodes are present yet. Use the same scheme to find > + * the device that we use for mounting the root file system. > + */ > +void __init pstore_blk_early_init(void) > +{ > + const char devname[] = "/dev/pstore-blk"; > + dev_t dev = name_to_dev_t(blkdev); > + > + if (!dev) > + return; > + init_unlink(devname); > + init_mknod(devname, S_IFBLK | 0600, new_encode_dev(dev)); > + __pstore_blk_init(devname); > +} > +#endif /* MODULE */
That the allowed naming conventions change based on _how_ pstore is built seems very wrong to me.
While I do like the clean up to simplify the read/write activities, this seems like totally the wrong approach here.
> > /* get information of pstore/blk */ > int pstore_blk_get_config(struct pstore_blk_config *info) > diff --git a/include/linux/pstore_blk.h b/include/linux/pstore_blk.h > index 0abd412a6cb3e3..7c06b9d6740e2a 100644 > --- a/include/linux/pstore_blk.h > +++ b/include/linux/pstore_blk.h > @@ -39,4 +39,6 @@ struct pstore_blk_config { > */ > int pstore_blk_get_config(struct pstore_blk_config *info); > > +void __init pstore_blk_early_init(void); > + > #endif > diff --git a/init/main.c b/init/main.c > index 1af84337cb18d5..058cce64f70390 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -98,6 +98,7 @@ > #include <linux/mem_encrypt.h> > #include <linux/kcsan.h> > #include <linux/init_syscalls.h> > +#include <linux/pstore_blk.h> > > #include <asm/io.h> > #include <asm/bugs.h> > @@ -1524,6 +1525,9 @@ static noinline void __init kernel_init_freeable(void) > prepare_namespace(); > } > > + if (IS_BUILTIN(CONFIG_PSTORE_BLK)) > + pstore_blk_early_init(); > +
I hate this being a special-case in kernel_init. For ramoops, we use:
postcore_initcall(ramoops_init);
which is much better than open coding this here.
> /* > * Ok, we have completed the initial bootup, and > * we're essentially up and running. Get rid of the > -- > 2.28.0 >
-- Kees Cook
| |