[Crash-utility] [PATCH] Fix show_kernel_taints() for kernel version > 4.9
Pratyush Anand
panand at redhat.com
Thu Jan 5 10:53:44 UTC 2017
Hi Dave,
Thanks for the review
On Wednesday 04 January 2017 08:54 PM, Dave Anderson wrote:
>
> 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?
>
This is what I see with `mod -t`. But looking into code, it seems that
show_module_taint() will have to be modified similarly.
crash> mod -t
NAME TAINTS
realtek 2000
> More comments and questions below:
>
> ----- Original Message -----
>> Following kernel commit removed "struct tnt".
>>
>> commit 7fd8329ba502ef76dd91db561c7aed696b2c7720
>> Author: Petr Mladek <pmladek at suse.com>
>> 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 at redhat.com>
>> ---
>> 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.
>
OK.
>
>> 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?
Yes, in 4.10, "tnts_addr" and "tnts_len" will always have none-zero values.
>
>> +
>> + 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.
>
OK.
>> +
>> + 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.
>
Ok..That sounds better.
Will send v2 with your feedback implemented.
~Pratyush
More information about the Crash-utility
mailing list