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

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

[Hans de Goede]

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

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

[Hans de Goede]

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

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

[Hans de Goede]

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

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

[Barret Rhoden via iommu]

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

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

[Hans de Goede]

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

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

[Barret Rhoden via iommu]

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

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

[Lu Baolu]

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

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

[Lu Baolu]

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

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

[Joerg Roedel]

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