Messages in this thread Patch in this message | | | Date | Wed, 29 Mar 2023 15:31:16 +0200 | Subject | Re: [BUG] [BISECTED] [CORRECTION] systemd-devd triggers kernel memleak apparently in drivers/core/dd.c: driver_register() | From | Mirsad Goran Todorovac <> |
| |
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: >>>>> Am 28.03.23 um 14:44 schrieb Mirsad Todorovac: >>>>> >>>>>> On 3/28/23 14:17, Greg Kroah-Hartman wrote: >>>>>>> On Tue, Mar 28, 2023 at 02:08:06PM +0200, Mirsad Todorovac wrote: >>>>>>>> On 3/28/23 13:59, Mirsad Todorovac wrote: >>>>>>>> >>>>>>>>> On 3/28/23 13:28, Greg Kroah-Hartman wrote: >>>>>>>>>> On Tue, Mar 28, 2023 at 01:13:33PM +0200, Mirsad Todorovac wrote: >>>>>>>>>>> Hi all, >>>>>>>>>>> >>>>>>>>>>> Here is another kernel memory leak report, just as I thought we >>>>>>>>>>> have done with >>>>>>>>>>> them by the xhci patch by Mathias. >>>>>>>>>>> >>>>>>>>>>> The memory leaks were caught on an AlmaLinux 8.7 (CentOS) fork >>>>>>>>>>> system, running >>>>>>>>>>> on a Lenovo desktop box (see lshw.txt) and the newest Linux >>>>>>>>>>> kernel 6.3-rc4 commit >>>>>>>>>>> g3a93e40326c8 with Mathias' patch for a xhci systemd-devd >>>>>>>>>>> triggered leak. >>>>>>>>>>> >>>>>>>>>>> See: >>>>>>>>>>> <20230327095019.1017159-1-mathias.nyman@linux.intel.com> on LKML. >>>>>>>>>>> >>>>>>>>>>> This leak is also systemd-devd triggered, except for the >>>>>>>>>>> memstick_check() leaks >>>>>>>>>>> which I was unable to bisect due to the box not booting older >>>>>>>>>>> kernels (work in >>>>>>>>>>> progress). >>>>>>>>>>> >>>>>>>>>>> unreferenced object 0xffff88ad12392710 (size 96): >>>>>>>>>>> comm "systemd-udevd", pid 735, jiffies 4294896759 (age >>>>>>>>>>> 2257.568s) >>>>>>>>>>> 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: >>>>>>>>>>> [<ffffffffae8fb26c>] slab_post_alloc_hook+0x8c/0x3e0 >>>>>>>>>>> [<ffffffffae902b49>] __kmem_cache_alloc_node+0x1d9/0x2a0 >>>>>>>>>>> [<ffffffffae8773c9>] __kmalloc_node_track_caller+0x59/0x180 >>>>>>>>>>> [<ffffffffae866a1a>] kstrdup+0x3a/0x70 >>>>>>>>>>> [<ffffffffc0d839aa>] >>>>>>>>>>> tlmi_extract_output_string.isra.0+0x2a/0x60 [think_lmi] >>>>>>>>>>> [<ffffffffc0d83b64>] tlmi_setting.constprop.4+0x54/0x90 >>>>>>>>>>> [think_lmi] >>>>>>>>>>> [<ffffffffc0d842b1>] tlmi_probe+0x591/0xba0 [think_lmi] >>>>>>>>>>> [<ffffffffc051dc53>] wmi_dev_probe+0x163/0x230 [wmi] >>>>>>>>>> >>>>> Hi, >>>>> >>>>> this "SerialPort1Address" string looks like a BIOS setup option, and >>>>> indeed think_lmi allows for >>>>> changing BIOS setup options over sysfs. While looking at >>>>> current_value_show() in think-lmi.c, i noticed >>>>> that "item" holds a string which is allocated with kstrdup(), so it >>>>> has to be freed using kfree(). >>>>> This however does not happen if strbrk() fails, so maybe the memory >>>>> leak is caused by this? >>>>> >>>>> Armin Wolf >>>> >>>> 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@squebb.ca> >> 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@squebb.ca> >> 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@squebb.ca> >> 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@squebb.ca> >> 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 >>>>>>>>>> where the >>>>>>>>>> issue is, not the driver core, right? >>>>>>>>>> >>>>>>>>>> thanks, >>>>>>>>>> >>>>>>>>>> greg k-h >>>>>>>>> >>>>>>>>> Hi, Mr. Greg, >>>>>>>>> >>>>>>>>> Thanks for the quick reply. >>>>>>>>> >>>>>>>>> I have added CC: for additional developers per >>>>>>>>> drivers/platform/x86/wmi.c, >>>>>>>>> however, this seems to me like hieroglyphs. There is nothing >>>>>>>>> obvious, but >>>>>>>>> I had not noticed it with v6.3-rc3? >>>>>>>>> >>>>>>>>> Maybe, there seems to be something off: >>>>>>>>> >>>>>>>>> 949 static int wmi_dev_probe(struct device *dev) >>>>>>>>> 950 { >>>>>>>>> 951 struct wmi_block *wblock = dev_to_wblock(dev); >>>>>>>>> 952 struct wmi_driver *wdriver = >>>>>>>>> drv_to_wdrv(dev->driver); >>>>>>>>> 953 int ret = 0; >>>>>>>>> 954 char *buf; >>>>>>>>> 955 >>>>>>>>> 956 if (ACPI_FAILURE(wmi_method_enable(wblock, true))) >>>>>>>>> 957 dev_warn(dev, "failed to enable device >>>>>>>>> -- probing anyway\n"); >>>>>>>>> 958 >>>>>>>>> 959 if (wdriver->probe) { >>>>>>>>> 960 ret = wdriver->probe(dev_to_wdev(dev), >>>>>>>>> 961 find_guid_context(wblock, wdriver)); >>>>>>>>> 962 if (ret != 0) >>>>>>>>> 963 goto probe_failure; >>>>>>>>> 964 } >>>>>>>>> 965 >>>>>>>>> 966 /* driver wants a character device made */ >>>>>>>>> 967 if (wdriver->filter_callback) { >>>>>>>>> 968 /* check that required buffer size >>>>>>>>> declared by driver or MOF */ >>>>>>>>> 969 if (!wblock->req_buf_size) { >>>>>>>>> 970 dev_err(&wblock->dev.dev, >>>>>>>>> 971 "Required buffer size >>>>>>>>> not set\n"); >>>>>>>>> 972 ret = -EINVAL; >>>>>>>>> 973 goto probe_failure; >>>>>>>>> 974 } >>>>>>>>> 975 >>>>>>>>> 976 wblock->handler_data = >>>>>>>>> kmalloc(wblock->req_buf_size, >>>>>>>>> 977 GFP_KERNEL); >>>>>>>>> 978 if (!wblock->handler_data) { >>>>>>>>> 979 ret = -ENOMEM; >>>>>>>>> 980 goto probe_failure; >>>>>>>>> 981 } >>>>>>>>> 982 >>>>>>>>> 983 buf = kasprintf(GFP_KERNEL, "wmi/%s", >>>>>>>>> wdriver->driver.name); >>>>>>>>> 984 if (!buf) { >>>>>>>>> 985 ret = -ENOMEM; >>>>>>>>> 986 goto probe_string_failure; >>>>>>>>> 987 } >>>>>>>>> 988 wblock->char_dev.minor = >>>>>>>>> MISC_DYNAMIC_MINOR; >>>>>>>>> 989 wblock->char_dev.name = buf; >>>>>>>>> 990 wblock->char_dev.fops = &wmi_fops; >>>>>>>>> 991 wblock->char_dev.mode = 0444; >>>>>>>>> 992 ret = misc_register(&wblock->char_dev); >>>>>>>>> 993 if (ret) { >>>>>>>>> 994 dev_warn(dev, "failed to >>>>>>>>> register char dev: %d\n", ret); >>>>>>>>> 995 ret = -ENOMEM; >>>>>>>>> 996 goto probe_misc_failure; >>>>>>>>> 997 } >>>>>>>>> 998 } >>>>>>>>> 999 >>>>>>>>> 1000 set_bit(WMI_PROBED, &wblock->flags); >>>>>>>>> 1001 return 0; >>>>>>>>> 1002 >>>>>>>>> 1003 probe_misc_failure: >>>>>>>>> 1004 kfree(buf); >>>>>>>>> 1005 probe_string_failure: >>>>>>>>> 1006 kfree(wblock->handler_data); >>>>>>>>> 1007 probe_failure: >>>>>>>>> 1008 if (ACPI_FAILURE(wmi_method_enable(wblock, >>>>>>>>> false))) >>>>>>>>> 1009 dev_warn(dev, "failed to disable >>>>>>>>> device\n"); >>>>>>>>> >>>>>>>>> >>>>>>>>> char *buf is passed to kfree(buf) uninitialised if >>>>>>>>> wdriver->filter_callback >>>>>>>>> is not set. >>>>>>>>> >>>>>>>>> It seems like a logical error per se, but I don't believe this is >>>>>>>>> the cause >>>>>>>>> of the leak? >>>>>>>> >>>>>>>> CORRECTION: >>>>>>>> >>>>>>>> I overlooked the "return 0" in line 1001. >>>>>>> >>>>>>> Yeah, and the memory looks to be freed properly in the >>>>>>> wmi_dev_remove() >>>>>>> callback, right? >>>>>> >>>>>> It would appear so. To verify that: >>>>>> >>>>>> Alloc: >>>>>> 976 wblock->handler_data = kmalloc(wblock->req_buf_size, >>>>>> GFP_KERNEL); >>>>>> <check> >>>>>> >>>>>> 983 buf = kasprintf(GFP_KERNEL, "wmi/%s", wdriver->driver.name); >>>>>> <check> >>>>>> 989 wblock->char_dev.name = buf; >>>>>> >>>>>> In lines 1022-1023: >>>>>> >>>>>> 1022 kfree(wblock->char_dev.name); >>>>>> 1023 kfree(wblock->handler_data); >>>>>> >>>>>>>> This is why I don't think things should be rushed, but analysed >>>>>>>> with clear and >>>>>>>> cold head. And with as many eyes as possible :) >>>>>>>> >>>>>>>> The driver stuff is my long-term research interest. To state the >>>>>>>> obvious, >>>>>>>> the printing and multimedia education and industry in general >>>>>>>> would benefit from >>>>>>>> the open-source drivers for many instruments that still work, but >>>>>>>> are obsoleted >>>>>>>> by the producer and require unsupported versions of the OS. >>>>>>>> >>>>>>>> Thank you again for reviewing the bug report, however, ATM I do >>>>>>>> not think I have >>>>>>>> what it takes to hunt down the memleak. :-/ >>>>>>> >>>>>>> Do you have a reproducer that you can use to show the problem better? >>>>>> >>>>>> Unfortunately, the problem doesn't seem to appear during the run of >>>>>> a particular >>>>>> test, but immediately on startup of the OS. This makes it awkward to >>>>>> pinpoint the >>>>>> exact service that triggered memory leaks. But they would appear to >>>>>> have to do >>>>>> with the initialisation of the USB devices, wouldn't they? >>>>>> >>>>>> There seem to be strings: >>>>>> >>>>>> "USBPortAccess,Enabled;[Optional:" >>>>>> "USBBIOSSupport,Enabled;[Optional" >>>>>> "USBEnumerationDelay,Disabled;[Op" >>>>>> >>>>>> This seems to be happening during USB initialisation and before any >>>>>> services. >>>>>> But I might as well be wrong. >>>>>> >>>>>>> Or can you test kernel patches to verify the problem is fixed or >>>>>>> not if >>>>>>> we send you patches to test? >>>>>> >>>>>> Certainly, Lord willing, I can test the patches in the same >>>>>> environment that >>>>>> mainfeted the bug (or memleak).
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); + } + } } /* * 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
| |