Re: [PATCH 1/7] irqchip: Fix potential resource leaks

Re: [PATCH 1/7] irqchip: Fix potential resource leaks

[Markus Elfring]

> There exists some potential resource leaks in the error path, fix them.

Will the tag “Fixes” become relevant for the commit message?


…
> +++ b/drivers/irqchip/irq-nvic.c
> @@ -94,6 +94,7 @@ static int __init nvic_of_init(struct device_node *node,
>
>  	if (!nvic_irq_domain) {
>  		pr_warn("Failed to allocate irq domain\n");
> +		iounmap(nvic_base);
>  		return -ENOMEM;
>  	}
>
> @@ -103,6 +104,7 @@ static int __init nvic_of_init(struct device_node *node,
>  	if (ret) {
>  		pr_warn("Failed to allocate irq chips\n");
>  		irq_domain_remove(nvic_irq_domain);
> +		iounmap(nvic_base);
>  		return ret;
>  	}

Can it helpful to add jump targets so that a bit of exception handling
can be better reused at the end of this function?

Regards,
Markus

Re: [PATCH 1/7] irqchip: Fix potential resource leaks

[Tiezhu Yang]

On 06/23/2020 11:55 PM, Markus Elfring wrote:
>> There exists some potential resource leaks in the error path, fix them.
> Will the tag “Fixes” become relevant for the commit message?

Hi Markus,

Thanks for your reply and suggestion.

This patch contains many files in drivers/irqchip,
maybe I should split it into some small patches if use the tag "Fixes"?

>
>
> …
>> +++ b/drivers/irqchip/irq-nvic.c
>> @@ -94,6 +94,7 @@ static int __init nvic_of_init(struct device_node *node,
>>
>>   	if (!nvic_irq_domain) {
>>   		pr_warn("Failed to allocate irq domain\n");
>> +		iounmap(nvic_base);
>>   		return -ENOMEM;
>>   	}
>>
>> @@ -103,6 +104,7 @@ static int __init nvic_of_init(struct device_node *node,
>>   	if (ret) {
>>   		pr_warn("Failed to allocate irq chips\n");
>>   		irq_domain_remove(nvic_irq_domain);
>> +		iounmap(nvic_base);
>>   		return ret;
>>   	}
> Can it helpful to add jump targets so that a bit of exception handling
> can be better reused at the end of this function?

OK, no problem, I will do it in the v2.

By the way, I have resent this patch series due to git send-email failed,
https://lore.kernel.org/patchwork/cover/1261517/

Thanks,
Tiezhu

>
> Regards,
> Markus

Re: [1/7] irqchip: Fix potential resource leaks

[Markus Elfring]

>> Can it helpful to add jump targets so that a bit of exception handling
>> can be better reused at the end of this function?
>
> OK, no problem, I will do it in the v2.

It seems that the software evolution will be continued with another
update suggestion like the following.

[PATCH v3 10/14 RESEND] irqchip/nvic: Fix potential resource leaks
https://lore.kernel.org/linux-mips/1592984711-3130-11-git-send-email-yangtiezhu@loongson.cn/
https://lore.kernel.org/patchwork/patch/1263191/


Can it matter to omit the word “potential” from change descriptions
after you detected that specific function calls were missing
in if branches?

Regards,
Markus

Re: [1/7] irqchip: Fix potential resource leaks

[Tiezhu Yang]

On 06/24/2020 04:42 PM, Markus Elfring wrote:
>>> Can it helpful to add jump targets so that a bit of exception handling
>>> can be better reused at the end of this function?
>> OK, no problem, I will do it in the v2.
> It seems that the software evolution will be continued with another
> update suggestion like the following.
>
> [PATCH v3 10/14 RESEND] irqchip/nvic: Fix potential resource leaks
> https://lore.kernel.org/linux-mips/1592984711-3130-11-git-send-email-yangtiezhu@loongson.cn/
> https://lore.kernel.org/patchwork/patch/1263191/
>
>
> Can it matter to omit the word “potential” from change descriptions
> after you detected that specific function calls were missing
> in if branches?

Oh, I find this issue through code review, I have no test environment
to trigger the error path, but I think it is better to release the resource
in the error path, so I use "potential" description.

>
> Regards,
> Markus

Re: [1/7] irqchip: Fix potential resource leaks

[Markus Elfring]

>> [PATCH v3 10/14 RESEND] irqchip/nvic: Fix potential resource leaks
>> https://lore.kernel.org/linux-mips/1592984711-3130-11-git-send-email-yangtiezhu@loongson.cn/
>> https://lore.kernel.org/patchwork/patch/1263191/
>>
>>
>> Can it matter to omit the word “potential” from change descriptions
>> after you detected that specific function calls were missing
>> in if branches?
>
> Oh, I find this issue through code review, I have no test environment
> to trigger the error path, but I think it is better to release the resource
> in the error path, so I use "potential" description.

Did you determine that special function calls were generally missing
in error cases?

Were any known software analysis tools involved for the detection of
questionable source code places?

Regards,
Markus

Re: [1/7] irqchip: Fix potential resource leaks

[Tiezhu Yang]

On 06/24/2020 05:23 PM, Markus Elfring wrote:
>>> [PATCH v3 10/14 RESEND] irqchip/nvic: Fix potential resource leaks
>>> https://lore.kernel.org/linux-mips/1592984711-3130-11-git-send-email-yangtiezhu@loongson.cn/
>>> https://lore.kernel.org/patchwork/patch/1263191/
>>>
>>>
>>> Can it matter to omit the word “potential” from change descriptions
>>> after you detected that specific function calls were missing
>>> in if branches?
>> Oh, I find this issue through code review, I have no test environment
>> to trigger the error path, but I think it is better to release the resource
>> in the error path, so I use "potential" description.
> Did you determine that special function calls were generally missing
> in error cases?

Yes, I read many files in drivers/irqchip,
the resource is released in the error path.

>
> Were any known software analysis tools involved for the detection of
> questionable source code places?

kmemleak can detect memory leak,
but I do not know how to detect other kind of leaks.
I think consciously release resource in the error path can avoid leaks.

>
> Regards,
> Markus

Re: [1/7] irqchip: Fix potential resource leaks

[Markus Elfring]

>> Were any known software analysis tools involved for the detection of
>> questionable source code places?
>
> kmemleak can detect memory leak,
> but I do not know how to detect other kind of leaks.

How do you think about to extend source code analysis tools accordingly?


> I think consciously release resource in the error path can avoid leaks.

Is it often too easy to overlook relevant function calls?

Regards,
Markus

Re: [1/7] irqchip: Fix potential resource leaks

[Tiezhu Yang]

On 06/24/2020 06:06 PM, Markus Elfring wrote:
>>> Were any known software analysis tools involved for the detection of
>>> questionable source code places?
>> kmemleak can detect memory leak,
>> but I do not know how to detect other kind of leaks.
> How do you think about to extend source code analysis tools accordingly?

I have no good idea,
maybe some simple match check tools can do this.

>
>
>> I think consciously release resource in the error path can avoid leaks.
> Is it often too easy to overlook relevant function calls?

Yes,  I think code review can avoid this issue in some certain degree.

>
> Regards,
> Markus

Re: [1/7] irqchip: Fix potential resource leaks

[Markus Elfring]

>> How do you think about to extend source code analysis tools accordingly?
>
> I have no good idea,
> maybe some simple match check tools can do this.

Would you like to help with any additional software development resources
(besides your current contribution)?

Have you heard anything according to recent research (from computer science)
for this application domain?

Regards,
Markus

Re: [1/7] irqchip: Fix potential resource leaks

[Tiezhu Yang]

On 06/24/2020 08:08 PM, Markus Elfring wrote:
>>> How do you think about to extend source code analysis tools accordingly?
>> I have no good idea,
>> maybe some simple match check tools can do this.
> Would you like to help with any additional software development resources
> (besides your current contribution)?

I am glad to do it in my spare time.

>
> Have you heard anything according to recent research (from computer science)
> for this application domain?

No.

>
> Regards,
> Markus