[Crash-utility] [Patch v4] Show module taint flags

[Aaron Tomlin]

----- Original Message -----
> From: "Dave Anderson" <anderson at redhat.com>
> To: "Discussion list for crash utility usage, maintenance and development" <crash-utility at redhat.com>
> Sent: Tuesday, 21 May, 2013 7:36:57 PM
> Subject: Re: [Crash-utility] [Patch v4] Show module taint flags
> 
> 
> 
> Hi Aaron,
> 
> With respect to your v4 version of the patch, I just found a few
> issues that need addressing.
> 
> The module.gpgsig_ok "unsigned" check was only made if module.taints
> exists, and not checked if module.license_gplok exists.  Earlier
> kernels have license_gplok/gpgsig_ok combinations.
> 
> I'm not sure how you were able to get this sample display from your
> help page:
> 
>    crash> mod -T
>      NAME                 TAINT
>      dm_mod               G
>      scsi_tgt             G
>      serio_raw            G
>      dm_log               G
>      ata_generic          G
>      qla2xxx              P(U)
>      dm_region_hash       G
>      enclosure            G
>      pata_acpi            G
> 

Yes, this is a relic from an older patch, which wasn't updated.

> because the module.taints field would be 0 for all but the
> qla2xxx module, and your code gates the tnts[] true/false display
> by "if (taints)"?  In my tests of your patch, the 'G' would only
> be shown if some *other* bit in the bitfield were set.
> 
> Anway, I don't feel that it's necessary to print the 'G' for GPL'd
> modules.  We could probably just drop the "false" bit check entirely,
> but perhaps there will be something put there in the future that
> would be worthy of displaying?  But it would only get picked up if
> some *other* bit in the mask were set, so for now it's pretty much
> useless code.
> 

Agreed.

> The module maxnamelen should be restricted to the modules that
> are actually tainted or unsigned.
> 
> Another issue -- up until 2.6.19, the module->license_gplok was
> a boolean, where "1" meant it was GPL.  RHEL5 completely turned
> that around, and made license_gplok a bitmask, whereas upstream
> 2.6.18 still has it as a boolean.  So since we're keying on non-zero
> values in that field, it confuses the issue because, say on a RHEL4
> kernel, it will show *all* GPL modules as having a hexadecimal value
> of 1.  But I can't find an obvious way of determining what's in play
> for a given kernel, so the "consult the kernel sources" reference
> in the help page will have to suffice.
> 
> Anyway, in order to get some traction in getting this patch into
> place, I've reworked it a bit, and have attached my counter-proposal
> patch.  Here's what I've changed:
> 
> (1) The '-t' and '-T' difference is too confusing.  I've changed it
>     to simply be 't', and the display will be symbolic (the letter)
>     if tnts[] exists, and hexadecimal if not.  The unsigned indication
>     will always be shown if gdbsig_ok exists and is 0.
> 
> (2) If module.taints exists, the header shows "TAINTS".  For older
>     kernels, it shows "LICENSE_GPLOK".
>     
> (3) I moved the module structure's gpgsig_ok, taints and license_gplok
>     members, and the tnt structure's bit, true, false members into
>     the offset_table, and tnt structure size into the size_table.
>     This prevents having to dive into gdb each time MEMBER_EXISTS()
>     and MEMBER_OFFSET are called, and will catch any changes in
>     the structure names in the future.
>     
> (4) When walking through the modules, readmem() each module's module
>     struct into a buffer and then access the relevant fields with the
>     UINT() and INT() macros, making things a bit less verbose.
> 
> (5) Never show 'G'.
>     
> (6) Set the maxnamelen only according to tainted/unsigned modules.
> 
> (7) The help page has been changed to only have '-t' as an option,
>     as well as indicating that the relevant taints/license_gplok
>     field will only be shown if they are non-zero.  The kernel
>     source reference is especially relevant given that old
>     kernels with boolean license_gplok fields will show "1"
>     for all modules.
> 
> Let me know if you have any problems with, or find any bugs in,
> the attached patch, and if not, let's queue it for crash-7.0.1.
> 

This looks fine.

Thanks for showing interesting in this feature. I'm positive that it
will be a welcomed addition to an already extensive command set.

Cheers,
Aaron