Re: [BUG] [PATCH RFC v2] selftests/firmware: copious kernel memory leaks in test_fw_run_batch_request()
From: Mirsad Goran Todorovac
Date: Thu Mar 30 2023 - 14:43:16 EST
On 30. 03. 2023. 18:01, Takashi Iwai wrote:
> On Thu, 30 Mar 2023 17:44:42 +0200,
> Dan Carpenter wrote:
>>
>> I admire your enthusiam. :) What about if we just did this? Does it
>> help with the leaks?
>>
>> regards,
>> dan carpenter
>>
>> diff --git a/lib/test_firmware.c b/lib/test_firmware.c
>> index 05ed84c2fc4c..626b836895f4 100644
>> --- a/lib/test_firmware.c
>> +++ b/lib/test_firmware.c
>> @@ -895,6 +895,9 @@ static ssize_t trigger_batched_requests_store(struct device *dev,
>>
>> mutex_lock(&test_fw_mutex);
>>
>> + if (test_fw_config->reqs)
>> + return -EBUSY;
>> +
>
> This leaves the mutex locked.
> It should be the following instead, I suppose?
>
> if (test_fw_config->reqs) {
> rc = -EBUSY;
> goto out_unlock;
> }
>
>
> Takashi
Hi, Dan, Takashi,
Unfortunately, it did not suffice.
What I was building with was
commit 8bb95a1662f8:Merge tag 'sound-6.3-rc5' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
with the following patch for lib/test_firmware.c:
---
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 05ed84c2fc4c..4daa38bd2cac 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -353,6 +353,19 @@ static ssize_t config_test_show_str(char *dst,
return len;
}
+static inline int test_dev_config_update_bool_unlocked(const char *buf, size_t size,
+ bool *cfg)
+{
+ int ret;
+
+ if (kstrtobool(buf, cfg) < 0)
+ ret = -EINVAL;
+ else
+ ret = size;
+
+ return ret;
+}
+
static int test_dev_config_update_bool(const char *buf, size_t size,
bool *cfg)
{
@@ -373,6 +386,24 @@ static ssize_t test_dev_config_show_bool(char *buf, bool val)
return snprintf(buf, PAGE_SIZE, "%d\n", val);
}
+static int test_dev_config_update_size_t_unlocked(
+ const char *buf,
+ size_t size,
+ size_t *cfg)
+{
+ int ret;
+ long new;
+
+ ret = kstrtol(buf, 10, &new);
+ if (ret)
+ return ret;
+
+ *(size_t *)cfg = new;
+
+ /* Always return full write size even if we didn't consume all */
+ return size;
+}
+
static int test_dev_config_update_size_t(const char *buf,
size_t size,
size_t *cfg)
@@ -402,6 +433,21 @@ static ssize_t test_dev_config_show_int(char *buf, int val)
return snprintf(buf, PAGE_SIZE, "%d\n", val);
}
+static int test_dev_config_update_u8_unlocked(const char *buf, size_t size, u8 *cfg)
+{
+ u8 val;
+ int ret;
+
+ ret = kstrtou8(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ *(u8 *)cfg = val;
+
+ /* Always return full write size even if we didn't consume all */
+ return size;
+}
+
static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
{
u8 val;
@@ -471,10 +517,10 @@ static ssize_t config_num_requests_store(struct device *dev,
mutex_unlock(&test_fw_mutex);
goto out;
}
- mutex_unlock(&test_fw_mutex);
- rc = test_dev_config_update_u8(buf, count,
- &test_fw_config->num_requests);
+ rc = test_dev_config_update_u8_unlocked(buf, count,
+ &test_fw_config->num_requests);
+ mutex_unlock(&test_fw_mutex);
out:
return rc;
@@ -518,10 +564,10 @@ static ssize_t config_buf_size_store(struct device *dev,
mutex_unlock(&test_fw_mutex);
goto out;
}
- mutex_unlock(&test_fw_mutex);
- rc = test_dev_config_update_size_t(buf, count,
- &test_fw_config->buf_size);
+ rc = test_dev_config_update_size_t_unlocked(buf, count,
+ &test_fw_config->buf_size);
+ mutex_unlock(&test_fw_mutex);
out:
return rc;
@@ -548,10 +594,10 @@ static ssize_t config_file_offset_store(struct device *dev,
mutex_unlock(&test_fw_mutex);
goto out;
}
- mutex_unlock(&test_fw_mutex);
- rc = test_dev_config_update_size_t(buf, count,
- &test_fw_config->file_offset);
+ rc = test_dev_config_update_size_t_unlocked(buf, count,
+ &test_fw_config->file_offset);
+ mutex_unlock(&test_fw_mutex);
out:
return rc;
@@ -895,6 +941,11 @@ static ssize_t trigger_batched_requests_store(struct device *dev,
mutex_lock(&test_fw_mutex);
+ if (test_fw_config->reqs) {
+ rc = -EBUSY;
+ goto out_unlock;
+ }
+
test_fw_config->reqs =
vzalloc(array3_size(sizeof(struct test_batched_req),
test_fw_config->num_requests, 2));
@@ -993,6 +1044,11 @@ ssize_t trigger_batched_requests_async_store(struct device *dev,
mutex_lock(&test_fw_mutex);
+ if (test_fw_config->reqs) {
+ rc = -EBUSY;
+ goto out;
+ }
+
test_fw_config->reqs =
vzalloc(array3_size(sizeof(struct test_batched_req),
test_fw_config->num_requests, 2));
The leaks are the same:
unreferenced object 0xffff96deccc99c00 (size 1024):
comm "test_firmware-2", pid 3093, jiffies 4294945062 (age 605.444s)
hex dump (first 32 bytes):
45 46 47 48 34 35 36 37 0a 00 00 00 00 00 00 00 EFGH4567........
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffffb58fb26c>] slab_post_alloc_hook+0x8c/0x3e0
[<ffffffffb5902b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
[<ffffffffb587696e>] kmalloc_trace+0x2e/0xc0
[<ffffffffb5cc00c0>] test_fw_run_batch_request+0x90/0x170
[<ffffffffb55d6dff>] kthread+0x10f/0x140
[<ffffffffb5402fa9>] ret_from_fork+0x29/0x50
unreferenced object 0xffff96ded72be400 (size 1024):
comm "test_firmware-3", pid 3094, jiffies 4294945062 (age 605.444s)
hex dump (first 32 bytes):
45 46 47 48 34 35 36 37 0a 00 00 00 00 00 00 00 EFGH4567........
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffffb58fb26c>] slab_post_alloc_hook+0x8c/0x3e0
[<ffffffffb5902b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
[<ffffffffb587696e>] kmalloc_trace+0x2e/0xc0
[<ffffffffb5cc00c0>] test_fw_run_batch_request+0x90/0x170
[<ffffffffb55d6dff>] kthread+0x10f/0x140
[<ffffffffb5402fa9>] ret_from_fork+0x29/0x50
unreferenced object 0xffff96dec9e32800 (size 1024):
comm "test_firmware-0", pid 3101, jiffies 4294945072 (age 605.404s)
hex dump (first 32 bytes):
45 46 47 48 34 35 36 37 0a 00 00 00 00 00 00 00 EFGH4567........
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffffb58fb26c>] slab_post_alloc_hook+0x8c/0x3e0
[<ffffffffb5902b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
[<ffffffffb587696e>] kmalloc_trace+0x2e/0xc0
[<ffffffffb5cc00c0>] test_fw_run_batch_request+0x90/0x170
[<ffffffffb55d6dff>] kthread+0x10f/0x140
[<ffffffffb5402fa9>] ret_from_fork+0x29/0x50
unreferenced object 0xffff96df0ab17000 (size 1024):
comm "test_firmware-1", pid 3102, jiffies 4294945073 (age 605.432s)
hex dump (first 32 bytes):
45 46 47 48 34 35 36 37 0a 00 00 00 00 00 00 00 EFGH4567........
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffffb58fb26c>] slab_post_alloc_hook+0x8c/0x3e0
[<ffffffffb5902b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
[<ffffffffb587696e>] kmalloc_trace+0x2e/0xc0
[<ffffffffb5cc00c0>] test_fw_run_batch_request+0x90/0x170
[<ffffffffb55d6dff>] kthread+0x10f/0x140
[<ffffffffb5402fa9>] ret_from_fork+0x29/0x50
unreferenced object 0xffff96decd6f6400 (size 1024):
comm "test_firmware-2", pid 3103, jiffies 4294945073 (age 605.432s)
hex dump (first 32 bytes):
45 46 47 48 34 35 36 37 0a 00 00 00 00 00 00 00 EFGH4567........
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffffb58fb26c>] slab_post_alloc_hook+0x8c/0x3e0
[<ffffffffb5902b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
[<ffffffffb587696e>] kmalloc_trace+0x2e/0xc0
[<ffffffffb5cc00c0>] test_fw_run_batch_request+0x90/0x170
[<ffffffffb55d6dff>] kthread+0x10f/0x140
[<ffffffffb5402fa9>] ret_from_fork+0x29/0x50
unreferenced object 0xffff96df0dc69c00 (size 1024):
comm "test_firmware-3", pid 3104, jiffies 4294945073 (age 605.432s)
hex dump (first 32 bytes):
45 46 47 48 34 35 36 37 0a 00 00 00 00 00 00 00 EFGH4567........
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffffb58fb26c>] slab_post_alloc_hook+0x8c/0x3e0
[<ffffffffb5902b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
[<ffffffffb587696e>] kmalloc_trace+0x2e/0xc0
[<ffffffffb5cc00c0>] test_fw_run_batch_request+0x90/0x170
[<ffffffffb55d6dff>] kthread+0x10f/0x140
[<ffffffffb5402fa9>] ret_from_fork+0x29/0x50
[root@pc-mtodorov linux_torvalds]# uname -rms
Linux 6.3.0-rc4mt+20230330-00051-g8bb95a1662f8-dirty x86_64
[root@pc-mtodorov linux_torvalds]#
My gut feeling tells me that it is not test_fw_config->reqs because
there are 75 instances leaked.
Regards,
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
The European Union
"I see something approaching fast ... Will it be friends with me?"