lkml.org 
[lkml]   [2023]   [Mar]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [BUG] [BISECTED] [CORRECTION] systemd-devd triggers kernel memleak apparently in drivers/core/dd.c: driver_register()
From
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
\
 
 \ /
  Last update: 2023-03-29 15:32    [W:0.232 / U:0.392 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site