Re: [PATCH 2/2] iommu/vt-d: dmar_parse_one_rmrr: replace WARN_TAINT with pr_warn + add_taint

[Barret Rhoden via iommu <iommu@lists.linux-foundation.org>]

On 3/9/20 12:01 PM, Hans de Goede wrote:
> Hi,
> 
> On 3/9/20 4:57 PM, Barret Rhoden wrote:
>> On 3/9/20 10:01 AM, Hans de Goede wrote:
>>> Quoting from the comment describing the WARN functions in
>>> include/asm-generic/bug.h:
>>>
>>>    * WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report
>>>    * significant kernel issues that need prompt attention if they should ever
>>>    * appear at runtime.
>>>    *
>>>    * Do not use these macros when checking for invalid external inputs
>>>
>>> The (buggy) firmware tables which the dmar code was calling WARN_TAINT
>>> for really are invalid external inputs. They are not under the kernel's
>>> control and the issues in them cannot be fixed by a kernel update.
>>
>> This patch sounds good to me.
> 
> Can we have your Acked-by then ?

Acked-by Barret Rhoden <brho@google.com>

>> Given the rules with WARN and external inputs, it sounds like *all* uses of WARN_TAINT with TAINT_FIRMWARE_WORKAROUND are bad: WARNs that are likely based on invalid external input.  Presumably we're working around FW bugs.
> 
> Right, as I mentioned in the cover letter I'm working on a follow-up
> series fixing the other cases.

Great!

Thanks,

Barret


  I wanted to get these 2 out there (and
> hopefully into 5.6-rc# soon) as they are causing aprox 1-2 new
> bug-reports to be filed every day for just Fedora.
> 
>> While we're on the subject, is WARN_TAINT() ever worth the backtrace + bug report?  Given the criteria is "prompt attention", it should be something like "nice to know about when debugging."
> 
> I have not looked at WARN_TAINT usages other then those with the
> TAINT_FIRMWARE_WORKAROUND flag; and as mentioned I do plan to fix
> those. Feel free to take a look at any other callers :)
> 
> Regards,
> 
> Hans
> 
> 
> 
>>> So logging a backtrace, which invites bug reports to be filed about this,
>>> is not helpful.
>>>
>>> Some distros, e.g. Fedora, have tools watching for the kernel backtraces
>>> logged by the WARN macros and offer the user an option to file a bug for
>>> this when these are encountered. The WARN_TAINT in dmar_parse_one_rmrr
>>> + another iommu WARN_TAINT, addressed in another patch, have lead to over
>>> a 100 bugs being filed this way.
>>>
>>> This commit replaces the WARN_TAINT("...") call, with a
>>> pr_warn(FW_BUG "...") + add_taint(TAINT_FIRMWARE_WORKAROUND, ...) call
>>> avoiding the backtrace and thus also avoiding bug-reports being filed
>>> about this against the kernel.
>>>
>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1808874
>>> Fixes: f5a68bb0752e ("iommu/vt-d: Mark firmware tainted if RMRR fails sanity check")
>>> Cc: Barret Rhoden <brho@google.com>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>    drivers/iommu/intel-iommu.c | 6 ++++--
>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>>> index 6fa6de2b6ad5..3857a5cd1a75 100644
>>> --- a/drivers/iommu/intel-iommu.c
>>> +++ b/drivers/iommu/intel-iommu.c
>>> @@ -4460,14 +4460,16 @@ int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg)
>>>        struct dmar_rmrr_unit *rmrru;
>>>        rmrr = (struct acpi_dmar_reserved_memory *)header;
>>> -    if (rmrr_sanity_check(rmrr))
>>> -        WARN_TAINT(1, TAINT_FIRMWARE_WORKAROUND,
>>> +    if (rmrr_sanity_check(rmrr)) {
>>> +        pr_warn(FW_BUG
>>>                   "Your BIOS is broken; bad RMRR [%#018Lx-%#018Lx]\n"
>>>                   "BIOS vendor: %s; Ver: %s; Product Version: %s\n",
>>>                   rmrr->base_address, rmrr->end_address,
>>>                   dmi_get_system_info(DMI_BIOS_VENDOR),
>>>                   dmi_get_system_info(DMI_BIOS_VERSION),
>>>                   dmi_get_system_info(DMI_PRODUCT_VERSION));
>>> +        add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
>>> +    }
>>>        rmrru = kzalloc(sizeof(*rmrru), GFP_KERNEL);
>>>        if (!rmrru)
>>>
>>
> 

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu