[RESEND,v4,1/1] test_firmware: fix some memory leaks and racing conditions

  Some functions were called both from locked and unlocked context, so
the lock was dropped prematurely, introducing a race condition when
deadlock was avoided.

Having two locks wouldn't assure a race-proof mutual exclusion.

__test_dev_config_update_bool(), __test_dev_config_update_u8() and
__test_dev_config_update_size_t() unlocked versions of the functions
were introduced to be called from the locked contexts as a workaround
without releasing the main driver's lock and causing a race condition.

This should guarantee mutual exclusion and prevent any race conditions.

Locked versions simply allow for mutual exclusion and call the unlocked
counterparts, to avoid duplication of code.

trigger_batched_requests_store() and trigger_batched_requests_async_store()
now return -EBUSY if called with test_fw_config->reqs already allocated,
so the memory leak is prevented.

The same functions now keep track of the allocated buf for firmware in
req->fw_buf as release_firmware() will not deallocate this storage for us.

Additionally, in __test_release_all_firmware(), req->fw_buf is released
before calling release_firmware(req->fw),
foreach test_fw_config->reqs[i], i = 0 .. test_fw_config->num_requests-1

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> 
Cc: Luis Chamberlain <mcgrof@kernel.org> 
Cc: Russ Weight <russell.h.weight@intel.com> 
Cc: Tianfei zhang <tianfei.zhang@intel.com> 
Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr> 
Cc: Zhengchao Shao <shaozhengchao@huawei.com> 
Cc: Colin Ian King <colin.i.king@gmail.com> 
Cc: linux-kernel@vger.kernel.org 
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Kees Cook <keescook@chromium.org>
Cc: Scott Branden <sbranden@broadcom.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Suggested-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
---
v3 -> v4
 - fix additional memory leaks of the allocated firmware buffers
 - fix noticed racing conditions in conformance with the existing code
 - make it a single patch

 lib/test_firmware.c | 81 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 63 insertions(+), 18 deletions(-)
  

On Fri, Apr 21, 2023 at 08:52:06PM +0200, Mirsad Goran Todorovac wrote:
> Some functions were called both from locked and unlocked context, so
> the lock was dropped prematurely, introducing a race condition when
> deadlock was avoided.
> 
> Having two locks wouldn't assure a race-proof mutual exclusion.
> 
> __test_dev_config_update_bool(), __test_dev_config_update_u8() and
> __test_dev_config_update_size_t() unlocked versions of the functions
> were introduced to be called from the locked contexts as a workaround
> without releasing the main driver's lock and causing a race condition.
> 
> This should guarantee mutual exclusion and prevent any race conditions.
> 
> Locked versions simply allow for mutual exclusion and call the unlocked
> counterparts, to avoid duplication of code.
> 
> trigger_batched_requests_store() and trigger_batched_requests_async_store()
> now return -EBUSY if called with test_fw_config->reqs already allocated,
> so the memory leak is prevented.
> 
> The same functions now keep track of the allocated buf for firmware in
> req->fw_buf as release_firmware() will not deallocate this storage for us.
> 
> Additionally, in __test_release_all_firmware(), req->fw_buf is released
> before calling release_firmware(req->fw),
> foreach test_fw_config->reqs[i], i = 0 .. test_fw_config->num_requests-1
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> 
> Cc: Luis Chamberlain <mcgrof@kernel.org> 
> Cc: Russ Weight <russell.h.weight@intel.com> 
> Cc: Tianfei zhang <tianfei.zhang@intel.com> 
> Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr> 
> Cc: Zhengchao Shao <shaozhengchao@huawei.com> 
> Cc: Colin Ian King <colin.i.king@gmail.com> 
> Cc: linux-kernel@vger.kernel.org 
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Scott Branden <sbranden@broadcom.com>
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Suggested-by: Dan Carpenter <error27@gmail.com>
> Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>

Mirad, thanks for this work, good stuff! So the patch just needs to be
adjust with:

