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

[Dave Anderson]

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 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.


> 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.

Thanks,
  Dave