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 <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russ Weight <[email protected]>
Cc: Tianfei Zhang <[email protected]>
Cc: Shuah Khan <[email protected]>
Cc: Colin Ian King <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: [email protected]
Cc: [email protected] # v5.4
Suggested-by: Dan Carpenter <[email protected]>
Suggested-by: Takashi Iwai <[email protected]>
Signed-off-by: Mirsad Goran Todorovac <[email protected]>
---
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));
--
2.30.2
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 <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Russ Weight <[email protected]>
> Cc: Tianfei Zhang <[email protected]>
> Cc: Shuah Khan <[email protected]>
> Cc: Colin Ian King <[email protected]>
> Cc: Randy Dunlap <[email protected]>
> Cc: [email protected]
> Cc: [email protected] # v5.4
> Suggested-by: Dan Carpenter <[email protected]>
> Suggested-by: Takashi Iwai <[email protected]>
> Signed-off-by: Mirsad Goran Todorovac <[email protected]>
> ---
> 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
--
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
"What’s this thing suddenly coming towards me very fast? Very very fast.
... I wonder if it will be friends with me?"
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
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 <[email protected]>
> 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 <[email protected]>
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 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 <[email protected]>
>
Thanks for doing this work! It looks much better now split up!
For all 3 patches:
Acked-by: Luis Chamberlain <[email protected]>
Greg, can you pick these up?
Luis
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 <[email protected]>
>>
>
> 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 <[email protected]>
Thanks,
Mirsad
> Greg, can you pick these up?
>
> Luis