Re: [BUG] [BISECTED] [CORRECTION] systemd-devd triggers kernel memleak apparently in drivers/core/dd.c: driver_register()

From: Hans de Goede
Date: Wed Mar 29 2023 - 11:46:58 EST

Next message: Doug Anderson: "Re: [PATCH v14 14/14] drm/msm/dp: set self refresh aware based on PSR support"
Previous message: Bobby Eshleman: "Re: [PATCH net v2] virtio/vsock: fix leaks due to missing skb owner"
In reply to: Mirsad Goran Todorovac: "Re: [BUG] [BISECTED] [CORRECTION] systemd-devd triggers kernel memleak apparently in drivers/core/dd.c: driver_register()"
Next in thread: Mark Pearson: "Re: [BUG] [BISECTED] [CORRECTION] systemd-devd triggers kernel memleak apparently in drivers/core/dd.c: driver_register()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On 3/29/23 16:18, Mirsad Goran Todorovac wrote:
> On 29.3.2023. 15:35, Thomas Weißschuh wrote:
>>
>> 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.)
>
> Yes, thank you, Thomas, I am just building with the following patch:
>
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index c816646eb661..e8c28f4f5a71 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -1469,6 +1469,7 @@ static int tlmi_analyze(void)
>                                                         kstrndup(optstart, optend - optstart,
>                                                                         GFP_KERNEL);
>                                 }
> +                               kfree(item);
>                         }
>                 }
>                 /*
>
> You were 3 minutes faster ;-)
>
> The build with this patch is finished. Apparently, that was the culprit, for now
> I get:
>
> [root@pc-mtodorov marvin]# cat /sys/kernel/debug/kmemleak
> unreferenced object 0xffff8fa859c89f10 (size 16):
> comm "kworker/u12:5", pid 369, jiffies 4294897651 (age 91.724s)
> hex dump (first 16 bytes):
>     6d 65 6d 73 74 69 63 6b 30 00 cc cc cc cc cc cc memstick0.......
> backtrace:
>     [<ffffffff8ef0a8fc>] slab_post_alloc_hook+0x8c/0x3e0
>     [<ffffffff8ef12289>] __kmem_cache_alloc_node+0x1d9/0x2a0
>     [<ffffffff8ee85719>] __kmalloc_node_track_caller+0x59/0x1f0
>     [<ffffffff8ee749da>] kstrdup+0x3a/0x70
>     [<ffffffff8ee74a4c>] kstrdup_const+0x2c/0x40
>     [<ffffffff8f2c0a3c>] kvasprintf_const+0x7c/0xb0
>     [<ffffffff8fc5f427>] kobject_set_name_vargs+0x27/0xa0
>     [<ffffffff8f5a79f7>] dev_set_name+0x57/0x80
>     [<ffffffffc0cd2f0f>] memstick_check+0x10f/0x3b0 [memstick]
>     [<ffffffff8ebd41d0>] process_one_work+0x250/0x600
>     [<ffffffff8ebd45d8>] worker_thread+0x48/0x3a0
>     [<ffffffff8ebdfdef>] kthread+0x10f/0x140
>     [<ffffffff8ea03089>] ret_from_fork+0x29/0x50
> unreferenced object 0xffff8fa859c89d90 (size 16):
> comm "kworker/u12:5", pid 369, jiffies 4294897656 (age 91.704s)
> hex dump (first 16 bytes):
>     6d 65 6d 73 74 69 63 6b 30 00 cc cc cc cc cc cc memstick0.......
> backtrace:
>     [<ffffffff8ef0a8fc>] slab_post_alloc_hook+0x8c/0x3e0
>     [<ffffffff8ef12289>] __kmem_cache_alloc_node+0x1d9/0x2a0
>     [<ffffffff8ee85719>] __kmalloc_node_track_caller+0x59/0x1f0
>     [<ffffffff8ee749da>] kstrdup+0x3a/0x70
>     [<ffffffff8ee74a4c>] kstrdup_const+0x2c/0x40
>     [<ffffffff8f2c0a3c>] kvasprintf_const+0x7c/0xb0
>     [<ffffffff8fc5f427>] kobject_set_name_vargs+0x27/0xa0
>     [<ffffffff8f5a79f7>] dev_set_name+0x57/0x80
>     [<ffffffffc0cd2f0f>] memstick_check+0x10f/0x3b0 [memstick]
>     [<ffffffff8ebd41d0>] process_one_work+0x250/0x600
>     [<ffffffff8ebd45d8>] worker_thread+0x48/0x3a0
>     [<ffffffff8ebdfdef>] kthread+0x10f/0x140
>     [<ffffffff8ea03089>] ret_from_fork+0x29/0x50
> [root@pc-mtodorov marvin]#
>
> So, the "tlmi_setting" memory leak appears to be fixed by this diff.
>
> The next step is to add Armin-suggested 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);
>
> and Thomas' correction for the return type of the tlmi_setting() function:
>
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 86b33b74519be..c924e9e4a6a5b 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -1353,7 +1353,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) &&
> @@ -1390,8 +1389,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;
>
> A build on top of 6.3-rc4+ fcd476ea6a88 commit is on the way, with all three included.

Good work on catching these issues, thank you all for your work on this.

I assume that these fixes will be posted as a proper 3 patch
patch-series (one patch per fix) once you are done testing?

Regards,

Hans

Next message: Doug Anderson: "Re: [PATCH v14 14/14] drm/msm/dp: set self refresh aware based on PSR support"
Previous message: Bobby Eshleman: "Re: [PATCH net v2] virtio/vsock: fix leaks due to missing skb owner"
In reply to: Mirsad Goran Todorovac: "Re: [BUG] [BISECTED] [CORRECTION] systemd-devd triggers kernel memleak apparently in drivers/core/dd.c: driver_register()"
Next in thread: Mark Pearson: "Re: [BUG] [BISECTED] [CORRECTION] systemd-devd triggers kernel memleak apparently in drivers/core/dd.c: driver_register()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]