Re: [PATCH v2] Show module taint flags

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




----- Original Message -----
> 
> > 
> > On RHEL5 Xen kernel, it shows multiple garbage graphic characters, and I
> > cannot even transpose it, because it must have a CR, LF, or something else
> > embedded in the garbarge.
> > 
> > If you want to re-post, fix the module list truncation issue, try to do
> > something to handle kernel versions that do not have the same #define's
> > as what you're hardwiring, and perhaps make it into a "mod -t" option
> > that displays just the module name and its TAINT flag.
> > 
> > Dave
> > 
> 
> Hi Dave,
> 
> Firstly, sorry for the delay.
> Here's another attempt.
> 
> An example:
> 
> crash> linux_banner
> linux_banner = $3 = "Linux version 2.6.32-220.28.1.el6.x86_64
> (mockbuild@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx) (gcc version 4.4.6 20110731
> (Red Hat 4.4.6-3) (GCC) ) #1 SMP Wed Oct 3 12:26:28 EDT 2012\n"
> 
> crash> mod -T
> NAME                 TAINT
> vxspec               (P)(U)
> dmpaa                (P)(U)
> emcpgpx              (P)(U)
> dmpap                (P)(U)
> dmpjbod              (P)(U)
> emcp                 (P)(U)
> emcpmpx              (P)(U)
> emcpdm               (P)(U)
> emcpxcrypt           (P)(U)
> emcpvlumd            (P)(U)
> vxfs                 (P)(U)
> fdd                  (P)(U)
> vxportal             (P)(U)
> vxdmp                (P)(U)
> vxio                 (P)(U)
> llt                  (P)(U)
> gab                  (P)(U)
> vxfen                (P)(U)
> vxglm                (P)(U)
> vxgms                (P)(U)
> vxodm                (P)(U)
> 
> crash> mod -t vxfs
> NAME                 TAINT
> vxfs                 (P)(U)


Hi Aaron,

Before getting into my somewhat long-winded review, I do want to
emphasize that I think this is a valuable option for kernel debugging,
and it absolutely warrants being folded into the "mod" command.

The v2 patch tested OK on a wide variety of kernel versions without
running into any of the garbage display issues I saw with the first
version.

Here are my issues/comments with the v2 patch.  Let's get the simple 
things out of the way first.

At some point during your development, run the build through
"make warn", and address any issues there:

  $ make warn
  ...
  cc -c -g -DX86_64  -DGDB_7_3_1  kernel.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector 
  kernel.c:3512:1: warning: no previous prototype for 'is_module_taint' [-Wmissing-prototypes]
  kernel.c:3577:1: warning: no previous prototype for 'show_module_taint' [-Wmissing-prototypes]
  kernel.c: In function 'show_module_taint':
  kernel.c:3579:9: warning: unused variable 'bx' [-Wunused-variable]
  ...

