Hi, all,

This is not a formal patch, but please see if you think the way the locking and race are solved correctly this time.

(Having two mutexes over the same set of resources is obviously a hazard.)

On 3/28/23 12:06, Dan Carpenter wrote:

On Tue, Mar 28, 2023 at 11:23:00AM +0200, Mirsad Todorovac wrote:

...

The leaks are in chunks of 1024 bytes (+ overhead), but so far I could not reproduce w/o root privileges, as tests refuse to run as unprivileged user. (This is not the proof of non-existence of an unprivileged automated exploit that would exhaust the kernel memory at approx. rate 4 MB/hour on our setup.

This would mean about 96 MB / day or 3 GB / month (of kernel memory).

This is firmware testing stuff. In the real world people aren't going to run their test scripts in a loop for days.

There is no security implications. This is root only. Also if the user could load firmware then that would be the headline. Once someone is can already load firmware then who cares if they leak 100MB per day?

It looks like if you call trigger_batched_requests_store() twice in a row then it will leak memory. Definitely test_fw_config->reqs is leaked. That's different from what the bug report is complaining about, but the point is that there are some obvious leaks. It looks like you're supposed to call trigger_batched_requests_store() in between runs?

There are other races like config_num_requests_store() should hold the mutex over the call to test_dev_config_update_u8() instead of dropping and retaking it.

COMMENT: Like in libc putc() family of functions, there is also putc_unlocked() The similar approach is applied here.

As the functions are callable from within both locked and non-locked environment, we have to either:

1. have two or more locks, which is dubious in terms of concurrency 2. have locked and unlocked version of each function, for we cannot lock the same lock twice.

NOTE: Memory leaks are not solved with this patch, only a couple of racing conditions.

--- diff --git a/lib/test_firmware.c b/lib/test_firmware.c index 05ed84c2fc4c..d6ed20bd1eb0 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;

Best regards, Mirsad

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

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