linux-kernel - Re: [PATCH] platform/x86: wmi: Lower verbosity of some duplicate GUID messages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <12b98ed4-6bc3-455c-3b90-a159d811147d@amd.com>
Date:   Mon, 29 Aug 2022 07:17:48 -0500
From:   Mario Limonciello <mario.limonciello@....com>
To:     Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>
Cc:     platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform/x86: wmi: Lower verbosity of some duplicate GUID
 messages

On 8/29/22 06:45, Hans de Goede wrote:
> Hi Mario,
> 
> On 8/26/22 19:00, Mario Limonciello wrote:
>> The WMI subsystem in the kernel currently tracks WMI devices by
>> a GUID string not by ACPI device.  The GUID used by the `wmi-bmof`
>> module however is available from many devices on nearly every machine.
>>
>> This originally was though to be a bug, but as it happens on most
>> machines it is a design mistake.  As there isn't an active need to
>> get the binary from each of the `wmi-bmof` device, special case it
>> and lower the message to debugging.  This will help to identify if
>> there are other duplicate GUIDs in the wild.
>>
>> If there are and the information contained in them is desirable it
>> may be worth considering a design change to the WMI subsystem to
>> access those.
>>
>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2017%2F12%2F8%2F913&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7Ce38feb41da464767725808da89b3efcc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637973703162395560%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=sGh1bVTcO7vXOF6%2BwibhS7nbSiH3aEEdVNGfanKkGF8%3D&amp;reserved=0
>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
> 
> I am a bit surprised by this patch. I though that there was
> consensus that the right thing to do here is actually create
> wmi-bus devices for the duplicate WMI-ids adding a numbered
> postfix to the extra devices (lets not add the postfix
> to the first device for each WMI GUID as some userspace
> code / scripts may depend on the sysfs paths not changing).
> 
> IMHO registering wmi-bus devices for all the WMI devices
> in the ACPI table would be the right thing to do ?

I don't disagree it's the correct eventual direction, but I looked at it 
and it seems to be a much larger overhaul because that means drivers 
would also need to be able to specify which ACPI device they're 
intending on interacting with from wmi.c rather than just a GUID string.

So before going down that path I think it's best to understand if it 
really is just wmi-bmof causing these cases or more cases (low priority 
IMO) or if there really is a strong need for the overhaul.

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
>> ---
>>   drivers/platform/x86/wmi-bmof.c |  2 --
>>   drivers/platform/x86/wmi.c      | 10 ++++++++--
>>   include/linux/wmi.h             |  2 ++
>>   3 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/wmi-bmof.c b/drivers/platform/x86/wmi-bmof.c
>> index 80137afb9753..af46e9e03376 100644
>> --- a/drivers/platform/x86/wmi-bmof.c
>> +++ b/drivers/platform/x86/wmi-bmof.c
>> @@ -18,8 +18,6 @@
>>   #include <linux/types.h>
>>   #include <linux/wmi.h>
>>   
>> -#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910"
>> -
>>   struct bmof_priv {
>>   	union acpi_object *bmofdata;
>>   	struct bin_attribute bmof_bin_attr;
>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>> index aed293b5af81..d7a1f4bf443b 100644
>> --- a/drivers/platform/x86/wmi.c
>> +++ b/drivers/platform/x86/wmi.c
>> @@ -1157,6 +1157,9 @@ static void wmi_free_devices(struct acpi_device *device)
>>   static bool guid_already_parsed(struct acpi_device *device, const guid_t *guid)
>>   {
>>   	struct wmi_block *wblock;
>> +	guid_t guid_wmi_bmof;
>> +
>> +	guid_parse(WMI_BMOF_GUID, &guid_wmi_bmof);
>>   
>>   	list_for_each_entry(wblock, &wmi_block_list, list) {
>>   		if (guid_equal(&wblock->gblock.guid, guid)) {
>> @@ -1166,8 +1169,11 @@ static bool guid_already_parsed(struct acpi_device *device, const guid_t *guid)
>>   			 * we need to suppress GUIDs that are unique on a
>>   			 * given node but duplicated across nodes.
>>   			 */
>> -			dev_warn(&device->dev, "duplicate WMI GUID %pUL (first instance was on %s)\n",
>> -				 guid, dev_name(&wblock->acpi_device->dev));
>> +			if (guid_equal(guid, &guid_wmi_bmof))
>> +				dev_dbg(&device->dev, "duplicate WMI-BMOF GUID found\n");
>> +			else
>> +				dev_warn(&device->dev, "duplicate WMI GUID %pUL (first instance was on %s)\n",
>> +					 guid, dev_name(&wblock->acpi_device->dev));
>>   			return true;
>>   		}
>>   	}
>> diff --git a/include/linux/wmi.h b/include/linux/wmi.h
>> index b88d7b58e61e..59acdceb4411 100644
>> --- a/include/linux/wmi.h
>> +++ b/include/linux/wmi.h
>> @@ -13,6 +13,8 @@
>>   #include <linux/mod_devicetable.h>
>>   #include <uapi/linux/wmi.h>
>>   
>> +#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910"
>> +
>>   struct wmi_device {
>>   	struct device dev;
>>   
>