* [PATCH 0/2] iommu/vt-d: replace WARN_TAINT with pr_warn + add_taint
@ 2020-03-09 14:01 Hans de Goede
2020-03-09 14:01 ` Hans de Goede
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Hans de Goede @ 2020-03-09 14:01 UTC (permalink / raw)
To: David Woodhouse, Lu Baolu, Joerg Roedel; +Cc: Hans de Goede, iommu
Hi All,
The iommu/vt-d code calls WARN_TAINT(... TAINT_FIRMWARE_WORKAROUND ...)
in various places. Since the firmware is outside of the kernel's control
this should not be using the WARN_TAINT macro for this, calling the WARN*
macros based on external inputs is wrong, as there is nothing we can do
to fix those external inputs and the WARN* macros are intended for things
which we can fix (also see the patch commit msg).
I'm working on a patch-set which converts all in kernel uses of
WARN_TAINT(... TAINT_FIRMWARE_WORKAROUND ...) to pr_warn + add_taint,
but I'm sending these 2 out separately because these address the 2
most troublesome cases of the vt-d code calling WARN_TAINT() which
together are responsible for over a 100 bugzilla-s in Fedora alone.
Can we please get these 2 patches queued up as fixes for 5.6-rc# ?
Regards,
Hans
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/2] iommu/vt-d: dmar: replace WARN_TAINT with pr_warn + add_taint 2020-03-09 14:01 [PATCH 0/2] iommu/vt-d: replace WARN_TAINT with pr_warn + add_taint Hans de Goede @ 2020-03-09 14:01 ` Hans de Goede 2020-03-09 14:01 ` Hans de Goede 2020-03-10 10:42 ` [PATCH 0/2] iommu/vt-d: " Joerg Roedel 2 siblings, 0 replies; 16+ messages in thread From: Hans de Goede @ 2020-03-09 14:01 UTC (permalink / raw) To: David Woodhouse, Lu Baolu, Joerg Roedel; +Cc: Hans de Goede, iommu, stable 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. 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 warn_invalid_dmar() + 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("...") calls, with pr_warn(FW_BUG "...") + add_taint(TAINT_FIRMWARE_WORKAROUND, ...) calls 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=1564895 Fixes: e625b4a95d50 ("iommu/vt-d: Parse ANDD records") Fixes: fd0c8894893c ("intel-iommu: Set a more specific taint flag for invalid BI Cc: stable@vger.kernel.org Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/iommu/dmar.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c index 071bb42bbbc5..87194a46cb0b 100644 --- a/drivers/iommu/dmar.c +++ b/drivers/iommu/dmar.c @@ -440,12 +440,13 @@ static int __init dmar_parse_one_andd(struct acpi_dmar_header *header, /* Check for NUL termination within the designated length */ if (strnlen(andd->device_name, header->length - 8) == header->length - 8) { - WARN_TAINT(1, TAINT_FIRMWARE_WORKAROUND, + pr_warn(FW_BUG "Your BIOS is broken; ANDD object name is not NUL-terminated\n" "BIOS vendor: %s; Ver: %s; Product Version: %s\n", 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); return -EINVAL; } pr_info("ANDD device: %x name: %s\n", andd->device_number, @@ -471,14 +472,14 @@ static int dmar_parse_one_rhsa(struct acpi_dmar_header *header, void *arg) return 0; } } - WARN_TAINT( - 1, TAINT_FIRMWARE_WORKAROUND, + pr_warn(FW_BUG "Your BIOS is broken; RHSA refers to non-existent DMAR unit at %llx\n" "BIOS vendor: %s; Ver: %s; Product Version: %s\n", drhd->reg_base_addr, 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); return 0; } @@ -827,14 +828,14 @@ int __init dmar_table_init(void) static void warn_invalid_dmar(u64 addr, const char *message) { - WARN_TAINT_ONCE( - 1, TAINT_FIRMWARE_WORKAROUND, + pr_warn_once(FW_BUG "Your BIOS is broken; DMAR reported at address %llx%s!\n" "BIOS vendor: %s; Ver: %s; Product Version: %s\n", addr, message, 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); } static int __ref -- 2.25.1 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 1/2] iommu/vt-d: dmar: replace WARN_TAINT with pr_warn + add_taint @ 2020-03-09 14:01 ` Hans de Goede 0 siblings, 0 replies; 16+ messages in thread From: Hans de Goede @ 2020-03-09 14:01 UTC (permalink / raw) To: David Woodhouse, Lu Baolu, Joerg Roedel; +Cc: Hans de Goede, iommu, stable 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. 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 warn_invalid_dmar() + 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("...") calls, with pr_warn(FW_BUG "...") + add_taint(TAINT_FIRMWARE_WORKAROUND, ...) calls 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=1564895 Fixes: e625b4a95d50 ("iommu/vt-d: Parse ANDD records") Fixes: fd0c8894893c ("intel-iommu: Set a more specific taint flag for invalid BI Cc: stable@vger.kernel.org Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/iommu/dmar.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c index 071bb42bbbc5..87194a46cb0b 100644 --- a/drivers/iommu/dmar.c +++ b/drivers/iommu/dmar.c @@ -440,12 +440,13 @@ static int __init dmar_parse_one_andd(struct acpi_dmar_header *header, /* Check for NUL termination within the designated length */ if (strnlen(andd->device_name, header->length - 8) == header->length - 8) { - WARN_TAINT(1, TAINT_FIRMWARE_WORKAROUND, + pr_warn(FW_BUG "Your BIOS is broken; ANDD object name is not NUL-terminated\n" "BIOS vendor: %s; Ver: %s; Product Version: %s\n", 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); return -EINVAL; } pr_info("ANDD device: %x name: %s\n", andd->device_number, @@ -471,14 +472,14 @@ static int dmar_parse_one_rhsa(struct acpi_dmar_header *header, void *arg) return 0; } } - WARN_TAINT( - 1, TAINT_FIRMWARE_WORKAROUND, + pr_warn(FW_BUG "Your BIOS is broken; RHSA refers to non-existent DMAR unit at %llx\n" "BIOS vendor: %s; Ver: %s; Product Version: %s\n", drhd->reg_base_addr, 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); return 0; } @@ -827,14 +828,14 @@ int __init dmar_table_init(void) static void warn_invalid_dmar(u64 addr, const char *message) { - WARN_TAINT_ONCE( - 1, TAINT_FIRMWARE_WORKAROUND, + pr_warn_once(FW_BUG "Your BIOS is broken; DMAR reported at address %llx%s!\n" "BIOS vendor: %s; Ver: %s; Product Version: %s\n", addr, message, 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); } static int __ref -- 2.25.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] iommu/vt-d: dmar: replace WARN_TAINT with pr_warn + add_taint 2020-03-09 14:01 ` Hans de Goede @ 2020-03-10 1:44 ` Lu Baolu -1 siblings, 0 replies; 16+ messages in thread From: Lu Baolu @ 2020-03-10 1:44 UTC (permalink / raw) To: Hans de Goede, David Woodhouse, Joerg Roedel; +Cc: iommu, stable On 2020/3/9 22:01, 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. > 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 warn_invalid_dmar() > + 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("...") calls, with > pr_warn(FW_BUG "...") + add_taint(TAINT_FIRMWARE_WORKAROUND, ...) calls > 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=1564895 > Fixes: e625b4a95d50 ("iommu/vt-d: Parse ANDD records") > Fixes: fd0c8894893c ("intel-iommu: Set a more specific taint flag for invalid BI > Cc: stable@vger.kernel.org > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Lu Baolu <baolu.lu@linux.intel.com> Best regards, baolu > --- > drivers/iommu/dmar.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c > index 071bb42bbbc5..87194a46cb0b 100644 > --- a/drivers/iommu/dmar.c > +++ b/drivers/iommu/dmar.c > @@ -440,12 +440,13 @@ static int __init dmar_parse_one_andd(struct acpi_dmar_header *header, > > /* Check for NUL termination within the designated length */ > if (strnlen(andd->device_name, header->length - 8) == header->length - 8) { > - WARN_TAINT(1, TAINT_FIRMWARE_WORKAROUND, > + pr_warn(FW_BUG > "Your BIOS is broken; ANDD object name is not NUL-terminated\n" > "BIOS vendor: %s; Ver: %s; Product Version: %s\n", > 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); > return -EINVAL; > } > pr_info("ANDD device: %x name: %s\n", andd->device_number, > @@ -471,14 +472,14 @@ static int dmar_parse_one_rhsa(struct acpi_dmar_header *header, void *arg) > return 0; > } > } > - WARN_TAINT( > - 1, TAINT_FIRMWARE_WORKAROUND, > + pr_warn(FW_BUG > "Your BIOS is broken; RHSA refers to non-existent DMAR unit at %llx\n" > "BIOS vendor: %s; Ver: %s; Product Version: %s\n", > drhd->reg_base_addr, > 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); > > return 0; > } > @@ -827,14 +828,14 @@ int __init dmar_table_init(void) > > static void warn_invalid_dmar(u64 addr, const char *message) > { > - WARN_TAINT_ONCE( > - 1, TAINT_FIRMWARE_WORKAROUND, > + pr_warn_once(FW_BUG > "Your BIOS is broken; DMAR reported at address %llx%s!\n" > "BIOS vendor: %s; Ver: %s; Product Version: %s\n", > addr, message, > 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); > } > > static int __ref > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] iommu/vt-d: dmar: replace WARN_TAINT with pr_warn + add_taint @ 2020-03-10 1:44 ` Lu Baolu 0 siblings, 0 replies; 16+ messages in thread From: Lu Baolu @ 2020-03-10 1:44 UTC (permalink / raw) To: Hans de Goede, David Woodhouse, Joerg Roedel; +Cc: baolu.lu, iommu, stable On 2020/3/9 22:01, 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. > 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 warn_invalid_dmar() > + 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("...") calls, with > pr_warn(FW_BUG "...") + add_taint(TAINT_FIRMWARE_WORKAROUND, ...) calls > 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=1564895 > Fixes: e625b4a95d50 ("iommu/vt-d: Parse ANDD records") > Fixes: fd0c8894893c ("intel-iommu: Set a more specific taint flag for invalid BI > Cc: stable@vger.kernel.org > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Lu Baolu <baolu.lu@linux.intel.com> Best regards, baolu > --- > drivers/iommu/dmar.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c > index 071bb42bbbc5..87194a46cb0b 100644 > --- a/drivers/iommu/dmar.c > +++ b/drivers/iommu/dmar.c > @@ -440,12 +440,13 @@ static int __init dmar_parse_one_andd(struct acpi_dmar_header *header, > > /* Check for NUL termination within the designated length */ > if (strnlen(andd->device_name, header->length - 8) == header->length - 8) { > - WARN_TAINT(1, TAINT_FIRMWARE_WORKAROUND, > + pr_warn(FW_BUG > "Your BIOS is broken; ANDD object name is not NUL-terminated\n" > "BIOS vendor: %s; Ver: %s; Product Version: %s\n", > 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); > return -EINVAL; > } > pr_info("ANDD device: %x name: %s\n", andd->device_number, > @@ -471,14 +472,14 @@ static int dmar_parse_one_rhsa(struct acpi_dmar_header *header, void *arg) > return 0; > } > } > - WARN_TAINT( > - 1, TAINT_FIRMWARE_WORKAROUND, > + pr_warn(FW_BUG > "Your BIOS is broken; RHSA refers to non-existent DMAR unit at %llx\n" > "BIOS vendor: %s; Ver: %s; Product Version: %s\n", > drhd->reg_base_addr, > 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); > > return 0; > } > @@ -827,14 +828,14 @@ int __init dmar_table_init(void) > > static void warn_invalid_dmar(u64 addr, const char *message) > { > - WARN_TAINT_ONCE( > - 1, TAINT_FIRMWARE_WORKAROUND, > + pr_warn_once(FW_BUG > "Your BIOS is broken; DMAR reported at address %llx%s!\n" > "BIOS vendor: %s; Ver: %s; Product Version: %s\n", > addr, message, > 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); > } > > static int __ref > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2] iommu/vt-d: dmar_parse_one_rmrr: replace WARN_TAINT with pr_warn + add_taint 2020-03-09 14:01 [PATCH 0/2] iommu/vt-d: replace WARN_TAINT with pr_warn + add_taint Hans de Goede @ 2020-03-09 14:01 ` Hans de Goede 2020-03-09 14:01 ` Hans de Goede 2020-03-10 10:42 ` [PATCH 0/2] iommu/vt-d: " Joerg Roedel 2 siblings, 0 replies; 16+ messages in thread From: Hans de Goede @ 2020-03-09 14:01 UTC (permalink / raw) To: David Woodhouse, Lu Baolu, Joerg Roedel Cc: Hans de Goede, iommu, Barret Rhoden, stable 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. 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) -- 2.25.1 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] iommu/vt-d: dmar_parse_one_rmrr: replace WARN_TAINT with pr_warn + add_taint @ 2020-03-09 14:01 ` Hans de Goede 0 siblings, 0 replies; 16+ messages in thread From: Hans de Goede @ 2020-03-09 14:01 UTC (permalink / raw) To: David Woodhouse, Lu Baolu, Joerg Roedel Cc: Hans de Goede, iommu, Barret Rhoden, stable 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. 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) -- 2.25.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] iommu/vt-d: dmar_parse_one_rmrr: replace WARN_TAINT with pr_warn + add_taint 2020-03-09 14:01 ` Hans de Goede @ 2020-03-09 15:57 ` Barret Rhoden -1 siblings, 0 replies; 16+ messages in thread From: Barret Rhoden via iommu @ 2020-03-09 15:57 UTC (permalink / raw) To: Hans de Goede, David Woodhouse, Lu Baolu, Joerg Roedel; +Cc: iommu, stable 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. 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. 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." Thanks, Barret > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] iommu/vt-d: dmar_parse_one_rmrr: replace WARN_TAINT with pr_warn + add_taint @ 2020-03-09 15:57 ` Barret Rhoden 0 siblings, 0 replies; 16+ messages in thread From: Barret Rhoden @ 2020-03-09 15:57 UTC (permalink / raw) To: Hans de Goede, David Woodhouse, Lu Baolu, Joerg Roedel; +Cc: iommu, stable 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. 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. 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." Thanks, Barret > 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) > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] iommu/vt-d: dmar_parse_one_rmrr: replace WARN_TAINT with pr_warn + add_taint 2020-03-09 15:57 ` Barret Rhoden @ 2020-03-09 16:01 ` Hans de Goede -1 siblings, 0 replies; 16+ messages in thread From: Hans de Goede @ 2020-03-09 16:01 UTC (permalink / raw) To: Barret Rhoden, David Woodhouse, Lu Baolu, Joerg Roedel; +Cc: iommu, stable 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) >> > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] iommu/vt-d: dmar_parse_one_rmrr: replace WARN_TAINT with pr_warn + add_taint @ 2020-03-09 16:01 ` Hans de Goede 0 siblings, 0 replies; 16+ messages in thread From: Hans de Goede @ 2020-03-09 16:01 UTC (permalink / raw) To: Barret Rhoden, David Woodhouse, Lu Baolu, Joerg Roedel; +Cc: iommu, stable 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) >> > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] iommu/vt-d: dmar_parse_one_rmrr: replace WARN_TAINT with pr_warn + add_taint 2020-03-09 16:01 ` Hans de Goede @ 2020-03-09 16:11 ` Barret Rhoden -1 siblings, 0 replies; 16+ messages in thread From: Barret Rhoden via iommu @ 2020-03-09 16:11 UTC (permalink / raw) To: Hans de Goede, David Woodhouse, Lu Baolu, Joerg Roedel; +Cc: iommu, stable 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] iommu/vt-d: dmar_parse_one_rmrr: replace WARN_TAINT with pr_warn + add_taint @ 2020-03-09 16:11 ` Barret Rhoden 0 siblings, 0 replies; 16+ messages in thread From: Barret Rhoden @ 2020-03-09 16:11 UTC (permalink / raw) To: Hans de Goede, David Woodhouse, Lu Baolu, Joerg Roedel; +Cc: iommu, stable 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) >>> >> > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] iommu/vt-d: dmar_parse_one_rmrr: replace WARN_TAINT with pr_warn + add_taint 2020-03-09 14:01 ` Hans de Goede @ 2020-03-10 1:44 ` Lu Baolu -1 siblings, 0 replies; 16+ messages in thread From: Lu Baolu @ 2020-03-10 1:44 UTC (permalink / raw) To: Hans de Goede, David Woodhouse, Joerg Roedel; +Cc: iommu, Barret Rhoden, stable On 2020/3/9 22:01, 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. > 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> Acked-by: Lu Baolu <baolu.lu@linux.intel.com> Best regards, baolu > --- > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] iommu/vt-d: dmar_parse_one_rmrr: replace WARN_TAINT with pr_warn + add_taint @ 2020-03-10 1:44 ` Lu Baolu 0 siblings, 0 replies; 16+ messages in thread From: Lu Baolu @ 2020-03-10 1:44 UTC (permalink / raw) To: Hans de Goede, David Woodhouse, Joerg Roedel Cc: baolu.lu, iommu, Barret Rhoden, stable On 2020/3/9 22:01, 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. > 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> Acked-by: Lu Baolu <baolu.lu@linux.intel.com> Best regards, baolu > --- > 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) > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] iommu/vt-d: replace WARN_TAINT with pr_warn + add_taint 2020-03-09 14:01 [PATCH 0/2] iommu/vt-d: replace WARN_TAINT with pr_warn + add_taint Hans de Goede 2020-03-09 14:01 ` Hans de Goede 2020-03-09 14:01 ` Hans de Goede @ 2020-03-10 10:42 ` Joerg Roedel 2 siblings, 0 replies; 16+ messages in thread From: Joerg Roedel @ 2020-03-10 10:42 UTC (permalink / raw) To: Hans de Goede; +Cc: iommu, David Woodhouse On Mon, Mar 09, 2020 at 03:01:36PM +0100, Hans de Goede wrote: > Can we please get these 2 patches queued up as fixes for 5.6-rc# ? Applied both for v5.6, thanks. > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-03-10 10:42 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-03-09 14:01 [PATCH 0/2] iommu/vt-d: replace WARN_TAINT with pr_warn + add_taint Hans de Goede 2020-03-09 14:01 ` [PATCH 1/2] iommu/vt-d: dmar: " Hans de Goede 2020-03-09 14:01 ` Hans de Goede 2020-03-10 1:44 ` Lu Baolu 2020-03-10 1:44 ` Lu Baolu 2020-03-09 14:01 ` [PATCH 2/2] iommu/vt-d: dmar_parse_one_rmrr: " Hans de Goede 2020-03-09 14:01 ` Hans de Goede 2020-03-09 15:57 ` Barret Rhoden via iommu 2020-03-09 15:57 ` Barret Rhoden 2020-03-09 16:01 ` Hans de Goede 2020-03-09 16:01 ` Hans de Goede 2020-03-09 16:11 ` Barret Rhoden via iommu 2020-03-09 16:11 ` Barret Rhoden 2020-03-10 1:44 ` Lu Baolu 2020-03-10 1:44 ` Lu Baolu 2020-03-10 10:42 ` [PATCH 0/2] iommu/vt-d: " Joerg Roedel
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.