I wouldn't bother with the "mod -t <module>" vs. "mod -T".
The number of modules that are tainted would not be so onerous
that it warrants a per-module option.  (Just pipe it to grep if
you *really* feel it's necessary.)  So I would just make one simple
"mod -t" option that checks all the modules and displays those with
taint flags.
  
If no modules are tainted, then indicate that -- instead of just this:

  crash> mod -T
  NAME                   TAINT
  crash>

You could change it to only show the header if there are actually tainted
modules, and if there are none, then show something like "no tainted modules".

And the "help mod" page would obviously need documentation...
  
Now to the meat of the matter, as I mentioned when reviewing the 
first patch, I really prefer that hardwiring of kernel #define's 
be avoided if at all possible.  Your patch seems to pick a
subset of the possible flags:

   /*
  +* Module taint flags
  +*/
  +
  +#define TAINT_PROPRIETARY_MODULE        0
  +#define TAINT_FORCED_MODULE             1
  +#define TAINT_UNSIGNED_MODULE           6
  +#define TAINT_CRAP                     10
  +#define TAINT_OOT_MODULE               12
  +#define TAINT_TECH_PREVIEW             29
  +

And looking at the kernel history, say, starting at upstream 2.6.18:
  
  #define TAINT_PROPRIETARY_MODULE        (1<<0)
  #define TAINT_FORCED_MODULE             (1<<1)
  #define TAINT_UNSAFE_SMP                (1<<2)
  #define TAINT_FORCED_RMMOD              (1<<3)
  #define TAINT_MACHINE_CHECK             (1<<4)
  #define TAINT_BAD_PAGE                  (1<<5)

where RHEL5 expounded upon the base 2.6.18 kernel like this:
  
  #define TAINT_PROPRIETARY_MODULE	(1<<0)
  #define TAINT_FORCED_MODULE		(1<<1)
  #define TAINT_UNSAFE_SMP		(1<<2)
  #define TAINT_FORCED_RMMOD		(1<<3)
  #define TAINT_MACHINE_CHECK		(1<<4)
  #define TAINT_BAD_PAGE			(1<<5)
  #define TAINT_UNSIGNED_MODULE		(1<<6)
  #define TAINT_7 			(1<<7)
  #define TAINT_8 			(1<<8)
  #define TAINT_9 			(1<<9)
  #define TAINT_10			(1<<10)
  #define TAINT_11			(1<<11)
  #define TAINT_12			(1<<12)
  #define TAINT_13			(1<<13)
  #define TAINT_14			(1<<14)
  #define TAINT_15			(1<<15)
  #define TAINT_16			(1<<16)
  #define TAINT_17			(1<<17)
  #define TAINT_18			(1<<18)
  #define TAINT_19			(1<<19)
  #define TAINT_20			(1<<20)
  #define TAINT_21			(1<<21)
  #define TAINT_22			(1<<22)
  #define TAINT_23			(1<<23)
  #define TAINT_24			(1<<24)
  #define TAINT_25			(1<<25)
  #define TAINT_26			(1<<26)
  #define TAINT_27			(1<<27)
  /* Reserving bits for vendor specific uses */
  #define TAINT_HARDWARE_UNSUPPORTED	(1<<28)
  #define TAINT_TECH_PREVIEW		(1<<29)
  #define TAINT_RESERVED30		(1<<30)
  #define TAINT_RESERVED31		(1<<31)
  
Then if we look at upstream 2.6.32, a few more bits
were added:
  
  #define TAINT_PROPRIETARY_MODULE        0
  #define TAINT_FORCED_MODULE             1
  #define TAINT_UNSAFE_SMP                2
  #define TAINT_FORCED_RMMOD              3
  #define TAINT_MACHINE_CHECK             4
  #define TAINT_BAD_PAGE                  5
  #define TAINT_USER                      6
  #define TAINT_DIE                       7
  #define TAINT_OVERRIDDEN_ACPI_TABLE     8
  #define TAINT_WARN                      9
  #define TAINT_CRAP                      10
  
But RHEL6 (2.6.32-based) went a bit further:
  
  #define TAINT_PROPRIETARY_MODULE	0
  #define TAINT_FORCED_MODULE		1
  #define TAINT_UNSAFE_SMP		2
  #define TAINT_FORCED_RMMOD		3
  #define TAINT_MACHINE_CHECK		4
  #define TAINT_BAD_PAGE			5
  #define TAINT_USER			6
  #define TAINT_DIE			7
  #define TAINT_OVERRIDDEN_ACPI_TABLE	8
  #define TAINT_WARN			9
  #define TAINT_CRAP			10
  #define TAINT_FIRMWARE_WORKAROUND	11
  #define TAINT_12			12
  #define TAINT_13			13
  #define TAINT_14			14
  #define TAINT_15			15
  #define TAINT_16			16
  #define TAINT_17			17
  #define TAINT_18			18
  #define TAINT_19			19
  #define TAINT_20			20
  #define TAINT_21			21
  #define TAINT_22			22
  #define TAINT_23			23
  #define TAINT_24			24
  #define TAINT_25			25
  #define TAINT_26			26
  #define TAINT_BIT_BY_ZOMBIE		27
  /* Reserving bits for vendor specific uses */
  #define TAINT_HARDWARE_UNSUPPORTED	28
  #define TAINT_TECH_PREVIEW		29
  /* Bits 30 - 31 are reserved for Red Hat use only */
  #define TAINT_RESERVED30		30
  #define TAINT_RESERVED31		31
   
And here's the state in the current upstream linux git tree:
  
  #define TAINT_PROPRIETARY_MODULE        0
  #define TAINT_FORCED_MODULE             1
  #define TAINT_UNSAFE_SMP                2
  #define TAINT_FORCED_RMMOD              3
  #define TAINT_MACHINE_CHECK             4
  #define TAINT_BAD_PAGE                  5
  #define TAINT_USER                      6
  #define TAINT_DIE                       7
  #define TAINT_OVERRIDDEN_ACPI_TABLE     8
  #define TAINT_WARN                      9
  #define TAINT_CRAP                      10
  #define TAINT_FIRMWARE_WORKAROUND       11
  #define TAINT_OOT_MODULE                12
  
So how would be the best way to deal with handling the other bits 
that you are not checking? 

When I reviewed your first post, I didn't notice that the
solution -- at least for 2.6.28 and later kernels -- is pretty
much implemented in the kernel by the introduction of the
kernel's "tnts" structure and the print_tainted() function:

  struct tnt {
          u8      bit;
          char    true;
          char    false;
  };
  
  static const struct tnt tnts[] = {
          { TAINT_PROPRIETARY_MODULE,     'P', 'G' },
          { TAINT_FORCED_MODULE,          'F', ' ' },
          { TAINT_UNSAFE_SMP,             'S', ' ' },
          { TAINT_FORCED_RMMOD,           'R', ' ' },
          { TAINT_MACHINE_CHECK,          'M', ' ' },
          { TAINT_BAD_PAGE,               'B', ' ' },
          { TAINT_USER,                   'U', ' ' },
          { TAINT_DIE,                    'D', ' ' },
          { TAINT_OVERRIDDEN_ACPI_TABLE,  'A', ' ' },
          { TAINT_WARN,                   'W', ' ' },
          { TAINT_CRAP,                   'C', ' ' },
          { TAINT_FIRMWARE_WORKAROUND,    'I', ' ' },
          { TAINT_OOT_MODULE,             'O', ' ' },
  };
  
Why not just read the "tnts[]" array and then use the same
logic as the kernel's print_tainted() function?  Doing that 
would even handle the vendor-specific issue, where for example,
RHEL6 has this version of the tnts array:

  static const struct tnt tnts[] = {
          { TAINT_PROPRIETARY_MODULE,     'P', 'G' },
          { TAINT_FORCED_MODULE,          'F', ' ' },
          { TAINT_UNSAFE_SMP,             'S', ' ' },
          { TAINT_FORCED_RMMOD,           'R', ' ' },
          { TAINT_MACHINE_CHECK,          'M', ' ' },
          { TAINT_BAD_PAGE,               'B', ' ' },
          { TAINT_USER,                   'U', ' ' },
          { TAINT_DIE,                    'D', ' ' },
          { TAINT_OVERRIDDEN_ACPI_TABLE,  'A', ' ' },
          { TAINT_WARN,                   'W', ' ' },
          { TAINT_CRAP,                   'C', ' ' },
          { TAINT_FIRMWARE_WORKAROUND,    'I', ' ' },
          { TAINT_12,                     '?', '-' },
          { TAINT_13,                     '?', '-' },
          { TAINT_14,                     '?', '-' },
          { TAINT_15,                     '?', '-' },
          { TAINT_16,                     '?', '-' },
          { TAINT_17,                     '?', '-' },
          { TAINT_18,                     '?', '-' },
          { TAINT_19,                     '?', '-' },
          { TAINT_20,                     '?', '-' },
          { TAINT_21,                     '?', '-' },
          { TAINT_22,                     '?', '-' },
          { TAINT_23,                     '?', '-' },
          { TAINT_24,                     '?', '-' },
          { TAINT_25,                     '?', '-' },
          { TAINT_26,                     '?', '-' },
          { TAINT_BIT_BY_ZOMBIE,          'Z', ' ' },
          { TAINT_HARDWARE_UNSUPPORTED,   'H', ' ' },
          { TAINT_TECH_PREVIEW,           'T', ' ' },
  };

For kernels earlier than 2.6.28, you could still hardwire
the values that were valid at that point in time, or you
could use the subset of bits that you've chosen.  But for
bits that are set that are *not* part of your chosen
subset (if you do it that way), you should probably put
the bit number in brackets, and then the user could refer
to the relevant kernel source for details on what the bit
number means.  

But from 2.6.28 on, please use the kernel data structure
contents.  

And lastly, what about the transition from module.gpgsig_ok to 
module.sig_ok?  And BTW, if the relevant "ok" bit is not set, 
will the (U) be displayed twice?  Maybe there should be a
special display to differentiate the two (U) possibilities?

Thanks,
  Dave

--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux