From: Armin Wolf <W_Armin@gmx.de>
To: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
linux-kernel@vger.kernel.org, Hans de Goede <hdegoede@redhat.com>,
Mark Gross <markgross@kernel.org>,
platform-driver-x86@vger.kernel.org
Subject: Re: [BUG] systemd-devd triggers kernel memleak apparently in drivers/core/dd.c: driver_register()
Date: Tue, 28 Mar 2023 21:55:15 +0200 [thread overview]
Message-ID: <4f65a23f-4e04-f04f-e56b-230a38ac5ec4@gmx.de> (raw)
In-Reply-To: <16862c45-2ffd-a2f2-6719-020c5d515800@alu.unizg.hr>
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().
However, i have no idea on how *info is somehow leaked, it has to happen inside the for-loop between the call
to tlmi_setting() and strreplace(), because otherwise the strings would not contain the "/" character.
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")
Also current_value_show() is used by attr_current_val, the __ATTR_RW_MODE() macro arranges for that.
Armin Wolf
>>>>>>> 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).
>>>
>>> Best regards,
>>> Mirsad
>>>
>
next prev parent reply other threads:[~2023-03-28 19:55 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-28 11:13 [BUG] systemd-devd triggers kernel memleak apparently in drivers/core/dd.c: driver_register() Mirsad Todorovac
2023-03-28 11:28 ` Greg Kroah-Hartman
2023-03-28 11:59 ` Mirsad Todorovac
2023-03-28 12:08 ` Mirsad Todorovac
2023-03-28 12:17 ` Greg Kroah-Hartman
2023-03-28 12:44 ` Mirsad Todorovac
2023-03-28 16:53 ` Armin Wolf
2023-03-28 19:06 ` Mirsad Goran Todorovac
2023-03-28 19:55 ` Armin Wolf [this message]
2023-03-29 8:13 ` Mirsad Goran Todorovac
2023-03-29 13:22 ` [BUG] [BISECTED] " Mirsad Goran Todorovac
2023-03-29 13:31 ` [BUG] [BISECTED] [CORRECTION] " Mirsad Goran Todorovac
2023-03-29 13:35 ` Thomas Weißschuh
2023-03-29 14:18 ` Mirsad Goran Todorovac
2023-03-29 15:46 ` Hans de Goede
2023-03-29 16:24 ` Mark Pearson
2023-03-29 16:43 ` Mirsad Goran Todorovac
2023-03-29 18:49 ` [BUG] [RFC] " Mirsad Goran Todorovac
2023-03-29 18:59 ` Mark Pearson
2023-03-29 19:21 ` Thomas Weißschuh
2023-03-29 21:50 ` Mirsad Goran Todorovac
2023-03-31 18:54 ` Mark Pearson
2023-03-31 19:04 ` Hans de Goede
2023-03-31 19:10 ` Mark Pearson
2023-03-31 19:13 ` Mirsad Goran Todorovac
2023-03-29 16:27 ` [BUG] [BISECTED] [CORRECTION] " Mirsad Goran Todorovac
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4f65a23f-4e04-f04f-e56b-230a38ac5ec4@gmx.de \
--to=w_armin@gmx.de \
--cc=gregkh@linuxfoundation.org \
--cc=hdegoede@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=markgross@kernel.org \
--cc=mirsad.todorovac@alu.unizg.hr \
--cc=platform-driver-x86@vger.kernel.org \
--cc=rafael@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.