Message-ID: <20190520120142.00004c6b.zbestahu@gmail.com>
Date:   Mon, 20 May 2019 12:01:42 +0800
From:   Yue Hu <zbestahu@...il.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Tony Luck <tony.luck@...el.com>,
        LKML <linux-kernel@...r.kernel.org>, Yue Hu <huyue2@...ong.com>
Subject: Re: [PATCH v2] pstore: Add boot loader log messages support

On Wed, 15 May 2019 15:04:09 -0700
Kees Cook <keescook@...omium.org> wrote:

> Hi!
> 
> Thanks for the reminder to review this code. :) Sorry for the delay!
> 
> On Thu, Feb 14, 2019 at 11:49 PM Yue Hu <zbestahu@...il.com> wrote:
> >
> > From: Yue Hu <huyue2@...ong.com>
> >
> > Sometimes we hope to check boot loader log messages (e.g. Android
> > Verified Boot status) when kernel is coming up. Generally it does
> > depend on serial device, but it will be removed for the hardware
> > shipping to market by most of manufacturers. In that case better
> > solder and proper serial cable for different interface (e.g. Type-C
> > or microUSB) are needed. That is inconvenient and even wasting much
> > time on it.  
> 
> Can you give some examples of how this would be used on a real device?
> More notes below...
> 
> >
> > Therefore, let's add a logging support: PSTORE_TYPE_XBL.
> >
> > Signed-off-by: Yue Hu <huyue2@...ong.com>
> > ---
> > v2: mention info of interacting with boot loader
> >
> >  fs/pstore/Kconfig      | 10 +++++++
> >  fs/pstore/platform.c   | 16 ++++++++++
> >  fs/pstore/ram.c        | 81 ++++++++++++++++++++++++++++++++++++++++++++++++--
> >  include/linux/pstore.h | 21 +++++++++----
> >  4 files changed, 121 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
> > index 0d19d19..ef4a2dc 100644
> > --- a/fs/pstore/Kconfig
> > +++ b/fs/pstore/Kconfig
> > @@ -137,6 +137,16 @@ config PSTORE_FTRACE
> >
> >           If unsure, say N.
> >
> > +config PSTORE_XBL
> > +       bool "Log bootloader messages"
> > +       depends on PSTORE
> > +       help
> > +         When the option is enabled, pstore will log boot loader
> > +         messages to /sys/fs/pstore/xbl-ramoops-[ID] after reboot.
> > +         Boot loader needs to support log buffer reserved.
> > +
> > +         If unsure, say N.
> > +
> >  config PSTORE_RAM
> >         tristate "Log panic/oops to a RAM buffer"
> >         depends on PSTORE
> > diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> > index 2d1066e..2e6c3f8f 100644
> > --- a/fs/pstore/platform.c
> > +++ b/fs/pstore/platform.c
> > @@ -65,6 +65,7 @@
> >         "mce",
> >         "console",
> >         "ftrace",
> > +       "xbl",
> >         "rtas",
> >         "powerpc-ofw",
> >         "powerpc-common",
> > @@ -530,6 +531,19 @@ static void pstore_register_console(void) {}
> >  static void pstore_unregister_console(void) {}
> >  #endif
> >
> > +#ifdef CONFIG_PSTORE_XBL
> > +static void pstore_register_xbl(void)
> > +{
> > +       struct pstore_record record;
> > +
> > +       pstore_record_init(&record, psinfo);
> > +       record.type = PSTORE_TYPE_XBL;
> > +       psinfo->write(&record);
> > +}  
> 
> This seems like a very strange way to get the record: this is an
> "empty" write that has a side-effect of reading the XBL region and
> copying it into the prz area. I would expect this to all happen in
> ramoops_pstore_read() instead.
> 
> > +#else
> > +static void pstore_register_xbl(void) {}
> > +#endif
> > +
> >  static int pstore_write_user_compat(struct pstore_record *record,
> >                                     const char __user *buf)
> >  {
> > @@ -616,6 +630,8 @@ int pstore_register(struct pstore_info *psi)
> >                 pstore_register_ftrace();
> >         if (psi->flags & PSTORE_FLAGS_PMSG)
> >                 pstore_register_pmsg();
> > +       if (psi->flags & PSTORE_FLAGS_XBL)
> > +               pstore_register_xbl();
> >
> >         /* Start watching for new records, if desired. */
> >         if (pstore_update_ms >= 0) {
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> > index 1adb5e3..b114b1d 100644
> > --- a/fs/pstore/ram.c
> > +++ b/fs/pstore/ram.c
> > @@ -56,6 +56,27 @@
> >  module_param_named(pmsg_size, ramoops_pmsg_size, ulong, 0400);
> >  MODULE_PARM_DESC(pmsg_size, "size of user space message log");  
> 
> How is the base address of the XBL area specified? It looks currently
> like it's up to the end user to do all the math correctly to line it
> up with where it's expected?
> 
> >
> > +/*
> > + * interact with boot loader
> > + * =========================
> > + *
> > + * xbl memory layout:
> > + * +----+
> > + * |dst |
> > + * |----|     +------------+
> > + * |src ----> |   header   |
> > + * +----+     |log messages|
> > + *            +------------+
> > + *
> > + * As above, src memory is used to store header and log messages generated
> > + * by boot loader, pstore will copy the log messages to dst memory which
> > + * has same size as src. The header in src is to record log messages size
> > + * written and make xbl cookie.  
> 
> Why is such a copy needed? The log is already present in memory; why
> can't pstore just use what's already there?
> 
> > + */
> > +static ulong ramoops_xbl_size = MIN_MEM_SIZE;
> > +module_param_named(xbl_size, ramoops_xbl_size, ulong, 0400);
> > +MODULE_PARM_DESC(xbl_size, "size of boot loader log");
> > +
> >  static unsigned long long mem_address;
> >  module_param_hw(mem_address, ullong, other, 0400);
> >  MODULE_PARM_DESC(mem_address,
> > @@ -88,6 +109,7 @@ struct ramoops_context {
> >         struct persistent_ram_zone *cprz;       /* Console zone */
> >         struct persistent_ram_zone **fprzs;     /* Ftrace zones */
> >         struct persistent_ram_zone *mprz;       /* PMSG zone */
> > +       struct persistent_ram_zone *bprz;       /* XBL zone */
> >         phys_addr_t phys_addr;
> >         unsigned long size;
> >         unsigned int memtype;
> > @@ -95,6 +117,7 @@ struct ramoops_context {
> >         size_t console_size;
> >         size_t ftrace_size;
> >         size_t pmsg_size;
> > +       size_t xbl_size;
> >         int dump_oops;
> >         u32 flags;
> >         struct persistent_ram_ecc_info ecc_info;
> > @@ -106,6 +129,7 @@ struct ramoops_context {
> >         unsigned int max_ftrace_cnt;
> >         unsigned int ftrace_read_cnt;
> >         unsigned int pmsg_read_cnt;
> > +       unsigned int xbl_read_cnt;
> >         struct pstore_info pstore;
> >  };
> >
> > @@ -119,6 +143,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
> >         cxt->console_read_cnt = 0;
> >         cxt->ftrace_read_cnt = 0;
> >         cxt->pmsg_read_cnt = 0;
> > +       cxt->xbl_read_cnt = 0;
> >         return 0;
> >  }
> >
> > @@ -272,6 +297,10 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
> >         if (!prz_ok(prz) && !cxt->pmsg_read_cnt++)
> >                 prz = ramoops_get_next_prz(&cxt->mprz, 0 /* single */, record);
> >
> > +       if (!prz_ok(prz) && !cxt->xbl_read_cnt++) {
> > +               prz = ramoops_get_next_prz(&cxt->bprz, 0 /* single */, record);
> > +       }
> > +
> >         /* ftrace is last since it may want to dynamically allocate memory. */
> >         if (!prz_ok(prz)) {
> >                 if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU) &&
> > @@ -360,6 +389,26 @@ static size_t ramoops_write_kmsg_hdr(struct persistent_ram_zone *prz,
> >         return len;
> >  }
> >
> > +static void ramoops_get_xbl_record(struct persistent_ram_zone *prz,
> > +                                 struct pstore_record *record,
> > +                                 size_t xbl_size)
> > +{
> > +       struct pstore_xbl_header *hdr = NULL;
> > +       size_t half = xbl_size / 2;
> > +       size_t max = half - PSTORE_XBL_HDR_SIZE;
> > +
> > +       hdr = (struct pstore_xbl_header *)((size_t)prz->buffer + half);
> > +
> > +       if (hdr->cookie == PSTORE_XBL_COOKIE) {
> > +               record->buf = (char *)((size_t)hdr + PSTORE_XBL_HDR_SIZE);
> > +               record->size = hdr->size_written;
> > +               if (unlikely(record->size > max))
> > +                       record->size = max;
> > +               return;
> > +       }
> > +       pr_debug("no valid xbl record (cookie = 0x%08x)\n", hdr->cookie);
> > +}
> > +
> >  static int notrace ramoops_pstore_write(struct pstore_record *record)
> >  {
> >         struct ramoops_context *cxt = record->psi->data;
> > @@ -390,6 +439,16 @@ static int notrace ramoops_pstore_write(struct pstore_record *record)
> >         } else if (record->type == PSTORE_TYPE_PMSG) {
> >                 pr_warn_ratelimited("PMSG shouldn't call %s\n", __func__);
> >                 return -EINVAL;
> > +       } else if (record->type == PSTORE_TYPE_XBL) {
> > +               if (!cxt->bprz)
> > +                       return -ENOMEM;
> > +
> > +               ramoops_get_xbl_record(cxt->bprz, record, cxt->xbl_size);
> > +               if (record->size == 0)
> > +                       return -EINVAL;
> > +
> > +               persistent_ram_write(cxt->bprz, record->buf, record->size);
> > +               return 0;
> >         }  
> 
> This operates using a very strange side-effect (strange in the sense
> that none of the other przs work like this). I don't get the sense
> that XBL is actually a persistent ram zone: these are storage areas to
> be used for specific front-end purposes (console, oops, ftrace, etc).
> The XBL seems much more like a read-only backend, but having a
> read-only backend kind of defeats the purpose of pstore. :) This
> doesn't seem like it maps to pstore very well (it's not a storage
> area). You're looking to just expose a memory region somewhere in a
> kernel filesystem.
> 
> I'm not entirely opposed to creating a new type of "thing" for pstore,
> just so the filesystem infrastructure can be reused (and it's
> conceptually similar), but it looks like the expected information
> needed to read XBL are: where in memory to find it, and what is the
> maximum expected size of the log. Extracting that directly should be a
> pretty small addition to pstore.

ok, just use it locally in my side :]

Thx.

> 
> Alternatively, why not inject the XBL into the kernel printk log
> directly instead?
> 
> Also, where can I find a public specification about XBL?
> 

Powered by blists - more mailing lists