Re: [Crash-utility] [PATCH] Fix show_kernel_taints() for kernel version > 4.9

Hi Pratyush, Thanks for catching this so soon, and I appreciate the patch proposal along with it. I've got a few questions, comments, and suggestions re: the patch. This failure occurs during session initialization, and I'm sure that the patch fixes that, and also when running the "sys -t" option during runtime. But what happens when you run "mod -t"? It would seem that the same type of bug would occur, right?

More comments and questions below: ----- Original Message ----- > Following kernel commit removed "struct tnt". > > commit 7fd8329ba502ef76dd91db561c7aed696b2c7720 > Author: Petr Mladek <pmladek(a)suse.com&gt; > Date: Wed Sep 21 13:47:22 2016 +0200 > > taint/module: Clean up global and module taint flags handling > > Now "struct taint_flag" has tainted character information. > > Without this patch we see following error on a kernel version v4.10-rc1. > > crash: invalid structure size: tnt > FILE: kernel.c LINE: 10459 FUNCTION: show_kernel_taints() > > [./crash] error trace: 4cb49c => 4c7cd0 => 50f4e0 => 50f464 > > 50f464: SIZE_verify.part.29+72 > 50f4e0: store_module_kallsyms_v1.part.30+0 > 4c7cd0: show_kernel_taints+352 > 4cb49c: is_livepatch+44 > > Signed-off-by: Pratyush Anand <panand(a)redhat.com&gt; > --- > defs.h | 1 + > kernel.c | 66 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 67 insertions(+) > > diff --git a/defs.h b/defs.h > index 31a4dc490ed4..0c25d5aa4afd 100644 > --- a/defs.h > +++ b/defs.h > @@ -2117,6 +2117,7 @@ struct size_table { /* stash of commonly-used > sizes */ > long hrtimer_clock_base; > long hrtimer_base; > long tnt; > + long taint_flag; > long trace_print_flags; > long task_struct_flags; > long timer_base; Minor nit here -- all additions to the size_table (and offset_table) should be added to the end of the structure to avoid breaking pre-compiled extension modules. Also, it's customary to display the value of the new field in dump_offset_table(), which is called from "help -o". You can display the size of the "taint_flag" underneath the "tnt" size in that function.

> diff --git a/kernel.c b/kernel.c > index bdd0d05eed97..31917176e8c9 100644 > --- a/kernel.c > +++ b/kernel.c > @@ -10416,6 +10416,67 @@ dump_variable_length_record(void) > } > > static void > +show_kernel_taints_v4_10(char *buf, int verbose) It's definitely worth breaking out a new function given that the current show_kernel_taints() is covering a bit of history. But I think you're carrying forward some unnecessary baggage. See below... > +{ > + int i, bx; > + char tnt_true, tnt_false; > + int tnts_len; > + ulong tnts_addr; > + ulong tainted_mask, *tainted_mask_ptr; > + int tainted; > + struct syment *sp; > + > + if (!VALID_STRUCT(taint_flag)) { > + STRUCT_SIZE_INIT(taint_flag, "taint_flag"); > + MEMBER_OFFSET_INIT(tnt_true, "taint_flag", "true"); > + MEMBER_OFFSET_INIT(tnt_false, "taint_flag", "false"); > + } > + > + if (VALID_STRUCT(taint_flag) && (sp = symbol_search("taint_flags"))) { > + tnts_len = get_array_length("taint_flags", NULL, 0); > + tnts_addr = sp->value; > + } else > + tnts_addr = tnts_len = 0; As of 4.10, is there any possibility of "tnts_addr" and "tnts_len" being 0?

> + > + bx = 0; > + buf[0] = '\0'; > + > + tainted_mask = tainted = 0; > + > + if (kernel_symbol_exists("tainted_mask")) { > + get_symbol_data("tainted_mask", sizeof(ulong), &tainted_mask); > + tainted_mask_ptr = &tainted_mask; > + } else if (kernel_symbol_exists("tainted")) { > + get_symbol_data("tainted", sizeof(int), &tainted); > + if (verbose) > + fprintf(fp, "TAINTED: %x\n", tainted); > + return; > + } else if (verbose) > + option_not_supported('t'); In 4.10, only "tainted_mask" applies, so there is no reason to continue checking for the old "tainted" symbol.

> + > + for (i = 0; i < (tnts_len * SIZE(taint_flag)); i += SIZE(taint_flag)) { > + if (NUM_IN_BITMAP(tainted_mask_ptr, i)) { > + readmem((tnts_addr + i) + OFFSET(tnt_true), > + KVADDR, &tnt_true, sizeof(char), > + "tnt true", FAULT_ON_ERROR); > + buf[bx++] = tnt_true; > + } else { > + readmem((tnts_addr + i) + OFFSET(tnt_false), > + KVADDR, &tnt_false, sizeof(char), > + "tnt false", FAULT_ON_ERROR); > + if (tnt_false != ' ' && tnt_false != '-' && > + tnt_false != 'G') > + buf[bx++] = tnt_false; > + } > + } > + > + buf[bx++] = '\0'; > + > + if (verbose) > + fprintf(fp, "TAINTED_MASK: %lx %s\n", tainted_mask, buf); > +} > + > +static void > show_kernel_taints(char *buf, int verbose) > { > int i, bx; > @@ -10427,6 +10488,11 @@ show_kernel_taints(char *buf, int verbose) > int tainted; > struct syment *sp; > > + if (THIS_KERNEL_VERSION > LINUX(4,9,0)) { > + show_kernel_taints_v4_10(buf, verbose); > + return; > + } > + > if (!VALID_STRUCT(tnt)) { > STRUCT_SIZE_INIT(tnt, "tnt"); > MEMBER_OFFSET_INIT(tnt_bit, "tnt", "bit"); > -- While the THIS_KERNEL_VERSION is used quite frequently, it can be a problem if the kernel patch is backported into an older kernel. So it is often preferable to utilize the existence of a kernel data structure and/or symbol as the decision point instead. So in this case, I would suggest something like: show_kernel_taints(char *buf, int verbose) { int i, bx; @@ -10427,6 +10488,11 @@ show_kernel_taints(char *buf, int verbose) int tainted; struct syment *sp; + if (kernel_symbol_exists("taint_flags") && STRUCT_EXISTS("taint_flag")) { + show_kernel_taints_v4_10(buf, verbose); + return; + } + if (!VALID_STRUCT(tnt)) { STRUCT_SIZE_INIT(tnt, "tnt"); MEMBER_OFFSET_INIT(tnt_bit, "tnt", "bit"); Note that the STRUCT_EXISTS() macro works regardless whether the size_table.taint_flag has been initialized.