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

From: Hans de Goede <hdegoede@redhat.com>
To: Barret Rhoden <brho@google.com>,
	David Woodhouse <dwmw2@infradead.org>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	Joerg Roedel <joro@8bytes.org>
Cc: iommu@lists.linux-foundation.org, stable@vger.kernel.org
Subject: Re: [PATCH 2/2] iommu/vt-d: dmar_parse_one_rmrr: replace WARN_TAINT with pr_warn + add_taint
Date: Mon, 9 Mar 2020 17:01:39 +0100	[thread overview]
Message-ID: <ef22beeb-fd9c-ead9-496f-e7466edc94f9@redhat.com> (raw)
In-Reply-To: <34b13929-cbea-9906-0169-8f274bd40377@google.com>

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 ?

> 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. 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)
>>
> 