Re: [PATCH v1 1/1] test_firmware: prevent race conditions by a correct implementation of locking

[Mirsad Todorovac]

On 01. 08. 2023. 11:57, Mirsad Todorovac wrote:
> On 8/1/23 10:24, Mirsad Todorovac wrote:
>> On 7/31/23 19:27, Luis Chamberlain wrote:
>>> On Mon, Jul 31, 2023 at 06:50:19PM +0200, Mirsad Todorovac wrote:
>>>> NOTE: This patch is tested against 5.4 stable
>>>>
>>>> NOTE: This is a patch for the 5.4 stable branch, not for the torvalds tree.
>>>>
>>>>        The torvalds tree, and stable tree 5.10, 5.15, 6.1 and 6.4 branches
>>>>        were fixed in the separate
>>>>        commit ID 4acfe3dfde68 ("test_firmware: prevent race conditions by a correct implementation of locking")
>>>>        which was incompatible with 5.4
>>>>
>>>
>>> The above part is not part of the original commit, you also forgot to
>>> mention the upstream commit:
>>>
>>> [ Upstream commit 4acfe3dfde685a5a9eaec5555351918e2d7266a1 ]
>>
>> Will fix. Actually, I wasn't sure if it was required, because this backported patch
>> isn't verbatim equal to commit 4acfe3dfde685a5a9eaec5555351918e2d7266a1 .
>>
>> Though they are cousins, addressing the same issue.
>>
>> There is a race to be fixed, despite not all racy functions present in the original commit c92316bf8e948.
>>
>>>> Fixes: c92316bf8e948 ("test_firmware: add batched firmware tests")
>>>> Cc: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
>>>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>>> Cc: Russ Weight <russell.h.weight@xxxxxxxxx>
>>>> Cc: Takashi Iwai <tiwai@xxxxxxx>
>>>> Cc: Tianfei Zhang <tianfei.zhang@xxxxxxxxx>
>>>> Cc: Shuah Khan <shuah@xxxxxxxxxx>
>>>> Cc: Colin Ian King <colin.i.king@xxxxxxxxx>
>>>> Cc: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
>>>> Cc: linux-kselftest@xxxxxxxxxxxxxxx
>>>> Cc: stable@xxxxxxxxxxxxxxx # v5.4
>>>> Suggested-by: Dan Carpenter <error27@xxxxxxxxx>
>>>
>>> Here you can add the above note in brackets:
>>>
>>> [ explain your changes here from the original commit ]
>>>
>>> Then, I see two commits upstream on Linus tree which are also fixes
>>> but not merged on v5.4, did you want those applied too?
>>
>> These seem merged in the stable 5.4?
>>
>> commit 75d9e00f65cd2e0f2ce9ceeb395f821976773489 test_firmware: fix a memory leak with reqs buffer
>> commit 94f3bc7e84af2f17dbfbc7afe93991c2a6f2f25e test_firmware: fix the memory leak of the allocated firmware buffer
>>
>> Maybe this commit should be backported instead:
>>
>> test_firmware: return ENOMEM instead of ENOSPC on failed memory allocation
>> [ Upstream commit 7dae593cd226a0bca61201cf85ceb9335cf63682 ]
>>
>> It was also merged into 6.4, 6.1, 5.15 and 5.10 stable, but not on 5.4
>>
>> I might also check whether the 4.19 and 4.14 are vulnerable to these memory leaks and this race
>> (Yes, they are, so it might be prudent that we backport this fix.)
> 
> FYI, just checked, the patch applied w/o modifications to 4.19 and 4.14 LTS stable branches.

Hi, Mr. Luis,

I tried to guess the best way how to backport these four patches:

48e156023059 test_firmware: fix the memory leak of the allocated firmware buffer
	5.4		[ALREADY IN THE TREE]
	4.1[49]		N/A
be37bed754ed test_firmware: fix a memory leak with reqs buffer
	5.4		[ALREADY IN THE TREE]
	4.19		https://lore.kernel.org/lkml/20230801170746.191505-1-mirsad.todorovac@xxxxxxxxxxxx/
	4.14		https://lore.kernel.org/lkml/20230802053253.667634-1-mirsad.todorovac@xxxxxxxxxxxx/
4acfe3dfde68 test_firmware: prevent race conditions by a correct implementation of locking
	5.4,4.19,4.14	https://lore.kernel.org/lkml/20230803165304.9200-1-mirsad.todorovac@xxxxxxxxxxxx/
7dae593cd226 test_firmware: return ENOMEM instead of ENOSPC on failed memory allocation
	5.4		https://lore.kernel.org/lkml/20230803165304.9200-2-mirsad.todorovac@xxxxxxxxxxxx/
	4.1[49]		https://lore.kernel.org/lkml/20230801185324.197544-1-mirsad.todorovac@xxxxxxxxxxxx/

I have tested the 5.4 and 4.19 builds, but 4.14 still won't boot at my hw (black screen,
no msgs at all to diagnose).

I hope you will manage between the patches that have the same name and version, but
address the backport to a different stable LTS branch. They differ by the patch proper,
naturally, to state the obvious, or the upstream would apply of course.

I don't know the exact formatting procedure for the backports, so I improvised, but I feel that backporting
bug fixes is very important, even if they are not security fixes.

I found no new weaknesses in the firmware driver after reviewing the code again. The buffer for name can be
released twice, though, but kfree(NULL) is permissible:

        kfree_const(test_fw_config->name);
        test_fw_config->name = NULL;

This about ends this chapter, and I am waiting for a review and an ACK.

Kind regards,
Mirsad Todorovac

-- 
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
The European Union

"I see something approaching fast ... Will it be friends with me?"