[Crash-utility] [PATCH] defs.h: fix breakage of compatibility of struct machdep_table for extension modules

[lijiang]

On Fri, Dec 10, 2021 at 12:52 PM lijiang <lijiang at redhat.com> wrote:
>
> On Fri, Dec 10, 2021 at 12:44 PM HAGIO KAZUHITO(萩尾　一仁)
> <k-hagio-ab at nec.com> wrote:
> >
> > (sorry, I mistook the email to reply.. this is the correct one.)
> >
> > -----Original Message-----
> > > Commit 2f967fb5ebd737ce5eadba462df35935122e8865 (crash_taget:
> > > fetch_registers support) added new member get_cpu_reg in the middle of
> > > struct machdep_table as:
> > >
> > >     @@ -1013,6 +1013,7 @@ struct machdep_table {
> > >            ulong (*processor_speed)(void);
> > >            int (*uvtop)(struct task_context *, ulong, physaddr_t *, int);
> > >            int (*kvtop)(struct task_context *, ulong, physaddr_t *, int);
> > >     +       int (*get_cpu_reg)(int, int, const char *, int, void *);
> > >            ulong (*get_task_pgd)(ulong);
> > >           void (*dump_irq)(int);
> > >           void (*get_stack_frame)(struct bt_info *, ulong *, ulong *);
> > >
> > > which breaks compatibility of struct machdep_table for extension
> > > modules. In general, new member variables must be added at the end of
> > > structures to maintain compatibility of data structures shared among
> > > extension modules.
> > >
> > > For example, as the result, crash gcore command results in unexpected
> > > behavior:
> > >
> > >     crash> gcore -v 7
> > >     GETBUF(168 -> 0)
> > >     <readmem: ffff8b267dd28020, KVADDR, "task_struct.stack", 8, (FOE), 7fffaef8afa8>
> > >     <read_kdump: addr: ffff8b267dd28020 paddr: 7dd28020 cnt: 8>
> > >     read_netdump: addr: ffff8b267dd28020 paddr: 7dd28020 cnt: 8 offset: 767ec020
> > >     <readmem: ffffa3f740abff58, KVADDR, "genregs_get: pt_regs", 168, (FOE), e71f80>
> > >     gcore: invalid kernel virtual address: ffffa3f740abff58  type: "genregs_get: pt_regs"
> > >              vma hit rate:  0% (0 of 274308)
> > >
> > > crash gcore uses machdep->get_stacktop() to retrieve registers saved
> > > at the bottom of a given user-space task's kernel stack:
> > >
> > >     static int genregs_get(struct task_context *target,
> > >                          const struct user_regset *regset,
> > >                          unsigned int size, void *buf)
> > >     {
> > >     ...snip...
> > >           pt_regs_buf = GETBUF(SIZE(pt_regs));
> > >
> > >           readmem(machdep->get_stacktop(target->task) - SIZE(pt_regs), KVADDR,
> > >                   pt_regs_buf, SIZE(pt_regs), "genregs_get: pt_regs",
> > >                   gcore_verbose_error_handle());
> > >
> > > As seen above, the position of get_stacktop in struct machdep_table is
> > > behind the position where get_cpu_reg was added:
> > >
> > >         int (*uvtop)(struct task_context *, ulong, physaddr_t *, int);
> > >         int (*kvtop)(struct task_context *, ulong, physaddr_t *, int);
> > >         int (*get_cpu_reg)(int, int, const char *, int, void *);        ulong (*get_task_pgd)(ulong);
> > >       void (*dump_irq)(int);
> > >       void (*get_stack_frame)(struct bt_info *, ulong *, ulong *);
> > >       ulong (*get_stackbase)(ulong);
> > >       ulong (*get_stacktop)(ulong);
> > >
> > > Signed-off-by: HATAYAMA Daisuke <d.hatayama at fujitsu.com>
> >
> > Acked-by: Kazuhito Hagio <k-hagio-ab at nec.com>
> >

Applied:
https://github.com/crash-utility/crash/commit/c477b04aee34d4f4784c326ed715e91b2c43eb3e

Thanks
Lianbo