On 2/16/23 15:16, Bartosz Golaszewski wrote:
On Wed, Feb 15, 2023 at 3:45 PM Mirsad Goran Todorovac
<mirsad.todorovac@xxxxxxxxxxxx> wrote:
On 15.2.2023. 11:53, Andy Shevchenko wrote:
On Tue, Feb 14, 2023 at 07:19:16PM +0100, Mirsad Goran Todorovac wrote:
On 12. 02. 2023. 15:19, Andy Shevchenko wrote:
On Wed, Feb 08, 2023 at 08:55:24PM +0100, Mirsad Goran Todorovac wrote:
On 31. 01. 2023. 10:36, Mirsad Goran Todorovac wrote:
I came across this memory leak apparently in the GPIO device driver.
It is still present in 6.2-rc6 release candidate kernel (just ran kselftest).
This is a vanilla Torvalds tree kernel with MGLRU and KMEMLEAK (obviously)
enabled.
If you think this bug is significant, I can attempt the bug bisect in the
environment that triggered it (Lenovo LENOVO_MT_10TX_BU_Lenovo_FM_V530S-07ICB)
with BIOS M22KT49A from 11/10/2022 and AlmaLinux 8.7.
Here is the /sys/kernel/debug/kmemleak output:
unreferenced object 0xffff9e67ad71f160 (size 32):
comm "gpio-sim.sh", pid 208926, jiffies 4372229685 (age 2101.564s)
hex dump (first 32 bytes):
67 70 69 6f 2d 73 69 6d 2e 30 2d 6e 6f 64 65 30 gpio-sim.0-node0
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<0000000098bf3d1b>] slab_post_alloc_hook+0x91/0x320
[<00000000da3205c5>] __kmem_cache_alloc_node+0x1bf/0x2b0
[<00000000aa51a58a>] __kmalloc_node_track_caller+0x55/0x140
[<00000000bd682ecc>] kvasprintf+0x6b/0xd0
[<00000000a3431d55>] kasprintf+0x4e/0x70
[<00000000f52d2629>] gpio_sim_device_config_live_store+0x401/0x59d [gpio_sim]
[<00000000673fc6df>] configfs_write_iter+0xcc/0x130
[<000000001d5d0829>] vfs_write+0x2b4/0x3d0
[<00000000d2336251>] ksys_write+0x61/0xe0
[<00000000f7015bb1>] __x64_sys_write+0x1a/0x20
[<000000008ac743d2>] do_syscall_64+0x58/0x80
[<000000004d7b7d50>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
Sorry, but unfortunately this patch didn't fix the memleak. Please see the result:
Thank you for give a try!
No sweat. It was worth the effort.
Yeah, that's why I put that I'm skeptical, because while patch is correct per
se it wouldn't prevent the initial leakage (it seems it happens due to other
circumstances).
I must admit that is looks like hieroglyphs to me. My learning curve had not
yet reached that point of debugging memory leaks in the kernel drivers ...
root@marvin-IdeaPad-3-15ITL6:/home/marvin/linux/kernel/linux_torvalds/tools/testing/selftests/gpio# echo clear > /sys/kernel/debug/kmemleak
root@marvin-IdeaPad-3-15ITL6:/home/marvin/linux/kernel/linux_torvalds/tools/testing/selftests/gpio# time ./gpio-sim.sh
trap: SIGTERM: bad trap
1. chip_name and dev_name attributes
1.1. Chip name is communicated to user
1.2. chip_name returns 'none' if the chip is still pending
1.3. Device name is communicated to user
2. Creating and configuring simulated chips
2.1. Default number of lines is 1
2.2. Number of lines can be specified
2.3. Label can be set
2.4. Label can be left empty
2.5. Line names can be configured
2.6. Line config can remain unused if offset is greater than number of lines
2.7. Line configfs directory names are sanitized
2.8. Multiple chips can be created
2.9. Can't modify settings when chip is live
2.10. Can't create line items when chip is live
2.11. Probe errors are propagated to user-space
2.12. Cannot enable a chip without any GPIO banks
2.13. Duplicate chip labels are not allowed
2.14. Lines can be hogged
3. Controlling simulated chips
3.1. Pull can be set over sysfs
3.2. Pull can be read from sysfs
3.3. Incorrect input in sysfs is rejected
3.4. Can't write to value
4. Simulated GPIO chips are functional
4.1. Values can be read from sysfs
4.2. Bias settings work correctly
GPIO gpio-sim test PASS
real 0m1.120s
user 0m0.283s
sys 0m0.842s
root@marvin-IdeaPad-3-15ITL6:/home/marvin/linux/kernel/linux_torvalds/tools/testing/selftests/gpio# echo scan > /sys/kernel/debug/kmemleak
root@marvin-IdeaPad-3-15ITL6:/home/marvin/linux/kernel/linux_torvalds/tools/testing/selftests/gpio# cat /sys/kernel/debug/kmemleak
unreferenced object 0xffff91f019219660 (size 32):
comm "gpio-sim.sh", pid 11223, jiffies 4295028142 (age 87.304s)
hex dump (first 32 bytes):
67 70 69 6f 2d 73 69 6d 2e 30 2d 6e 6f 64 65 30 gpio-sim.0-node0
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffff86fcd408>] __kmem_cache_alloc_node+0x1d8/0x330
[<ffffffff86f4c271>] __kmalloc_node_track_caller+0x51/0xd0
[<ffffffff872fb359>] kvasprintf+0x69/0xd0
[<ffffffff872fb4ce>] kasprintf+0x4e/0x70
[<ffffffffc1864553>] gpio_sim_device_config_live_store+0x483/0x6b5 [gpio_sim]
[<ffffffff870dde9c>] configfs_write_iter+0xcc/0x130
[<ffffffff87016869>] vfs_write+0x1f9/0x3b0
[<ffffffff87016dab>] ksys_write+0x6b/0xf0
[<ffffffff87016e59>] __x64_sys_write+0x19/0x20
[<ffffffff87b91188>] do_syscall_64+0x58/0x80
[<ffffffff87c000aa>] entry_SYSCALL_64_after_hwframe+0x72/0xdc
root@marvin-IdeaPad-3-15ITL6:/home/marvin/linux/kernel/linux_torvalds/tools/testing/selftests/gpio# uname -rms
Linux 6.2.0-rc8-lru-km-andy-00015-gf6feea56f66d-dirty x86_64
root@marvin-IdeaPad-3-15ITL6:/home/marvin/linux/kernel/linux_torvalds/tools/testing/selftests/gpio#
I can verify that I build the right patch:
marvin@marvin-IdeaPad-3-15ITL6:~/linux/kernel/linux_torvalds$ git diff
diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index 60514bc5454f..7f79e49b23d7 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -954,9 +954,9 @@ static void gpio_sim_device_deactivate_unlocked(struct gpio_sim_device *dev)
swnode = dev_fwnode(&dev->pdev->dev);
platform_device_unregister(dev->pdev);
+ gpio_sim_remove_hogs(dev);
gpio_sim_remove_swnode_recursive(swnode);
dev->pdev = NULL;
- gpio_sim_remove_hogs(dev);
}
static ssize_t
marvin@marvin-IdeaPad-3-15ITL6:~/linux/kernel/linux_torvalds$
Alternatively, I could try to bisect if you think it's prudent to try that.
But first I need a stroll after this kernel build :-)
Do you think that knowing when the bug was introduced might help find the culprit?
Thanks for the report Mirsad, just sent out a fix.
Hi, Bart,
It is really nothing. The reproducer shows that the leak is apparently gone.
As Mr. McKenney once said, a bunch of monkeys with keyboard could
have done it in a considerable number of trials and errors ;-)
But here I have something that could potentially leak as well. I could not devise a
reproducer due to the leak being lightly triggered only in extreme memory contention.
See it for yourself:
drivers/gpio/gpio-sim.c:
301 static int gpio_sim_setup_sysfs(struct gpio_sim_chip *chip)
302 {
303 struct device_attribute *val_dev_attr, *pull_dev_attr;
304 struct gpio_sim_attribute *val_attr, *pull_attr;
305 unsigned int num_lines = chip->gc.ngpio;
306 struct device *dev = chip->gc.parent;
307 struct attribute_group *attr_group;
308 struct attribute **attrs;
309 int i, ret;
310
311 chip->attr_groups = devm_kcalloc(dev, sizeof(*chip->attr_groups),
312 num_lines + 1, GFP_KERNEL);
313 if (!chip->attr_groups)
314 return -ENOMEM;
315
316 for (i = 0; i < num_lines; i++) {
317 attr_group = devm_kzalloc(dev, sizeof(*attr_group), GFP_KERNEL);
318 attrs = devm_kcalloc(dev, GPIO_SIM_NUM_ATTRS, sizeof(*attrs),
319 GFP_KERNEL);
320 val_attr = devm_kzalloc(dev, sizeof(*val_attr), GFP_KERNEL);
321 pull_attr = devm_kzalloc(dev, sizeof(*pull_attr), GFP_KERNEL);
322 if (!attr_group || !attrs || !val_attr || !pull_attr)
323 return -ENOMEM;
324
325 attr_group->name = devm_kasprintf(dev, GFP_KERNEL,
326 "sim_gpio%u", i);
327 if (!attr_group->name)
328 return -ENOMEM;
Apparently, if the memory allocation only partially succeeds, in the theoretical case
that the system is close to its kernel memory exhaustion, `return -ENOMEM` would not
free the partially succeeded allocs, would it?
To explain it better, I tried a version that is not yet full doing "all or nothing"
memory allocation for the gpio-sim driver, because I am not that familiar with the
driver internals.
I give this just as an illustration to what I mean to say (one line of code speaks
like thousand words):
301 static int gpio_sim_setup_sysfs(struct gpio_sim_chip *chip)
302 {
303 struct device_attribute *val_dev_attr, *pull_dev_attr;
304 struct gpio_sim_attribute *val_attr, *pull_attr;
305 unsigned int num_lines = chip->gc.ngpio;
306 struct device *dev = chip->gc.parent;
307 struct attribute_group *attr_group;
308 struct attribute **attrs;
309 int i, ret;
310
311 chip->attr_groups = devm_kcalloc(dev, sizeof(*chip->attr_groups),
312 num_lines + 1, GFP_KERNEL);
313 if (!chip->attr_groups)
314 return -ENOMEM;
315
316 for (i = 0; i < num_lines; i++) {
317 attr_group = devm_kzalloc(dev, sizeof(*attr_group), GFP_KERNEL);
318 attrs = devm_kcalloc(dev, GPIO_SIM_NUM_ATTRS, sizeof(*attrs),
319 GFP_KERNEL);
320 val_attr = devm_kzalloc(dev, sizeof(*val_attr), GFP_KERNEL);
321 pull_attr = devm_kzalloc(dev, sizeof(*pull_attr), GFP_KERNEL);
322 if (!attr_group || !attrs || !val_attr || !pull_attr) {
323 if (attr_group)
324 devm_kfree(dev, attr_group);
325 if (attrs)
326 devm_kfree(dev, attrs);
327 if (val_attr)
328 devm_kfree(dev, val_attr);
329 if (pull_attr)
330 devm_kfree(dev, pull_attr);
331 return -ENOMEM;
332 }
333
334 attr_group->name = devm_kasprintf(dev, GFP_KERNEL,
335 "sim_gpio%u", i);
336 if (!attr_group->name) {
337 devm_kfree(dev, attr_group);
338 devm_kfree(dev, attrs);
339 devm_kfree(dev, val_attr);
340 devm_kfree(dev, pull_attr);
341 return -ENOMEM;
342 }
343
344 val_attr->offset = pull_attr->offset = i;
This is, of course, far from perfect, and it seems that the driver will still leak
memory in case of partially successful allocation because of the
for (i = 0; i < num_lines; i++) { ... } loop. :-/
I hope this makes any sense to you.
Thanks,
Mirsad
--
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia
