[Crash-utility] [PATCH] x86_64: Fix check for __per_cpu_offset initialisation

[Philipp Rudo]

Hi Lianbo,

On Wed, 11 Aug 2021 17:05:26 +0800
lijiang <lijiang at redhat.com> wrote:

> >
> > Date: Thu,  5 Aug 2021 15:19:37 +0200
> > From: Philipp Rudo <prudo at redhat.com>
> > To: crash-utility at redhat.com
> > Subject: [Crash-utility] [PATCH] x86_64: Fix check for
> >         __per_cpu_offset        initialisation
> > Message-ID: <20210805131937.5051-1-prudo at redhat.com>
> >
> > Since at least kernel v2.6.30 the __per_cpu_offset gets initialized to
> > __per_cpu_load. So first check if the __per_cpu_offset was set to a
> > proper value before reading any per cpu variable to prevent potential
> > bugs.
> >
> >  
> Hi, Philipp
> 
> Thank you for the patch. Can you help to describe  more details about the
> potential risks? and what conditions might trigger the potential bugs?

the bug is always triggered during initialization of the per-cpu data
on x86_64. At least for kernels not using struct x8664_pda, which
AFAIK was also removed with kernel v2.6.30.

The risk for crash is low. Right after the superfluous read there is a
check if the read cpunumber matches the expected one. 

                         if (cpunumber != cpus)
                                 break;

So the worst case scenario I see is that crash initializes one
additional cpu with non-sense data. But given that the bug exists for
~12 years and nobody reported such an bug I assume that the check worked
well so far.

> Did you mean that it's related to the crash live analysis issue(1978032)? I
> tried to reproduce it, but so far I haven't reproduced it with the upstream
> kernel.

Yes, this bug is related to bz1978032. For whatever reason the
superfluous read triggered the panic.

I could reproduce the bug upstream with CONFIG_IO_URING _disabled_.
Unfortunately there is a RHEL-only patch [1] that tampers with the
Kconfig for IO_URING. So when you copy a kernel-ark config to the
upstream repo and run 'make oldconfig' the IO_URING will silently be
_enabled_.

BTW, I tried to reproduce the panic yesterday on kernel-5.14.0-0.rc4
but failed. Not sure if the bug was fixed in the meantime or I was
simply "lucky"...

Thanks
Philipp


[1]
https://gitlab.com/cki-project/kernel-ark/-/commit/50f8c7fb77c97617dbf32519a79a00621ea7d1a3

> 
> Thanks.
> Lianbo
> 
> 
> > Signed-off-by: Philipp Rudo <prudo at redhat.com>
> > ---
> >  x86_64.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/x86_64.c b/x86_64.c
> > index 6eb7d67..0bb8705 100644
> > --- a/x86_64.c
> > +++ b/x86_64.c
> > @@ -1327,6 +1327,8 @@ x86_64_per_cpu_init(void)
> >                 ms->stkinfo.isize = 16384;
> >
> >         for (i = cpus = 0; i < NR_CPUS; i++) {
> > +               if (kt->__per_cpu_offset[i] ==
> > symbol_value("__per_cpu_load"))
> > +                       break;
> >                 if (!readmem(cpu_sp->value + kt->__per_cpu_offset[i],
> >                     KVADDR, &cpunumber, sizeof(int),
> >                     "cpu number (per_cpu)", QUIET|RETURN_ON_ERROR))
> > @@ -5602,7 +5604,7 @@ x86_64_get_smp_cpus(void)
> >                         return 1;
> >
> >                 for (i = cpus = 0; i < NR_CPUS; i++) {
> > -                       if (kt->__per_cpu_offset[i] == 0)
> > +                       if (kt->__per_cpu_offset[i] ==
> > symbol_value("__per_cpu_load"))
> >                                 break;
> >                         if (!readmem(sp->value + kt->__per_cpu_offset[i],
> >                             KVADDR, &cpunumber, sizeof(int),
> > --
> > 2.31.1
> >
> >