Re: [PATCH v3 4/4] kdb: Switch kdb_msg_write() to use safer polling I/O

[Daniel Thompson <daniel.thompson@linaro.org>]

On Thu, May 28, 2020 at 11:48:48AM +0530, Sumit Garg wrote:
> On Wed, 27 May 2020 at 19:01, Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> >
> > On Wed, May 27, 2020 at 11:55:59AM +0530, Sumit Garg wrote:
> > > In kgdb NMI context, calling console handlers isn't safe due to locks
> > > used in those handlers which could lead to a deadlock. Although, using
> > > oops_in_progress increases the chance to bypass locks in most console
> > > handlers but it might not be sufficient enough in case a console uses
> > > more locks (VT/TTY is good example).
> > >
> > > Currently when a driver provides both polling I/O and a console then kdb
> > > will output using the console. We can increase robustness by using the
> > > currently active polling I/O driver (which should be lockless) instead
> > > of the corresponding console. For several common cases (e.g. an
> > > embedded system with a single serial port that is used both for console
> > > output and debugger I/O) this will result in no console handler being
> > > used.
> >
> > Not sure I would have predicted all those changes to kgdboc.c based on
> > this patch description. I assume this is to help identify which console
> > matches our dbg_io_ops but it would be good to spell this out.
> >
> 
> Okay, will add the corresponding description.
> 
> >
> > > Suggested-by: Daniel Thompson <daniel.thompson@linaro.org>
> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > ---
> > >  drivers/tty/serial/kgdboc.c | 17 ++++++++---------
> > >  include/linux/kgdb.h        |  2 ++
> > >  kernel/debug/kdb/kdb_io.c   | 46 +++++++++++++++++++++++++++++++--------------
> > >  3 files changed, 42 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> > > index c9f94fa..6199fe1 100644
> > > --- a/drivers/tty/serial/kgdboc.c
> > > +++ b/drivers/tty/serial/kgdboc.c
> > > @@ -35,7 +35,6 @@ static struct kparam_string kps = {
> > >  };
> > >
> > >  static int kgdboc_use_kms;  /* 1 if we use kernel mode switching */
> > > -static struct tty_driver     *kgdb_tty_driver;
> > >  static int                   kgdb_tty_line;
> > >
> > >  #ifdef CONFIG_KDB_KEYBOARD
> > > @@ -154,7 +153,7 @@ static int configure_kgdboc(void)
> > >       }
> > >
> > >       kgdboc_io_ops.is_console = 0;
> > > -     kgdb_tty_driver = NULL;
> > > +     kgdboc_io_ops.tty_drv = NULL;
> > >
> > >       kgdboc_use_kms = 0;
> > >       if (strncmp(cptr, "kms,", 4) == 0) {
> > > @@ -178,7 +177,7 @@ static int configure_kgdboc(void)
> > >               }
> > >       }
> > >
> > > -     kgdb_tty_driver = p;
> > > +     kgdboc_io_ops.tty_drv = p;
> > >       kgdb_tty_line = tty_line;
> > >
> > >  do_register:
> > > @@ -216,18 +215,18 @@ static int __init init_kgdboc(void)
> > >
> > >  static int kgdboc_get_char(void)
> > >  {
> > > -     if (!kgdb_tty_driver)
> > > +     if (!kgdboc_io_ops.tty_drv)
> > >               return -1;
> > > -     return kgdb_tty_driver->ops->poll_get_char(kgdb_tty_driver,
> > > -                                             kgdb_tty_line);
> > > +     return kgdboc_io_ops.tty_drv->ops->poll_get_char(kgdboc_io_ops.tty_drv,
> > > +                                                      kgdb_tty_line);
> > >  }
> > >
> > >  static void kgdboc_put_char(u8 chr)
> > >  {
> > > -     if (!kgdb_tty_driver)
> > > +     if (!kgdboc_io_ops.tty_drv)
> > >               return;
> > > -     kgdb_tty_driver->ops->poll_put_char(kgdb_tty_driver,
> > > -                                     kgdb_tty_line, chr);
> > > +     kgdboc_io_ops.tty_drv->ops->poll_put_char(kgdboc_io_ops.tty_drv,
> > > +                                               kgdb_tty_line, chr);
> > >  }
> > >
> > >  static int param_set_kgdboc_var(const char *kmessage,
> > > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> > > index b072aeb..05d165d 100644
> > > --- a/include/linux/kgdb.h
> > > +++ b/include/linux/kgdb.h
> > > @@ -275,6 +275,7 @@ struct kgdb_arch {
> > >   * for the I/O driver.
> > >   * @is_console: 1 if the end device is a console 0 if the I/O device is
> > >   * not a console
> > > + * @tty_drv: Pointer to polling tty driver.
> > >   */
> > >  struct kgdb_io {
> > >       const char              *name;
> > > @@ -285,6 +286,7 @@ struct kgdb_io {
> > >       void                    (*pre_exception) (void);
> > >       void                    (*post_exception) (void);
> > >       int                     is_console;
> > > +     struct tty_driver       *tty_drv;
> >
> > Should this be a struct tty_driver or a struct console?
> >
> > In other words if the lifetime the console structure is the same as the
> > tty_driver then isn't it better to capture the console instead
> > (easier to compare and works with non-tty devices such as the
> > USB debug mode).
> >
> 
> IIUC, you mean to say we can easily replace "is_console" with "struct
> console". This sounds feasible and should be a straightforward
> comparison in order to prefer "dbg_io_ops" over console handlers. So I
> will switch to use "struct console" instead.

My comment contains an if ("if the lifetime of the console structure is
the same") so you need to check that it is true before sharing a patch to
make the change.


Daniel.

> 
> >
> > >  };
> > >
> > >  extern const struct kgdb_arch                arch_kgdb_ops;
> > > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> > > index f848482..c2efa52 100644
> > > --- a/kernel/debug/kdb/kdb_io.c
> > > +++ b/kernel/debug/kdb/kdb_io.c
> > > @@ -24,6 +24,7 @@
> > >  #include <linux/kgdb.h>
> > >  #include <linux/kdb.h>
> > >  #include <linux/kallsyms.h>
> > > +#include <linux/tty_driver.h>
> > >  #include "kdb_private.h"
> > >
> > >  #define CMD_BUFLEN 256
> > > @@ -542,13 +543,18 @@ static int kdb_search_string(char *searched, char *searchfor)
> > >       return 0;
> > >  }
> > >
> > > -static void kdb_io_write(char *cp, int len, void (*io_put_char)(u8 ch))
> > > +static void kdb_io_write(char *cp, int len, void (*io_put_char)(u8),
> > > +                      struct tty_driver *p, int line,
> > > +                      void (*poll_put_char)(struct tty_driver *, int, char))
> >
> > Judging from your reply to comment 1 I guess this is already on the list
> > to eliminate ;-).
> >
> 
> Yeah.
> 
> -Sumit
> 
> >
> > Daniel.
> >
> >
> > >  {
> > >       if (len <= 0)
> > >               return;
> > >
> > >       while (len--) {
> > > -             io_put_char(*cp);
> > > +             if (io_put_char)
> > > +                     io_put_char(*cp);
> > > +             if (poll_put_char)
> > > +                     poll_put_char(p, line, *cp);
> > >               cp++;
> > >       }
> > >  }
> > > @@ -561,22 +567,34 @@ static void kdb_msg_write(char *msg, int msg_len)
> > >               return;
> > >
> > >       if (dbg_io_ops && !dbg_io_ops->is_console)
> > > -             kdb_io_write(msg, msg_len, dbg_io_ops->write_char);
> > > +             kdb_io_write(msg, msg_len, dbg_io_ops->write_char,
> > > +                          NULL, 0, NULL);
> > >
> > >       for_each_console(c) {
> > > +             int line;
> > > +             struct tty_driver *p;
> > > +
> > >               if (!(c->flags & CON_ENABLED))
> > >                       continue;
> > > -             /*
> > > -              * While rounding up CPUs via NMIs, its possible that
> > > -              * a rounded up CPU maybe holding a console port lock
> > > -              * leading to kgdb master CPU stuck in a deadlock during
> > > -              * invocation of console write operations. So in order
> > > -              * to avoid such a deadlock, enable oops_in_progress
> > > -              * prior to invocation of console handlers.
> > > -              */
> > > -             ++oops_in_progress;
> > > -             c->write(c, msg, msg_len);
> > > -             --oops_in_progress;
> > > +
> > > +             p = c->device ? c->device(c, &line) : NULL;
> > > +             if (p && dbg_io_ops && p == dbg_io_ops->tty_drv && p->ops &&
> > > +                 p->ops->poll_put_char) {
> > > +                     kdb_io_write(msg, msg_len, NULL, p, line,
> > > +                                  p->ops->poll_put_char);
> > > +             } else {
> > > +                     /*
> > > +                      * While rounding up CPUs via NMIs, its possible that
> > > +                      * a rounded up CPU maybe holding a console port lock
> > > +                      * leading to kgdb master CPU stuck in a deadlock during
> > > +                      * invocation of console write operations. So in order
> > > +                      * to avoid such a deadlock, enable oops_in_progress
> > > +                      * prior to invocation of console handlers.
> > > +                      */
> > > +                     ++oops_in_progress;
> > > +                     c->write(c, msg, msg_len);
> > > +                     --oops_in_progress;
> > > +             }
> > >               touch_nmi_watchdog();
> > >       }
> > >  }
> > > --
> > > 2.7.4
> > >