linux-kernel - Re: [PATCH 2/2] Bluetooth: btbcm: automate node cleanup in btbcm_get_board

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <91b55df5-b023-4fbd-925e-7ac97f287ac4@kernel.org>
Date: Thu, 31 Oct 2024 12:46:21 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Javier Carrasco <javier.carrasco.cruz@...il.com>,
 Marcel Holtmann <marcel@...tmann.org>,
 Luiz Augusto von Dentz <luiz.dentz@...il.com>,
 Linus Walleij <linus.walleij@...aro.org>
Cc: linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] Bluetooth: btbcm: automate node cleanup in
 btbcm_get_board_name()

On 31/10/2024 12:41, Javier Carrasco wrote:
> On 31/10/2024 12:30, Krzysztof Kozlowski wrote:
>> On 31/10/2024 12:14, Krzysztof Kozlowski wrote:
>>> On 31/10/2024 12:10, Javier Carrasco wrote:
>>>> On 31/10/2024 12:08, Krzysztof Kozlowski wrote:
>>>>> On 30/10/2024 16:46, Javier Carrasco wrote:
>>>>>> Switch to a more robust approach by automating the node release when it
>>>>>> goes out of scope, removing the need for explicit calls to
>>>>>> of_node_put().
>>>>>>
>>>>>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@...il.com>
>>>>>> ---
>>>>>>  drivers/bluetooth/btbcm.c | 8 ++------
>>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
>>>>>> index 400c2663d6b0..a1153ada74d2 100644
>>>>>> --- a/drivers/bluetooth/btbcm.c
>>>>>> +++ b/drivers/bluetooth/btbcm.c
>>>>>> @@ -541,23 +541,19 @@ static const struct bcm_subver_table bcm_usb_subver_table[] = {
>>>>>>  static const char *btbcm_get_board_name(struct device *dev)
>>>>>>  {
>>>>>>  #ifdef CONFIG_OF
>>>>>> -	struct device_node *root;
>>>>>> +	struct device_node *root __free(device_node) = of_find_node_by_path("/");
>>>>>>  	char *board_type;
>>>>>>  	const char *tmp;
>>>>>>  
>>>>>> -	root = of_find_node_by_path("/");
>>>>>>  	if (!root)
>>>>>>  		return NULL;
>>>>>>  
>>>>>> -	if (of_property_read_string_index(root, "compatible", 0, &tmp)) {
>>>>>> -		of_node_put(root);
>>>>>
>>>>> You just added this. Don't add code which is immediately removed. It's a
>>>>> noop or wrong code.
>>>>>
>>>>>
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>>
>>>>
>>>> Exactly, I added that code to fix the issue in stable kernels that don't
>>>
>>> Then send backport for stable.
>>>
>>>> support the __free() macro, and then I removed it to use a safer
>>>> approach from now on.
>>>
>>> This is not correct approach. We work here on mainline and in mainline
>>> this is one logical change: fixing issue. Whether you fix issue with
>>> of_node_put or cleanup or by removing of_find_node_by_path() call, it
>>> does not matter. All of these are fixing the same, one issue.
>>>
>>> If you think about stable kernels, then work on backports, not inflate
>>> mainline kernel with multiple commits doing the same, creating
>>> artificial history.
>>>
>>
>> And to clarify even more: these stable backports are close to useless,
>> because it does not matter for them. No impact, not much benefits,
>> nothing improved for users/developers. There is no need to backport
>> them, although of course there is no loss by doing so. Therefore entire
>> dance affects mainline kernel without any real benefits for stable.
>>
>> Your split suggests you don't really know what this dropping reference
>> is for.
> 
> Such splits were suggested in other threads, and they came exactly for

You mention it third time, but never provided a link. I tried to look
briefly for it but failed. Can you share a lore link?

> those reasons: they could not be applied to stable. That was not my
> first approach, which was just using __free() to fix the issue. I am not
> looking forward to inflating any history, as that's in the end more work
> for me.
> 
> If a simple patch that adds the cleanup attribute is enough, that's
> awesome. I will go for that approach for all cases then, and use your
> explanation as a reference if I am asked to split the fix again.

If maintainer asks you to split trivial things like of_node_put() for
simple patches, feel free to Cc me, so I can provide counter arguments.

Best regards,
Krzysztof