Fixes: 7feebfa487b92 ("test_firmware: add support for request_firmware_into_buf"
Cc: stable@vger.kernel.org # v5.4

Then, can you split the patch in two, one which fixes the memory leaks
and another that deals with the mutexes. The second patch might be a fix
for the original code but I can't tell until I see the changes split out.

The commit log should account for the memory leak and be clear how it
happens. The other commit log for the second patch should clarify what
it fixes and why as well.

  Luis
  

On 25.4.2023. 20:27, Luis Chamberlain wrote:
> On Fri, Apr 21, 2023 at 08:52:06PM +0200, Mirsad Goran Todorovac wrote:
>> Some functions were called both from locked and unlocked context, so
>> the lock was dropped prematurely, introducing a race condition when
>> deadlock was avoided.
>>
>> Having two locks wouldn't assure a race-proof mutual exclusion.
>>
>> __test_dev_config_update_bool(), __test_dev_config_update_u8() and
>> __test_dev_config_update_size_t() unlocked versions of the functions
>> were introduced to be called from the locked contexts as a workaround
>> without releasing the main driver's lock and causing a race condition.
>>
>> This should guarantee mutual exclusion and prevent any race conditions.
>>
>> Locked versions simply allow for mutual exclusion and call the unlocked
>> counterparts, to avoid duplication of code.
>>
>> trigger_batched_requests_store() and trigger_batched_requests_async_store()
>> now return -EBUSY if called with test_fw_config->reqs already allocated,
>> so the memory leak is prevented.
>>
>> The same functions now keep track of the allocated buf for firmware in
>> req->fw_buf as release_firmware() will not deallocate this storage for us.
>>
>> Additionally, in __test_release_all_firmware(), req->fw_buf is released
>> before calling release_firmware(req->fw),
>> foreach test_fw_config->reqs[i], i = 0 .. test_fw_config->num_requests-1
>>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Luis Chamberlain <mcgrof@kernel.org>
>> Cc: Russ Weight <russell.h.weight@intel.com>
>> Cc: Tianfei zhang <tianfei.zhang@intel.com>
>> Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> Cc: Zhengchao Shao <shaozhengchao@huawei.com>
>> Cc: Colin Ian King <colin.i.king@gmail.com>
>> Cc: linux-kernel@vger.kernel.org
>> Cc: Takashi Iwai <tiwai@suse.de>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Scott Branden <sbranden@broadcom.com>
>> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
>> Suggested-by: Dan Carpenter <error27@gmail.com>
>> Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
> 
> Mirad, thanks for this work, good stuff! So the patch just needs to be
> adjust with:
> 
> Fixes: 7feebfa487b92 ("test_firmware: add support for request_firmware_into_buf"
> Cc: stable@vger.kernel.org # v5.4
> 
> Then, can you split the patch in two, one which fixes the memory leaks
> and another that deals with the mutexes. The second patch might be a fix
> for the original code but I can't tell until I see the changes split out.
> 
> The commit log should account for the memory leak and be clear how it
> happens. The other commit log for the second patch should clarify what
> it fixes and why as well.

It seems to me that there is something wrong with the patchwork, as this commit
had not yet appeared in 5.4 LTS stable tree?

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/log/?qt=grep&q=Mirsad&h=linux-5.4.y

I was accidentally looking whether it passed, and it seems like more than a month
since the other commits propagated :-/

Thanks,
Mirsad
  

On Thu, Jul 27, 2023 at 08:16:17AM +0200, Mirsad Todorovac wrote:
> On 25.4.2023. 20:27, Luis Chamberlain wrote:
> > On Fri, Apr 21, 2023 at 08:52:06PM +0200, Mirsad Goran Todorovac wrote:
> > > Some functions were called both from locked and unlocked context, so
> > > the lock was dropped prematurely, introducing a race condition when
> > > deadlock was avoided.
> > > 
> > > Having two locks wouldn't assure a race-proof mutual exclusion.
> > > 
> > > __test_dev_config_update_bool(), __test_dev_config_update_u8() and
> > > __test_dev_config_update_size_t() unlocked versions of the functions
> > > were introduced to be called from the locked contexts as a workaround
> > > without releasing the main driver's lock and causing a race condition.
> > > 
> > > This should guarantee mutual exclusion and prevent any race conditions.
> > > 
> > > Locked versions simply allow for mutual exclusion and call the unlocked
> > > counterparts, to avoid duplication of code.
> > > 
> > > trigger_batched_requests_store() and trigger_batched_requests_async_store()
> > > now return -EBUSY if called with test_fw_config->reqs already allocated,
> > > so the memory leak is prevented.
> > > 
> > > The same functions now keep track of the allocated buf for firmware in
> > > req->fw_buf as release_firmware() will not deallocate this storage for us.
> > > 
> > > Additionally, in __test_release_all_firmware(), req->fw_buf is released
> > > before calling release_firmware(req->fw),
> > > foreach test_fw_config->reqs[i], i = 0 .. test_fw_config->num_requests-1
> > > 
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Luis Chamberlain <mcgrof@kernel.org>
> > > Cc: Russ Weight <russell.h.weight@intel.com>
> > > Cc: Tianfei zhang <tianfei.zhang@intel.com>
> > > Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > Cc: Zhengchao Shao <shaozhengchao@huawei.com>
> > > Cc: Colin Ian King <colin.i.king@gmail.com>
> > > Cc: linux-kernel@vger.kernel.org
> > > Cc: Takashi Iwai <tiwai@suse.de>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Scott Branden <sbranden@broadcom.com>
> > > Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> > > Suggested-by: Dan Carpenter <error27@gmail.com>
> > > Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
> > 
> > Mirad, thanks for this work, good stuff! So the patch just needs to be
> > adjust with:
> > 
> > Fixes: 7feebfa487b92 ("test_firmware: add support for request_firmware_into_buf"
> > Cc: stable@vger.kernel.org # v5.4
> > 
> > Then, can you split the patch in two, one which fixes the memory leaks
> > and another that deals with the mutexes. The second patch might be a fix
> > for the original code but I can't tell until I see the changes split out.
> > 
> > The commit log should account for the memory leak and be clear how it
> > happens. The other commit log for the second patch should clarify what
> > it fixes and why as well.
> 
> It seems to me that there is something wrong with the patchwork, as this commit
> had not yet appeared in 5.4 LTS stable tree?

Did you resend a new v5 with the requested changes?

  Luis
  

On 7/27/23 17:36, Luis Chamberlain wrote:
> On Thu, Jul 27, 2023 at 08:16:17AM +0200, Mirsad Todorovac wrote:
>> On 25.4.2023. 20:27, Luis Chamberlain wrote:
>>> On Fri, Apr 21, 2023 at 08:52:06PM +0200, Mirsad Goran Todorovac wrote:
>>>> Some functions were called both from locked and unlocked context, so
>>>> the lock was dropped prematurely, introducing a race condition when
>>>> deadlock was avoided.
>>>>
>>>> Having two locks wouldn't assure a race-proof mutual exclusion.
>>>>
>>>> __test_dev_config_update_bool(), __test_dev_config_update_u8() and
>>>> __test_dev_config_update_size_t() unlocked versions of the functions
>>>> were introduced to be called from the locked contexts as a workaround
>>>> without releasing the main driver's lock and causing a race condition.
>>>>
>>>> This should guarantee mutual exclusion and prevent any race conditions.
>>>>
>>>> Locked versions simply allow for mutual exclusion and call the unlocked
>>>> counterparts, to avoid duplication of code.
>>>>
>>>> trigger_batched_requests_store() and trigger_batched_requests_async_store()
>>>> now return -EBUSY if called with test_fw_config->reqs already allocated,
>>>> so the memory leak is prevented.
>>>>
>>>> The same functions now keep track of the allocated buf for firmware in
>>>> req->fw_buf as release_firmware() will not deallocate this storage for us.
>>>>
>>>> Additionally, in __test_release_all_firmware(), req->fw_buf is released
>>>> before calling release_firmware(req->fw),
>>>> foreach test_fw_config->reqs[i], i = 0 .. test_fw_config->num_requests-1
>>>>
>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>> Cc: Luis Chamberlain <mcgrof@kernel.org>
>>>> Cc: Russ Weight <russell.h.weight@intel.com>
>>>> Cc: Tianfei zhang <tianfei.zhang@intel.com>
>>>> Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>>> Cc: Zhengchao Shao <shaozhengchao@huawei.com>
>>>> Cc: Colin Ian King <colin.i.king@gmail.com>
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Cc: Takashi Iwai <tiwai@suse.de>
>>>> Cc: Kees Cook <keescook@chromium.org>
>>>> Cc: Scott Branden <sbranden@broadcom.com>
>>>> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
>>>> Suggested-by: Dan Carpenter <error27@gmail.com>
>>>> Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
>>>
>>> Mirad, thanks for this work, good stuff! So the patch just needs to be
>>> adjust with:
>>>
>>> Fixes: 7feebfa487b92 ("test_firmware: add support for request_firmware_into_buf"
>>> Cc: stable@vger.kernel.org # v5.4
>>>
>>> Then, can you split the patch in two, one which fixes the memory leaks
>>> and another that deals with the mutexes. The second patch might be a fix
>>> for the original code but I can't tell until I see the changes split out.
>>>
>>> The commit log should account for the memory leak and be clear how it
>>> happens. The other commit log for the second patch should clarify what
>>> it fixes and why as well.
>>
>> It seems to me that there is something wrong with the patchwork, as this commit
>> had not yet appeared in 5.4 LTS stable tree?
> 
> Did you resend a new v5 with the requested changes?

Actually, I am not sure what is the procedure, for it is accepted and visible in 5.10+ trees?

Mirsad
  

On Fri, Jul 28, 2023 at 12:14:22AM +0200, Mirsad Todorovac wrote:
> On 7/27/23 17:36, Luis Chamberlain wrote:
> > On Thu, Jul 27, 2023 at 08:16:17AM +0200, Mirsad Todorovac wrote:
> > > On 25.4.2023. 20:27, Luis Chamberlain wrote:
> > > > On Fri, Apr 21, 2023 at 08:52:06PM +0200, Mirsad Goran Todorovac wrote:
> > > > > Some functions were called both from locked and unlocked context, so
> > > > > the lock was dropped prematurely, introducing a race condition when
> > > > > deadlock was avoided.
> > > > > 
> > > > > Having two locks wouldn't assure a race-proof mutual exclusion.
> > > > > 
> > > > > __test_dev_config_update_bool(), __test_dev_config_update_u8() and
> > > > > __test_dev_config_update_size_t() unlocked versions of the functions
> > > > > were introduced to be called from the locked contexts as a workaround
> > > > > without releasing the main driver's lock and causing a race condition.
> > > > > 
> > > > > This should guarantee mutual exclusion and prevent any race conditions.
> > > > > 
> > > > > Locked versions simply allow for mutual exclusion and call the unlocked
> > > > > counterparts, to avoid duplication of code.
> > > > > 
> > > > > trigger_batched_requests_store() and trigger_batched_requests_async_store()
> > > > > now return -EBUSY if called with test_fw_config->reqs already allocated,
> > > > > so the memory leak is prevented.
> > > > > 
> > > > > The same functions now keep track of the allocated buf for firmware in
> > > > > req->fw_buf as release_firmware() will not deallocate this storage for us.
> > > > > 
> > > > > Additionally, in __test_release_all_firmware(), req->fw_buf is released
> > > > > before calling release_firmware(req->fw),
> > > > > foreach test_fw_config->reqs[i], i = 0 .. test_fw_config->num_requests-1
> > > > > 
> > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > Cc: Luis Chamberlain <mcgrof@kernel.org>
> > > > > Cc: Russ Weight <russell.h.weight@intel.com>
> > > > > Cc: Tianfei zhang <tianfei.zhang@intel.com>
> > > > > Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > > > Cc: Zhengchao Shao <shaozhengchao@huawei.com>
> > > > > Cc: Colin Ian King <colin.i.king@gmail.com>
> > > > > Cc: linux-kernel@vger.kernel.org
> > > > > Cc: Takashi Iwai <tiwai@suse.de>
> > > > > Cc: Kees Cook <keescook@chromium.org>
> > > > > Cc: Scott Branden <sbranden@broadcom.com>
> > > > > Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> > > > > Suggested-by: Dan Carpenter <error27@gmail.com>
> > > > > Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
> > > > 
> > > > Mirad, thanks for this work, good stuff! So the patch just needs to be
> > > > adjust with:
> > > > 
> > > > Fixes: 7feebfa487b92 ("test_firmware: add support for request_firmware_into_buf"
> > > > Cc: stable@vger.kernel.org # v5.4
> > > > 
> > > > Then, can you split the patch in two, one which fixes the memory leaks
> > > > and another that deals with the mutexes. The second patch might be a fix
> > > > for the original code but I can't tell until I see the changes split out.
> > > > 
> > > > The commit log should account for the memory leak and be clear how it
> > > > happens. The other commit log for the second patch should clarify what
> > > > it fixes and why as well.
> > > 
> > > It seems to me that there is something wrong with the patchwork, as this commit
> > > had not yet appeared in 5.4 LTS stable tree?
> > 
> > Did you resend a new v5 with the requested changes?
> 
> Actually, I am not sure what is the procedure, for it is accepted and visible in 5.10+ trees?

Ah its upstream but not on v5.4. Just send a patch to stable folks and
include its commit IDs as stable patches typically do.

  Luis