Re: [BUG] [BISECTED] [CORRECTION] systemd-devd triggers kernel memleak apparently in drivers/core/dd.c: driver_register()
From: Thomas Weißschuh
Date: Wed Mar 29 2023 - 09:35:19 EST
Mar 29, 2023 08:31:31 Mirsad Goran Todorovac <mirsad.todorovac@xxxxxxxxxxxx>:
> Hi, again,
>
> NOTE: I forgot to rewind to the first bad commit. So please ignore
> the previous email.
>
> On 29.3.2023. 15:22, Mirsad Goran Todorovac wrote:
>> Hi, Armin, Mr. Greg,
>> On 29.3.2023. 10:13, Mirsad Goran Todorovac wrote:
>>> On 28.3.2023. 21:55, Armin Wolf wrote:
>>>> Am 28.03.23 um 21:06 schrieb Mirsad Goran Todorovac:
>>>>
>>>>> On 3/28/2023 6:53 PM, Armin Wolf wrote:
>>>>>> …
>>>>>
>>>>> Hi Armin,
>>>>>
>>>>> I tried your suggestion, and though it is an obvious improvement and a
>>>>> leak fix, this
>>>>> was not the one we were searching for.
>>>>>
>>>>> I tested the following patch:
>>>>>
>>>>> diff --git a/drivers/platform/x86/think-lmi.c
>>>>> b/drivers/platform/x86/think-lmi.c
>>>>> index c816646eb661..1e77ecb0cba8 100644
>>>>> --- a/drivers/platform/x86/think-lmi.c
>>>>> +++ b/drivers/platform/x86/think-lmi.c
>>>>> @@ -929,8 +929,10 @@ static ssize_t current_value_show(struct kobject
>>>>> *kobj, struct kobj_attribute *a
>>>>>
>>>>> /* validate and split from `item,value` -> `value` */
>>>>> value = strpbrk(item, ",");
>>>>> - if (!value || value == item || !strlen(value + 1))
>>>>> + if (!value || value == item || !strlen(value + 1)) {
>>>>> + kfree(item);
>>>>> return -EINVAL;
>>>>> + }
>>>>>
>>>>> ret = sysfs_emit(buf, "%s\n", value + 1);
>>>>> kfree(item);
>>>>>
>>>>> (I would also object to the use of strlen() here, for it is inherently
>>>>> insecure
>>>>> against SEGFAULT in kernel space.)
>>>>>
>>>>> I still get:
>>>>> [root@pc-mtodorov marvin]# uname -rms
>>>>> Linux 6.3.0-rc4-armin-patch-00025-g3a93e40326c8-dirty x86_64
>>>>> [root@pc-mtodorov marvin]# cat /sys/kernel/debug/kmemleak [edited]
>>>>> unreferenced object 0xffff8eb008ef9260 (size 96):
>>>>> comm "systemd-udevd", pid 771, jiffies 4294896499 (age 74.880s)
>>>>> hex dump (first 32 bytes):
>>>>> 53 65 72 69 61 6c 50 6f 72 74 31 41 64 64 72 65 SerialPort1Addre
>>>>> 73 73 2c 33 46 38 2f 49 52 51 34 3b 5b 4f 70 74 ss,3F8/IRQ4;[Opt
>>>>> backtrace:
>>>>> [<ffffffff9eafb26c>] slab_post_alloc_hook+0x8c/0x3e0
>>>>> [<ffffffff9eb02b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
>>>>> [<ffffffff9ea773c9>] __kmalloc_node_track_caller+0x59/0x180
>>>>> [<ffffffff9ea66a1a>] kstrdup+0x3a/0x70
>>>>> [<ffffffffc0eef9aa>] tlmi_extract_output_string.isra.0+0x2a/0x60
>>>>> [think_lmi]
>>>>> [<ffffffffc0eefb64>] tlmi_setting.constprop.4+0x54/0x90 [think_lmi]
>>>>> [<ffffffffc0ef02c1>] tlmi_probe+0x591/0xba0 [think_lmi]
>>>>> [<ffffffffc0629c53>] wmi_dev_probe+0x163/0x230 [wmi]
>>>>> [<ffffffff9f1987eb>] really_probe+0x17b/0x3d0
>>>>> [<ffffffff9f198ad4>] __driver_probe_device+0x84/0x190
>>>>> [<ffffffff9f198c14>] driver_probe_device+0x24/0xc0
>>>>> [<ffffffff9f198ed2>] __driver_attach+0xc2/0x190
>>>>> [<ffffffff9f195ab1>] bus_for_each_dev+0x81/0xd0
>>>>> [<ffffffff9f197c62>] driver_attach+0x22/0x30
>>>>> [<ffffffff9f197354>] bus_add_driver+0x1b4/0x240
>>>>> [<ffffffff9f19a0a2>] driver_register+0x62/0x120
>>>>> unreferenced object 0xffff8eb018ddbb40 (size 64):
>>>>> comm "systemd-udevd", pid 771, jiffies 4294896528 (age 74.780s)
>>>>> hex dump (first 32 bytes):
>>>>> 55 53 42 50 6f 72 74 41 63 63 65 73 73 2c 45 6e USBPortAccess,En
>>>>> 61 62 6c 65 64 3b 5b 4f 70 74 69 6f 6e 61 6c 3a abled;[Optional:
>>>>> backtrace:
>>>>> [<ffffffff9eafb26c>] slab_post_alloc_hook+0x8c/0x3e0
>>>>> [<ffffffff9eb02b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
>>>>> [<ffffffff9ea773c9>] __kmalloc_node_track_caller+0x59/0x180
>>>>> [<ffffffff9ea66a1a>] kstrdup+0x3a/0x70
>>>>> [<ffffffffc0eef9aa>] tlmi_extract_output_string.isra.0+0x2a/0x60
>>>>> [think_lmi]
>>>>> [<ffffffffc0eefb64>] tlmi_setting.constprop.4+0x54/0x90 [think_lmi]
>>>>> [<ffffffffc0ef02c1>] tlmi_probe+0x591/0xba0 [think_lmi]
>>>>> [<ffffffffc0629c53>] wmi_dev_probe+0x163/0x230 [wmi]
>>>>> [<ffffffff9f1987eb>] really_probe+0x17b/0x3d0
>>>>> [<ffffffff9f198ad4>] __driver_probe_device+0x84/0x190
>>>>> [<ffffffff9f198c14>] driver_probe_device+0x24/0xc0
>>>>> [<ffffffff9f198ed2>] __driver_attach+0xc2/0x190
>>>>> [<ffffffff9f195ab1>] bus_for_each_dev+0x81/0xd0
>>>>> [<ffffffff9f197c62>] driver_attach+0x22/0x30
>>>>> [<ffffffff9f197354>] bus_add_driver+0x1b4/0x240
>>>>> [<ffffffff9f19a0a2>] driver_register+0x62/0x120
>>>>> unreferenced object 0xffff8eb006fe2b40 (size 64):
>>>>> comm "systemd-udevd", pid 771, jiffies 4294896542 (age 74.724s)
>>>>> hex dump (first 32 bytes):
>>>>> 55 53 42 42 49 4f 53 53 75 70 70 6f 72 74 2c 45 USBBIOSSupport,E
>>>>> 6e 61 62 6c 65 64 3b 5b 4f 70 74 69 6f 6e 61 6c nabled;[Optional
>>>>> backtrace:
>>>>> [<ffffffff9eafb26c>] slab_post_alloc_hook+0x8c/0x3e0
>>>>> [<ffffffff9eb02b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
>>>>> [<ffffffff9ea773c9>] __kmalloc_node_track_caller+0x59/0x180
>>>>> [<ffffffff9ea66a1a>] kstrdup+0x3a/0x70
>>>>> [<ffffffffc0eef9aa>] tlmi_extract_output_string.isra.0+0x2a/0x60
>>>>> [think_lmi]
>>>>> [<ffffffffc0eefb64>] tlmi_setting.constprop.4+0x54/0x90 [think_lmi]
>>>>> [<ffffffffc0ef02c1>] tlmi_probe+0x591/0xba0 [think_lmi]
>>>>> [<ffffffffc0629c53>] wmi_dev_probe+0x163/0x230 [wmi]
>>>>> [<ffffffff9f1987eb>] really_probe+0x17b/0x3d0
>>>>> [<ffffffff9f198ad4>] __driver_probe_device+0x84/0x190
>>>>> [<ffffffff9f198c14>] driver_probe_device+0x24/0xc0
>>>>> [<ffffffff9f198ed2>] __driver_attach+0xc2/0x190
>>>>> [<ffffffff9f195ab1>] bus_for_each_dev+0x81/0xd0
>>>>> [<ffffffff9f197c62>] driver_attach+0x22/0x30
>>>>> [<ffffffff9f197354>] bus_add_driver+0x1b4/0x240
>>>>> [<ffffffff9f19a0a2>] driver_register+0x62/0x120
>>>>>
>>>>> There are currently 84 wmi_dev_probe leaks, sized mostly 64 bytes, and
>>>>> one 96 and two 192 bytes.
>>>>>
>>>>> I also cannot figure out the mechanism by which current_value_show()
>>>>> is called, when it is static?
>>>>>
>>>>> Any idea?
>>>>>
>>>>> Thanks.
>>>>>
>>>>> Best regards,
>>>>> Mirsad
>>>>>
>>>> Can you tell me how many BIOS settings think-lmi provides on your machine? Because according to the stacktrace,
>>>> the other place where the leak could have occurred is inside tlmi_analyze(), which calls tlmi_setting().
>>>
>>> Yes, Sir!
>>>
>>> I think these could be the ones you need (totaling 83, which is close to 82 systemd-udevd leaks):
>>>
>>> [root@pc-mtodorov marvin]# ls -l /sys/devices/virtual/firmware-attributes/thinklmi/attributes | grep ^d
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 AfterPowerLoss
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 AlarmDate(MM\DD\YYYY)
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 AlarmDayofWeek
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 AlarmTime(HH:MM:SS)
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 ASPMSupport
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 AutomaticBootSequence
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 BIOSPasswordAtBootDeviceList
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 BIOSPasswordAtReboot
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 BIOSPasswordAtUnattendedBoot
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 BootMode
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 BootPriority
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 BootUpNum-LockStatus
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 C1ESupport
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 CardReader
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 ClearTCGSecurityFeature
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 ComputraceModuleActivation
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 ConfigurationChangeDetection
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 ConfigureSATAas
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 CoreMulti-Processing
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 CoverTamperDetected
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 CSM
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 CStateSupport
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 DeviceGuard
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 DustShieldAlert
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 EISTSupport
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 EnhancedPowerSavingMode
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 ErrorBootSequence
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 Friday
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 FrontUSBPorts
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 HardDiskPre-delay
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 Intel(R)SGXControl
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 InternalSpeaker
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 Monday
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 OnboardAudioController
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 OnboardEthernetController
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 OptionKeysDisplay
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 OptionKeysDisplayStyle
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 OSOptimizedDefaults
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 PasswordCountExceededError
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 PCIe16xSlotSpeed
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 PCIe1xSlot1Speed
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 PrimaryBootSequence
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 PXEIPV4NetworkStack
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 PXEIPV6NetworkStack
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 PXEOptionROM
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 RearUSBPorts
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 RequireAdmin.Pass.whenFlashing
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 RequireHDPonSystemBoot
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 SATAController
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 SATADrive1
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 SATADrive2
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 Saturday
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 SecureBoot
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 SecureRollBackPrevention
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 SecurityChip
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 SelectActiveVideo
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 SerialPort1Address
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 SmartUSBProtection
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 StartupDeviceMenuPrompt
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 StartupSequence
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 Sunday
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 Thursday
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 Tuesday
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 TurboMode
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 USBBIOSSupport
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 USBEnumerationDelay
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 USBPort1
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 USBPort2
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 USBPort3
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 USBPort4
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 USBPort5
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 USBPort6
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 USBPort7
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 USBPort8
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 USBPortAccess
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 UserDefinedAlarmTime(HH:MM:SS)
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 VirtualizationTechnology
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 VTdFeature
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 WakefromSerialPortRing
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 WakeOnLAN
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 WakeUponAlarm
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 Wednesday
>>> drwxr-xr-x 2 root root 0 Mar 29 08:24 WindowsUEFIFirmwareUpdate
>>> [root@pc-mtodorov marvin]# ls -l /sys/devices/virtual/firmware-attributes/thinklmi/attributes | grep ^d | wc -l
>>> 83
>>> [root@pc-mtodorov marvin]#
>>>
>>>> However, i have no idea on how *info is somehow leaked, it has to happen inside the for-loop tween the call
>>>> to tlmi_setting() and strreplace(), because otherwise the strings would not contain the "/" character.
>>>
>>> I see. It is the line 1404.
>>>
>>>> Can you check if the problem is somehow solved by applying the following commit from the platform-drivers-x86
>>>> for-next branch:
>>>> da62908efe80 ("platform/x86: think-lmi: Properly interpret return value of tlmi_setting")
>>>
>>> It could possibly be that. But I do not recall seeing these messages in 6.3-rc3 ...
>>>
>>> ...
>>>
>>> Unfortunately, the build with Thomas' patch you referred to did not work:
>>>
>>> unreferenced object 0xffff9dff4d28bbc8 (size 192):
>>> comm "systemd-udevd", pid 769, jiffies 4294897473 (age 85.700s)
>>> hex dump (first 32 bytes):
>>> 50 72 69 6d 61 72 79 42 6f 6f 74 53 65 71 75 65 PrimaryBootSeque
>>> 6e 63 65 2c 4d 2e 32 20 44 72 69 76 65 20 31 3a nce,M.2 Drive 1:
>>> backtrace:
>>> [<ffffffffa48fb26c>] slab_post_alloc_hook+0x8c/0x3e0
>>> [<ffffffffa4902b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
>>> [<ffffffffa48773c9>] __kmalloc_node_track_caller+0x59/0x180
>>> [<ffffffffa4866a1a>] kstrdup+0x3a/0x70
>>> [<ffffffffc0c7f9aa>] tlmi_extract_output_string.isra.0+0x2a/0x60 [think_lmi]
>>> [<ffffffffc0c7fb64>] tlmi_setting.constprop.4+0x54/0x90 [think_lmi]
>>> [<ffffffffc0c802c1>] tlmi_probe+0x591/0xba0 [think_lmi]
>>> [<ffffffffc03c9c53>] wmi_dev_probe+0x163/0x230 [wmi]
>>> [<ffffffffa4f987eb>] really_probe+0x17b/0x3d0
>>> [<ffffffffa4f98ad4>] __driver_probe_device+0x84/0x190
>>> [<ffffffffa4f98c14>] driver_probe_device+0x24/0xc0
>>> [<ffffffffa4f98ed2>] __driver_attach+0xc2/0x190
>>> [<ffffffffa4f95ab1>] bus_for_each_dev+0x81/0xd0
>>> [<ffffffffa4f97c62>] driver_attach+0x22/0x30
>>> [<ffffffffa4f97354>] bus_add_driver+0x1b4/0x240
>>> [<ffffffffa4f9a0a2>] driver_register+0x62/0x120
>>> unreferenced object 0xffff9dff4d28a008 (size 192):
>>> comm "systemd-udevd", pid 769, jiffies 4294897517 (age 85.540s)
>>> hex dump (first 32 bytes):
>>> 45 72 72 6f 72 42 6f 6f 74 53 65 71 75 65 6e 63 ErrorBootSequenc
>>> 65 2c 4e 65 74 77 6f 72 6b 20 31 3a 4d 2e 32 20 e,Network 1:M.2
>>> backtrace:
>>> [<ffffffffa48fb26c>] slab_post_alloc_hook+0x8c/0x3e0
>>> [<ffffffffa4902b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
>>> [<ffffffffa48773c9>] __kmalloc_node_track_caller+0x59/0x180
>>> [<ffffffffa4866a1a>] kstrdup+0x3a/0x70
>>> [<ffffffffc0c7f9aa>] tlmi_extract_output_string.isra.0+0x2a/0x60 [think_lmi]
>>> [<ffffffffc0c7fb64>] tlmi_setting.constprop.4+0x54/0x90 [think_lmi]
>>> [<ffffffffc0c802c1>] tlmi_probe+0x591/0xba0 [think_lmi]
>>> [<ffffffffc03c9c53>] wmi_dev_probe+0x163/0x230 [wmi]
>>> [<ffffffffa4f987eb>] really_probe+0x17b/0x3d0
>>> [<ffffffffa4f98ad4>] __driver_probe_device+0x84/0x190
>>> [<ffffffffa4f98c14>] driver_probe_device+0x24/0xc0
>>> [<ffffffffa4f98ed2>] __driver_attach+0xc2/0x190
>>> [<ffffffffa4f95ab1>] bus_for_each_dev+0x81/0xd0
>>> [<ffffffffa4f97c62>] driver_attach+0x22/0x30
>>> [<ffffffffa4f97354>] bus_add_driver+0x1b4/0x240
>>> [<ffffffffa4f9a0a2>] driver_register+0x62/0x120
>>> [root@pc-mtodorov marvin]# uname -rms
>>> Linux 6.3.0-rc4-armin+tw-patch-00025-g3a93e40326c8-dirty x86_64
>>> [root@pc-mtodorov marvin]#
>>>
>>> What was applied is:
>>>
>>> mtodorov@domac:~/linux/kernel/linux_torvalds$ git diff
>>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>>> index c816646eb661..9a3015f43aaf 100644
>>> --- a/drivers/platform/x86/think-lmi.c
>>> +++ b/drivers/platform/x86/think-lmi.c
>>> @@ -929,8 +929,10 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
>>>
>>> /* validate and split from `item,value` -> `value` */
>>> value = strpbrk(item, ",");
>>> - if (!value || value == item || !strlen(value + 1))
>>> + if (!value || value == item || !strlen(value + 1)) {
>>> + kfree(item);
>>> return -EINVAL;
>>> + }
>>>
>>> ret = sysfs_emit(buf, "%s\n", value + 1);
>>> kfree(item);
>>> @@ -1380,7 +1382,6 @@ static struct tlmi_pwd_setting *tlmi_create_auth(const char *pwd_type,
>>>
>>> static int tlmi_analyze(void)
>>> {
>>> - acpi_status status;
>>> int i, ret;
>>>
>>> if (wmi_has_guid(LENOVO_SET_BIOS_SETTINGS_GUID) &&
>>> @@ -1417,8 +1418,8 @@ static int tlmi_analyze(void)
>>> char *p;
>>>
>>> tlmi_priv.setting[i] = NULL;
>>> - status = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID);
>>> - if (ACPI_FAILURE(status))
>>> + ret = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID);
>>> + if (ret)
>>> break;
>>> if (!item)
>>> break;
>>>
>>>
>>>> Also current_value_show() is used by attr_current_val, the __ATTR_RW_MODE() macro arranges for that.
>>>
>>> Thanks.
>>>
>>> In this build:
>>>
>>> [root@pc-mtodorov marvin]# uname -rms
>>> Linux 6.3.0-rc34tests-00001-g6981739a967c x86_64
>>> [root@pc-mtodorov marvin]#
>>>
>>> ... the bug isn't present, so it might be something added recently:
>>>
>>> commit 8a02d70679fc1c434401863333c8ea7dbf201494
>>> Author: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
>>> Date: Sun Mar 19 20:32:21 2023 -0400
>>>
>>> platform/x86: think-lmi: Add possible_values for ThinkStation
>>>
>>> commit cf337f27f3bfc4aeab4954c468239fd6233c7638
>>> Author: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
>>> Date: Sun Mar 19 20:32:20 2023 -0400
>>>
>>> platform/x86: think-lmi: only display possible_values if available
>>>
>>> commit 45e21289bfc6e257885514790a8a8887da822d40
>>> Author: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
>>> Date: Sun Mar 19 20:32:19 2023 -0400
>>>
>>> platform/x86: think-lmi: use correct possible_values delimiters
>>>
>>> commit 583329dcf22e568a328a944f20427ccfc95dce01
>>> Author: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
>>> Date: Sun Mar 19 20:32:18 2023 -0400
>>>
>>> platform/x86: think-lmi: add missing type attribute
>>>
>>> I have CC:-ed the author of the commits.
>>>
>>> I can try bisect, but only after my day job.
>> I seem to have been right about the culprit commit.
>> Here is the bisect log:
>> mtodorov@domac:~/linux/kernel/linux_torvalds$ git bisect log
>> git bisect start '--' './drivers/platform/x86'
>> # good: [caa0708a81d6a2217c942959ef40d515ec1d3108] bootconfig: Change message if no bootconfig with CONFIG_BOOT_CONFIG_FORCE=y
>> git bisect good caa0708a81d6a2217c942959ef40d515ec1d3108
>> # bad: [8a02d70679fc1c434401863333c8ea7dbf201494] platform/x86: think-lmi: Add possible_values for ThinkStation
>> git bisect bad 8a02d70679fc1c434401863333c8ea7dbf201494
>> # good: [1a0009abfa7893b9cfcd3884658af1cbee6b26ce] platform: mellanox: mlx-platform: Initialize shift variable to 0
>> git bisect good 1a0009abfa7893b9cfcd3884658af1cbee6b26ce
>> # good: [b7c994f8c35e916e27c60803bb21457bc1373500] platform/x86 (gigabyte-wmi): Add support for A320M-S2H V2
>> git bisect good b7c994f8c35e916e27c60803bb21457bc1373500
>> # good: [45e21289bfc6e257885514790a8a8887da822d40] platform/x86: think-lmi: use correct possible_values delimiters
>> git bisect good 45e21289bfc6e257885514790a8a8887da822d40
>> # good: [cf337f27f3bfc4aeab4954c468239fd6233c7638] platform/x86: think-lmi: only display possible_values if available
>> git bisect good cf337f27f3bfc4aeab4954c468239fd6233c7638
>> # first bad commit: [8a02d70679fc1c434401863333c8ea7dbf201494] platform/x86: think-lmi: Add possible_values for ThinkStation
>> mtodorov@domac:~/linux/kernel/linux_torvalds$
>> Please see below.
>> Apparently, this commit broke something on my Lenovo box:
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> index e190fec26021..3f0641360251 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -941,9 +941,6 @@ static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute
>> {
>> struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> - if (!tlmi_priv.can_get_bios_selections)
>> - return -EOPNOTSUPP;
>> -
>> return sysfs_emit(buf, "%s\n", setting->possible_values);
>> }
>> @@ -1052,6 +1049,18 @@ static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 06
>> static struct kobj_attribute attr_type = __ATTR_RO(type);
>> +static umode_t attr_is_visible(struct kobject *kobj,
>> + struct attribute *attr, int n)
>> +{
>> + struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> +
>> + /* We don't want to display possible_values attributes if not available */
>> + if ((attr == &attr_possible_values.attr) && (!setting->possible_values))
>> + return 0;
>> +
>> + return attr->mode;
>> +}
>> +
>> static struct attribute *tlmi_attrs[] = {
>> &attr_displ_name.attr,
>> &attr_current_val.attr,
>> @@ -1061,6 +1070,7 @@ static struct attribute *tlmi_attrs[] = {
>> };
>> static const struct attribute_group tlmi_attr_group = {
>> + .is_visible = attr_is_visible,
>> .attrs = tlmi_attrs,
>> };
>> Hope this helps narrow down the problem.
>> Best regards,
>> Mirsad
>> Why aren't you looking at the wmi.c driver? That should be
>>>>>> …
>
> mtodorov@domac:~/linux/kernel/linux_torvalds$ git bisect log
> git bisect start '--' './drivers/platform/x86'
> # good: [caa0708a81d6a2217c942959ef40d515ec1d3108] bootconfig: Change message if no bootconfig with CONFIG_BOOT_CONFIG_FORCE=y
> git bisect good caa0708a81d6a2217c942959ef40d515ec1d3108
> # bad: [8a02d70679fc1c434401863333c8ea7dbf201494] platform/x86: think-lmi: Add possible_values for ThinkStation
> git bisect bad 8a02d70679fc1c434401863333c8ea7dbf201494
> # good: [1a0009abfa7893b9cfcd3884658af1cbee6b26ce] platform: mellanox: mlx-platform: Initialize shift variable to 0
> git bisect good 1a0009abfa7893b9cfcd3884658af1cbee6b26ce
> # good: [b7c994f8c35e916e27c60803bb21457bc1373500] platform/x86 (gigabyte-wmi): Add support for A320M-S2H V2
> git bisect good b7c994f8c35e916e27c60803bb21457bc1373500
> # good: [45e21289bfc6e257885514790a8a8887da822d40] platform/x86: think-lmi: use correct possible_values delimiters
> git bisect good 45e21289bfc6e257885514790a8a8887da822d40
> # good: [cf337f27f3bfc4aeab4954c468239fd6233c7638] platform/x86: think-lmi: only display possible_values if available
> git bisect good cf337f27f3bfc4aeab4954c468239fd6233c7638
> # first bad commit: [8a02d70679fc1c434401863333c8ea7dbf201494] platform/x86: think-lmi: Add possible_values for ThinkStation
> mtodorov@domac:~/linux/kernel/linux_torvalds$
>
> So the commit that triggered the bug on the Lenovo desktop box was:
>
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 3f0641360251..c816646eb661 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -1450,6 +1450,26 @@ static int tlmi_analyze(void)
> if (ret || !setting->possible_values)
> pr_info("Error retrieving possible values for %d : %s\n",
> i, setting->display_name);
> + } else {
> + /*
> + * Older Thinkstations don't support the bios_selections API.
> + * Instead they store this as a [Optional:Option1,Option2] section of the
> + * name string.
> + * Try and pull that out if it's available.
> + */
> + char *item, *optstart, *optend;
> +
> + if (!tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID)) {
> + optstart = strstr(item, "[Optional:");
> + if (optstart) {
> + optstart += strlen("[Optional:");
> + optend = strstr(optstart, "]");
> + if (optend)
> + setting->possible_values =
> + kstrndup(optstart, optend - optstart,
> + GFP_KERNEL);
I guess item needs to be freed here.
(Next week I have access to my Lenovo machine again.
I'll look at it then if it's not solved.)
> + }
> + }
> }
> /*
> * firmware-attributes requires that possible_values are separated by ';' but
>
> Thousand apologies, once again.
>
> Best regards,
> Mirsad
>
> --
> Mirsad Todorovac
> System engineer
> Faculty of Graphic Arts | Academy of Fine Arts
> University of Zagreb
> Republic of Croatia, the European Union
>
> Sistem inženjer
> Grafički fakultet | Akademija likovnih umjetnosti> Sveučilište u Zagrebu