| Message ID | 20230509084746.48259-2-mirsad.todorovac@alu.unizg.hr |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RESEND,v5,1/3] test_firmware: prevent race conditions by a correct implementation of locking | expand |
Hi Dan, On 5/9/23 10:47, Mirsad Goran Todorovac wrote: > Dan Carpenter spotted that test_fw_config->reqs will be leaked if > trigger_batched_requests_store() is called two or more times. > The same appears with trigger_batched_requests_async_store(). > > This bug wasn't trigger by the tests, but observed by Dan's visual > inspection of the code. > > The recommended workaround was to return -EBUSY if test_fw_config->reqs > is already allocated. > > Fixes: 7feebfa487b92 ("test_firmware: add support for request_firmware_into_buf") > Cc: Luis Chamberlain <mcgrof@kernel.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Russ Weight <russell.h.weight@intel.com> > Cc: Tianfei Zhang <tianfei.zhang@intel.com> > Cc: Shuah Khan <shuah@kernel.org> > Cc: Colin Ian King <colin.i.king@gmail.com> > Cc: Randy Dunlap <rdunlap@infradead.org> > Cc: linux-kselftest@vger.kernel.org > Cc: stable@vger.kernel.org # v5.4 > Suggested-by: Dan Carpenter <error27@gmail.com> > Suggested-by: Takashi Iwai <tiwai@suse.de> > Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr> > --- > lib/test_firmware.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/lib/test_firmware.c b/lib/test_firmware.c > index 35417e0af3f4..91b232ed3161 100644 > --- a/lib/test_firmware.c > +++ b/lib/test_firmware.c > @@ -913,6 +913,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_bail; > + } > + > test_fw_config->reqs = > vzalloc(array3_size(sizeof(struct test_batched_req), > test_fw_config->num_requests, 2)); > @@ -1011,6 +1016,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_bail; > + } > + > test_fw_config->reqs = > vzalloc(array3_size(sizeof(struct test_batched_req), > test_fw_config->num_requests, 2)); I was just thinking, since returning -EBUSY for the case of already allocated test_fw_config->reqs was your suggestion and your idea, maybe it would be OK to properly reflect that in Co-developed-by: or Signed-off-by: , but if I understood well, the CoC requires that I am explicitly approved of those? Thanks, Mirsad
On 12. 05. 2023. 15:09, Dan Carpenter wrote: > On Fri, May 12, 2023 at 02:34:29PM +0200, Mirsad Todorovac wrote: >>> @@ -1011,6 +1016,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_bail; >>> + } >>> + >>> test_fw_config->reqs = >>> vzalloc(array3_size(sizeof(struct test_batched_req), >>> test_fw_config->num_requests, 2)); >> >> I was just thinking, since returning -EBUSY for the case of already allocated >> test_fw_config->reqs was your suggestion and your idea, maybe it would be OK >> to properly reflect that in Co-developed-by: or Signed-off-by: , but if I >> understood well, the CoC requires that I am explicitly approved of those? >> > > If everyone else is okay, let's just apply this as-is. You did all the > hard bits. > > regards, > dan carpenter If it is OK with you, then I hope I have your Reviewed-by: I'm kinda still uncertain about the proper procedure. This certainly isn't "the perfect patch" :-) Best regards, Mirsad
On Fri, May 12, 2023 at 08:58:58PM +0200, Mirsad Goran Todorovac wrote: > On 12. 05. 2023. 15:09, Dan Carpenter wrote: > > On Fri, May 12, 2023 at 02:34:29PM +0200, Mirsad Todorovac wrote: > > > > @@ -1011,6 +1016,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_bail; > > > > + } > > > > + > > > > test_fw_config->reqs = > > > > vzalloc(array3_size(sizeof(struct test_batched_req), > > > > test_fw_config->num_requests, 2)); > > > > > > I was just thinking, since returning -EBUSY for the case of already allocated > > > test_fw_config->reqs was your suggestion and your idea, maybe it would be OK > > > to properly reflect that in Co-developed-by: or Signed-off-by: , but if I > > > understood well, the CoC requires that I am explicitly approved of those? > > > > > > > If everyone else is okay, let's just apply this as-is. You did all the > > hard bits. > > > > regards, > > dan carpenter > > If it is OK with you, then I hope I have your Reviewed-by: > Wow. Sorry for all the delay on this. Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org> > I'm kinda still uncertain about the proper procedure. > This certainly isn't "the perfect patch" :-) Heh. regards, dan carpenter
On 5/18/23 17:20, Dan Carpenter wrote: > On Fri, May 12, 2023 at 08:58:58PM +0200, Mirsad Goran Todorovac wrote: >> On 12. 05. 2023. 15:09, Dan Carpenter wrote: >>> On Fri, May 12, 2023 at 02:34:29PM +0200, Mirsad Todorovac wrote: >>>>> @@ -1011,6 +1016,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_bail; >>>>> + } >>>>> + >>>>> test_fw_config->reqs = >>>>> vzalloc(array3_size(sizeof(struct test_batched_req), >>>>> test_fw_config->num_requests, 2)); >>>> >>>> I was just thinking, since returning -EBUSY for the case of already allocated >>>> test_fw_config->reqs was your suggestion and your idea, maybe it would be OK >>>> to properly reflect that in Co-developed-by: or Signed-off-by: , but if I >>>> understood well, the CoC requires that I am explicitly approved of those? >>>> >>> >>> If everyone else is okay, let's just apply this as-is. You did all the >>> hard bits. >>> >>> regards, >>> dan carpenter >> >> If it is OK with you, then I hope I have your Reviewed-by: >> > > Wow. Sorry for all the delay on this. No, not at all. I don't want to be a nag and overwhelm developers. :-) > Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org> Thank you. I suppose this is for 2/3. Did you consider reviewing the other two patches? >> I'm kinda still uncertain about the proper procedure. >> This certainly isn't "the perfect patch" :-) > > Heh. > > regards, > dan carpenter Well, I have about come to the limits of CONFIG_DEBUG_KMEMLEAK setting, with a happy catch of about a dozen bugs, but this is still less than 0.1% of the expected 11,000 bugs for a codebase sized 10.9 million line. So I am considering the use of a static analysis tool. Like Smatch. Thank Heavens, most of the code is modular, and about 90% of the functions are static and thereof, of course, having the scope limited to their module. I am still only catching bugs like memleaks and lockups when they manifest, proactive search for bugs is a new level I suppose. Best regards, Mirsad
On 5/24/23 07:34, Luis Chamberlain wrote: > On Thu, May 18, 2023 at 06:20:37PM +0300, Dan Carpenter wrote: >> On Fri, May 12, 2023 at 08:58:58PM +0200, Mirsad Goran Todorovac wrote: >>> On 12. 05. 2023. 15:09, Dan Carpenter wrote: >>>> On Fri, May 12, 2023 at 02:34:29PM +0200, Mirsad Todorovac wrote: >>>>>> @@ -1011,6 +1016,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_bail; >>>>>> + } >>>>>> + >>>>>> test_fw_config->reqs = >>>>>> vzalloc(array3_size(sizeof(struct test_batched_req), >>>>>> test_fw_config->num_requests, 2)); >>>>> >>>>> I was just thinking, since returning -EBUSY for the case of already allocated >>>>> test_fw_config->reqs was your suggestion and your idea, maybe it would be OK >>>>> to properly reflect that in Co-developed-by: or Signed-off-by: , but if I >>>>> understood well, the CoC requires that I am explicitly approved of those? >>>>> >>>> >>>> If everyone else is okay, let's just apply this as-is. You did all the >>>> hard bits. >>>> >>>> regards, >>>> dan carpenter >>> >>> If it is OK with you, then I hope I have your Reviewed-by: >>> >> >> Wow. Sorry for all the delay on this. >> >> Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org> >> > > Thanks for doing this work! It looks much better now split up! No problem. It's a great exercise for the little grey cells :-) > For all 3 patches: > > Acked-by: Luis Chamberlain <mcgrof@kernel.org> Thanks, Mirsad > Greg, can you pick these up? > > Luis
diff --git a/lib/test_firmware.c b/lib/test_firmware.c index 35417e0af3f4..91b232ed3161 100644 --- a/lib/test_firmware.c +++ b/lib/test_firmware.c @@ -913,6 +913,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_bail; + } + test_fw_config->reqs = vzalloc(array3_size(sizeof(struct test_batched_req), test_fw_config->num_requests, 2)); @@ -1011,6 +1016,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_bail; + } + test_fw_config->reqs = vzalloc(array3_size(sizeof(struct test_batched_req), test_fw_config->num_requests, 2));
Dan Carpenter spotted that test_fw_config->reqs will be leaked if trigger_batched_requests_store() is called two or more times. The same appears with trigger_batched_requests_async_store(). This bug wasn't trigger by the tests, but observed by Dan's visual inspection of the code. The recommended workaround was to return -EBUSY if test_fw_config->reqs is already allocated. Fixes: 7feebfa487b92 ("test_firmware: add support for request_firmware_into_buf") Cc: Luis Chamberlain <mcgrof@kernel.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Russ Weight <russell.h.weight@intel.com> Cc: Tianfei Zhang <tianfei.zhang@intel.com> Cc: Shuah Khan <shuah@kernel.org> Cc: Colin Ian King <colin.i.king@gmail.com> Cc: Randy Dunlap <rdunlap@infradead.org> Cc: linux-kselftest@vger.kernel.org Cc: stable@vger.kernel.org # v5.4 Suggested-by: Dan Carpenter <error27@gmail.com> Suggested-by: Takashi Iwai <tiwai@suse.de> Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr> --- lib/test_firmware.c | 10 ++++++++++ 1 file changed, 10 insertions(+)