[Crash-utility] Crash-utility Digest, Vol 179, Issue 4

[David Wysochanski]

On Thu, Aug 13, 2020 at 9:08 AM lijiang <lijiang at redhat.com> wrote:
>
> 在 2020年08月13日 16:33, David Wysochanski 写道:
> > Hi Lianbo
> >
> > On Sat, Aug 8, 2020 at 10:46 PM lijiang <lijiang at redhat.com> wrote:
> >>
> >> 在 2020年08月07日 00:00, crash-utility-request at redhat.com 写道:
> >>> Message: 5
> >>> Date: Thu,  6 Aug 2020 09:30:22 -0400
> >>> From: Dave Wysochanski <dwysocha at redhat.com>
> >>> To: crash-utility at redhat.com
> >>> Subject: [Crash-utility] [PATCH v3] Fix "log" command when crash is
> >>>       started with "--minimal" option
> >>> Message-ID: <20200806133022.2127538-1-dwysocha at redhat.com>
> >>>
> >>> Commit c86250bce29f introduced the useful '-T' option to print the
> >>> log timestamp in human-readable form.  However, this option does
> >>> not work when crash is invoked with '--minimal' mode, and if tried,
> >>> crash will spin at 100% and continuously crash at a divide by 0
> >>> because machdep->hz == 0.
> >>>
> >>> Fix this by disallowing this option in minimal mode.  In addition,
> >>> only calculate the logic to calculate kt->boot_date.tv_sec
> >>> when this option is enabled.
> >>>
> >> Hi, Dave Wysochanski
> >>
> >> Thank you for the patch.
> >>
> >>> Fixes: c86250bce29f ("Introduction of the "log -T" option...")
> >>> Signed-off-by: Dave Wysochanski <dwysocha at redhat.com>
> >>> Reviewed-by: Wang Long <w at laoqinren.net>
> >>> Tested-by: Mathias Krause <minipli at grsecurity.net>
> >>> ---
> >>>  kernel.c | 5 ++++-
> >>>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/kernel.c b/kernel.c
> >>> index 5ed6021..95119f3 100644
> >>> --- a/kernel.c
> >>> +++ b/kernel.c
> >>> @@ -4939,7 +4939,10 @@ cmd_log(void)
> >>>          if (argerrs)
> >>>                  cmd_usage(pc->curcmd, SYNOPSIS);
> >>>
> >>> -     if (kt->boot_date.tv_sec == 0) {
> >>> +     if (msg_flags & SHOW_LOG_CTIME && pc->flags & MINIMAL_MODE)
> >>> +             error(FATAL, "log: option 'T' not available in minimal mode\n");
> >>> +
> >>> +     if (msg_flags & SHOW_LOG_CTIME && kt->boot_date.tv_sec == 0) {
> >>
> >> The above two 'if' statements have the same checking condition, would you mind putting them together
> >> as a statement block? E.g:
> >>
> > Sure I can resubmit a fixup of v4 patch once there are no more changes needed.
> >
> >> +       if (msg_flags & SHOW_LOG_CTIME) {
> >> +               if (pc->flags & MINIMAL_MODE) {
> >> +                       error(WARNING, "the option '-T' not available in minimal mode\n");
> >> +                       return;
> >> +               }
> >> +
> >> +               if (kt->boot_date.tv_sec == 0) {
> >> ...
> >> +               }
> >>         }
> >>
> >> In addition, might it be more reasonable to issue a warning instead of a fatal error?
> >>
> >
> > If you use WARNING it will not fix the infinite loop / CPU spin at
> > 100%.  You have to CTRL-C the crash program to get the prompt back.
> > So I do not think this is a good idea.
> >
> How did you reproduce it? Can you help to confirm if you have applied the correct patch
> as below?
>
> [root at intel-sharkbay-mb-03 crash]# git diff kernel.c
> diff --git a/kernel.c b/kernel.c
> index 5ed6021..6375b24 100644
> --- a/kernel.c
> +++ b/kernel.c
> @@ -4939,13 +4939,20 @@ cmd_log(void)
>          if (argerrs)
>                  cmd_usage(pc->curcmd, SYNOPSIS);
>
> -       if (kt->boot_date.tv_sec == 0) {
> -               ulonglong uptime_jiffies;
> -               ulong  uptime_sec;
> -               get_uptime(NULL, &uptime_jiffies);
> -               uptime_sec = (uptime_jiffies)/(ulonglong)machdep->hz;
> -               kt->boot_date.tv_sec = kt->date.tv_sec - uptime_sec;
> -               kt->boot_date.tv_nsec = 0;
> +       if (msg_flags & SHOW_LOG_CTIME) {
> +               if (pc->flags & MINIMAL_MODE) {
> +                       error(WARNING, "the option '-T' not available in minimal mode\n");
> +                       return;
> +               }
> +
> +               if (kt->boot_date.tv_sec == 0) {
> +                       ulonglong uptime_jiffies;
> +                       ulong  uptime_sec;
> +                       get_uptime(NULL, &uptime_jiffies);
> +                       uptime_sec = (uptime_jiffies)/(ulonglong)machdep->hz;
> +                       kt->boot_date.tv_sec = kt->date.tv_sec - uptime_sec;
> +                       kt->boot_date.tv_nsec = 0;
> +               }
>         }
>
>         if (msg_flags & SHOW_LOG_AUDIT) {
>
>
> I didn't see any problems, it's strange, this is my test steps.
>

You are right - I missed the 'return;' in your patch.  The WARNING is fine.

How do you want to handle this?  Do you want to take the original header
and add your signed-off-by line and commit your patch?  Or do you want
me to resubmit with review-by or signed-off-by lines?