[ commit be37bed754ed90b2655382f93f9724b3c1aae847 upstream ]
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 triggered 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: c92316bf8e94 ("test_firmware: add batched firmware tests")
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] # v4.14
Suggested-by: Dan Carpenter <[email protected]>
Suggested-by: Takashi Iwai <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Mirsad Todorovac <[email protected]>
[ This fix is applied against the 4.14 stable branch. There are no changes to the ]
[ fix in code when compared to the upstread, only the reformatting for backport. ]
---
v2 -> v3:
minor clarifications in the versioning for the patchwork. not change to commit.
v1 -> v2:
removed the Reviewed-by: and Acked-by tags, as this is a slightly different patch and
those need to be reacquired
lib/test_firmware.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 1c5e5246bf10..5318c5e18acf 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -621,6 +621,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(sizeof(struct test_batched_req) *
test_fw_config->num_requests * 2);
if (!test_fw_config->reqs) {
@@ -723,6 +728,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(sizeof(struct test_batched_req) *
test_fw_config->num_requests * 2);
if (!test_fw_config->reqs) {
--
2.34.1
On Fri, Aug 04, 2023 at 07:00:18PM +0200, Mirsad Todorovac wrote:
> [ commit be37bed754ed90b2655382f93f9724b3c1aae847 upstream ]
>
> 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 triggered 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: c92316bf8e94 ("test_firmware: add batched firmware tests")
> 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] # v4.14
> Suggested-by: Dan Carpenter <[email protected]>
> Suggested-by: Takashi Iwai <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> Signed-off-by: Mirsad Todorovac <[email protected]>
>
> [ This fix is applied against the 4.14 stable branch. There are no changes to the ]
> [ fix in code when compared to the upstread, only the reformatting for backport. ]
Thanks for all of these, now queued up.
greg k-h
On 8/7/23 11:15, Greg Kroah-Hartman wrote:
> On Fri, Aug 04, 2023 at 07:00:18PM +0200, Mirsad Todorovac wrote:
>> [ commit be37bed754ed90b2655382f93f9724b3c1aae847 upstream ]
>>
>> 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 triggered 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: c92316bf8e94 ("test_firmware: add batched firmware tests")
>> 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] # v4.14
>> Suggested-by: Dan Carpenter <[email protected]>
>> Suggested-by: Takashi Iwai <[email protected]>
>> Link: https://lore.kernel.org/r/[email protected]
>> Signed-off-by: Mirsad Todorovac <[email protected]>
>>
>> [ This fix is applied against the 4.14 stable branch. There are no changes to the ]
>> [ fix in code when compared to the upstread, only the reformatting for backport. ]
>
> Thanks for all of these, now queued up.
No problem, I should have done it right the first time to reduce your load.
I really believe that backporting bug fix patches is important because many systems
cannot upgrade because of the legacy apps and hardware, to state the obvious.
I know because we are in that group. :-)
Kind regards,
Mirsad Todorovac
On 8/8/23 06:28, Greg Kroah-Hartman wrote:
> On Mon, Aug 07, 2023 at 08:28:04PM +0200, Mirsad Todorovac wrote:
>> On 8/7/23 11:15, Greg Kroah-Hartman wrote:
>>> On Fri, Aug 04, 2023 at 07:00:18PM +0200, Mirsad Todorovac wrote:
>>>> [ commit be37bed754ed90b2655382f93f9724b3c1aae847 upstream ]
>>>>
>>>> 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 triggered 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: c92316bf8e94 ("test_firmware: add batched firmware tests")
>>>> 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] # v4.14
>>>> Suggested-by: Dan Carpenter <[email protected]>
>>>> Suggested-by: Takashi Iwai <[email protected]>
>>>> Link: https://lore.kernel.org/r/[email protected]
>>>> Signed-off-by: Mirsad Todorovac <[email protected]>
>>>>
>>>> [ This fix is applied against the 4.14 stable branch. There are no changes to the ]
>>>> [ fix in code when compared to the upstread, only the reformatting for backport. ]
>>>
>>> Thanks for all of these, now queued up.
>>
>> No problem, I should have done it right the first time to reduce your load.
>>
>> I really believe that backporting bug fix patches is important because many systems
>> cannot upgrade because of the legacy apps and hardware, to state the obvious.
>
> What "legacy apps" rely on a specific kernel version?
Hi, Mr. Greg,
Actually, in our particular case, it was the Eprints that required old mysql on Debian stretch
rather than MariaDB that came with Buster. So, the release required particular kernel version (4.9).
Of course, we can upgrade to any mainline kernel, but that is no longer a tested distro kernel,
and faults would be blamed on me entirely. Plus the overhead of regular patching ...
This is what I now do, but the old hardware at work still requires 60 minutes to build a decent
vanilla tree kernel ...
> As for hardware, just get the needed drivers merged into Linus's tree
> and then you can upgrade to newer kernels. What is preventing that from
> happening?
As I said, the problem is with reliability and testing. We used to have only the production virtual
servers w/o the testing ones, so the environment would be difficult to reproduce w/o interrupting
the services we run because of.
The Croatian universities are traditionally scarce in equipment. :-(
Kind regards,
Mirsad
> thanks,
>
> greg k-h
On Tue, Aug 08, 2023 at 08:24:43AM +0200, Mirsad Todorovac wrote:
> On 8/8/23 06:28, Greg Kroah-Hartman wrote:
> > On Mon, Aug 07, 2023 at 08:28:04PM +0200, Mirsad Todorovac wrote:
> > > On 8/7/23 11:15, Greg Kroah-Hartman wrote:
> > > > On Fri, Aug 04, 2023 at 07:00:18PM +0200, Mirsad Todorovac wrote:
> > > > > [ commit be37bed754ed90b2655382f93f9724b3c1aae847 upstream ]
> > > > >
> > > > > 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 triggered 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: c92316bf8e94 ("test_firmware: add batched firmware tests")
> > > > > 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] # v4.14
> > > > > Suggested-by: Dan Carpenter <[email protected]>
> > > > > Suggested-by: Takashi Iwai <[email protected]>
> > > > > Link: https://lore.kernel.org/r/[email protected]
> > > > > Signed-off-by: Mirsad Todorovac <[email protected]>
> > > > >
> > > > > [ This fix is applied against the 4.14 stable branch. There are no changes to the ]
> > > > > [ fix in code when compared to the upstread, only the reformatting for backport. ]
> > > >
> > > > Thanks for all of these, now queued up.
> > >
> > > No problem, I should have done it right the first time to reduce your load.
> > >
> > > I really believe that backporting bug fix patches is important because many systems
> > > cannot upgrade because of the legacy apps and hardware, to state the obvious.
> >
> > What "legacy apps" rely on a specific kernel version?
>
> Hi, Mr. Greg,
>
> Actually, in our particular case, it was the Eprints that required old mysql on Debian stretch
> rather than MariaDB that came with Buster. So, the release required particular kernel version (4.9).
So what happens when this kernel becomes end-of-life?
> Of course, we can upgrade to any mainline kernel, but that is no longer a tested distro kernel,
> and faults would be blamed on me entirely. Plus the overhead of regular patching ...
You should be doing regular patching for any LTS kernel as well, right?
Same for testing, there should not be any difference in testing any
kernel update be it on a LTS branch, or between major versions.
anyway, good luck!
greg k-h
On Mon, Aug 07, 2023 at 08:28:04PM +0200, Mirsad Todorovac wrote:
> On 8/7/23 11:15, Greg Kroah-Hartman wrote:
> > On Fri, Aug 04, 2023 at 07:00:18PM +0200, Mirsad Todorovac wrote:
> > > [ commit be37bed754ed90b2655382f93f9724b3c1aae847 upstream ]
> > >
> > > 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 triggered 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: c92316bf8e94 ("test_firmware: add batched firmware tests")
> > > 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] # v4.14
> > > Suggested-by: Dan Carpenter <[email protected]>
> > > Suggested-by: Takashi Iwai <[email protected]>
> > > Link: https://lore.kernel.org/r/[email protected]
> > > Signed-off-by: Mirsad Todorovac <[email protected]>
> > >
> > > [ This fix is applied against the 4.14 stable branch. There are no changes to the ]
> > > [ fix in code when compared to the upstread, only the reformatting for backport. ]
> >
> > Thanks for all of these, now queued up.
>
> No problem, I should have done it right the first time to reduce your load.
>
> I really believe that backporting bug fix patches is important because many systems
> cannot upgrade because of the legacy apps and hardware, to state the obvious.
What "legacy apps" rely on a specific kernel version?
As for hardware, just get the needed drivers merged into Linus's tree
and then you can upgrade to newer kernels. What is preventing that from
happening?
thanks,
greg k-h
On 8/8/23 09:35, Greg Kroah-Hartman wrote:
> On Tue, Aug 08, 2023 at 08:24:43AM +0200, Mirsad Todorovac wrote:
>> On 8/8/23 06:28, Greg Kroah-Hartman wrote:
>>> On Mon, Aug 07, 2023 at 08:28:04PM +0200, Mirsad Todorovac wrote:
>>>> On 8/7/23 11:15, Greg Kroah-Hartman wrote:
>>>>> On Fri, Aug 04, 2023 at 07:00:18PM +0200, Mirsad Todorovac wrote:
>>>>>> [ commit be37bed754ed90b2655382f93f9724b3c1aae847 upstream ]
>>>>>>
>>>>>> 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 triggered 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: c92316bf8e94 ("test_firmware: add batched firmware tests")
>>>>>> 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] # v4.14
>>>>>> Suggested-by: Dan Carpenter <[email protected]>
>>>>>> Suggested-by: Takashi Iwai <[email protected]>
>>>>>> Link: https://lore.kernel.org/r/[email protected]
>>>>>> Signed-off-by: Mirsad Todorovac <[email protected]>
>>>>>>
>>>>>> [ This fix is applied against the 4.14 stable branch. There are no changes to the ]
>>>>>> [ fix in code when compared to the upstread, only the reformatting for backport. ]
>>>>>
>>>>> Thanks for all of these, now queued up.
>>>>
>>>> No problem, I should have done it right the first time to reduce your load.
>>>>
>>>> I really believe that backporting bug fix patches is important because many systems
>>>> cannot upgrade because of the legacy apps and hardware, to state the obvious.
>>>
>>> What "legacy apps" rely on a specific kernel version?
>>
>> Hi, Mr. Greg,
>>
>> Actually, in our particular case, it was the Eprints that required old mysql on Debian stretch
>> rather than MariaDB that came with Buster. So, the release required particular kernel version (4.9).
>
> So what happens when this kernel becomes end-of-life?
I guess by now I could maintain the 4.19 line, with the bug fixes and the security fixes,
but it would impose significant overhead to already overwhelmed IT department.
I could use the same config and produce the same kernel, but w/o the testing as it would
happen w the distro kernels.
>> Of course, we can upgrade to any mainline kernel, but that is no longer a tested distro kernel,
>> and faults would be blamed on me entirely. Plus the overhead of regular patching ...
>
> You should be doing regular patching for any LTS kernel as well, right?
> Same for testing, there should not be any difference in testing any
> kernel update be it on a LTS branch, or between major versions.
Sure, but apt-get dist-upgrade is easier than rebuilding the kernel. I say, we'd have to
get the necessary "blessings" to make this routine or procedure. Now we have the machines
that could build a recent kernel in less than an hour, but it wasn't always so :-)
We still do not have a twin test server for each single one of our production releases.
> anyway, good luck!
Thanks, I think we'll need it.
Kind regards,
Mirsad Todorovac