5:43 a.m.
Following kernel commit removed "struct tnt".
commit 7fd8329ba502ef76dd91db561c7aed696b2c7720
Author: Petr Mladek <pmladek(a)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(a)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;
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)
+{
+ 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;
+
+ 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');
+
+ 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");
--
2.7.4
9:24 a.m.
New subject: [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>
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>
---
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
4:53 a.m.
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(a)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(a)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
2882
days inactive
2883
days old
devel@lists.crash-utility.osci.io
2 comments
2 participants
participants (2)
-
Dave Anderson -
Pratyush Anand