Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 25 Apr 2023 11:27:30 -0700
From:   Luis Chamberlain <mcgrof@kernel.org>
To:     Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>,
        Scott Branden <sbranden@broadcom.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Russ Weight <russell.h.weight@intel.com>,
        linux-kernel@vger.kernel.org,
        Tianfei zhang <tianfei.zhang@intel.com>,
        Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
        Zhengchao Shao <shaozhengchao@huawei.com>,
        Colin Ian King <colin.i.king@gmail.com>,
        Takashi Iwai <tiwai@suse.de>,
        Kees Cook <keescook@chromium.org>,
        Dan Carpenter <error27@gmail.com>,
        Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Subject: Re: [PATCH RESEND v4 1/1] test_firmware: fix some memory leaks and
 racing conditions
Message-ID: <ZEgbkoSjHcVLcCcp@bombadil.infradead.org>
References: <20230421185205.28743-1-mirsad.todorovac@alu.unizg.hr>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230421185205.28743-1-mirsad.todorovac@alu.unizg.hr>
Sender: Luis Chamberlain <mcgrof@infradead.org>
Precedence: bulk

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