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)