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

[lijiang]

> Date: Thu, 9 Dec 2021 01:05:07 +0000
> From: "d.hatayama at fujitsu.com" <d.hatayama at fujitsu.com>
> To: "crash-utility at redhat.com" <crash-utility at redhat.com>
> Subject: [Crash-utility] [PATCH] defs.h: fix breakage of compatibility
>         of struct machdep_table for extension modules
> Message-ID:
>         <TYAPR01MB6507D1D65B85CCE7567511AF95709 at TYAPR01MB6507.jpnprd01.prod.outlook.com>
>
> Content-Type: text/plain; charset="iso-2022-jp"
>
> 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.
>

This is similar with another issue, see its reply:
https://listman.redhat.com/archives/crash-utility/2021-December/msg00008.html

> 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>
> ---
>  defs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/defs.h b/defs.h
> index e9e8143..b63741c 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -1013,7 +1013,6 @@ 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 *);
> @@ -1063,6 +1062,7 @@ struct machdep_table {
>          void (*get_irq_affinity)(int);
>          void (*show_interrupts)(int, ulong *);
>         int (*is_page_ptr)(ulong, physaddr_t *);
> +       int (*get_cpu_reg)(int, int, const char *, int, void *);
>  };
>

Acked-by: Lianbo Jiang <lijiang at redhat.com>

>  /*
> --
> 2.31.1