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

From: "Dave Anderson" <anderson(a)redhat.com&gt; To: "Discussion list for crash utility usage, maintenance and development" <crash-utility(a)redhat.com&gt; 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

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.

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.