From: Markus Elfring <[email protected]>
Date: Mon, 1 Jan 2024 21:52:12 +0100
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (2):
Improve unlocking
Improve error handling
net/iucv/iucv.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
--
2.43.0
From: Markus Elfring <[email protected]>
Date: Mon, 1 Jan 2024 21:15:11 +0100
* Add a label so that a call of the function “cpus_read_unlock”
is stored only once in this function implementation.
* Replace one call at the end by a goto statement.
Signed-off-by: Markus Elfring <[email protected]>
---
net/iucv/iucv.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
index 0ed6e34d6edd..71ba309e05ee 100644
--- a/net/iucv/iucv.c
+++ b/net/iucv/iucv.c
@@ -555,13 +555,16 @@ static int iucv_enable(void)
if (cpumask_empty(&iucv_buffer_cpumask))
/* No cpu could declare an iucv buffer. */
goto out;
+
+ rc = 0;
+unlock:
cpus_read_unlock();
- return 0;
+ return rc;
+
out:
kfree(iucv_path_table);
iucv_path_table = NULL;
- cpus_read_unlock();
- return rc;
+ goto unlock;
}
/*
--
2.43.0
From: Markus Elfring <[email protected]>
Date: Mon, 1 Jan 2024 21:44:46 +0100
The kfree() function was called in one case during error handling
even if the passed variable contained a null pointer.
This issue was detected by using the Coccinelle software.
* Thus achieve an unlock operation by using the corresponding label.
* Move two error code assignments to other places.
Signed-off-by: Markus Elfring <[email protected]>
---
net/iucv/iucv.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
index 71ba309e05ee..09e78a57bab8 100644
--- a/net/iucv/iucv.c
+++ b/net/iucv/iucv.c
@@ -543,13 +543,14 @@ static int iucv_enable(void)
int cpu, rc;
cpus_read_lock();
- rc = -ENOMEM;
alloc_size = iucv_max_pathid * sizeof(struct iucv_path);
iucv_path_table = kzalloc(alloc_size, GFP_KERNEL);
- if (!iucv_path_table)
- goto out;
+ if (!iucv_path_table) {
+ rc = -ENOMEM;
+ goto unlock;
+ }
+
/* Declare per cpu buffers. */
- rc = -EIO;
for_each_online_cpu(cpu)
smp_call_function_single(cpu, iucv_declare_cpu, NULL, 1);
if (cpumask_empty(&iucv_buffer_cpumask))
@@ -564,6 +565,7 @@ static int iucv_enable(void)
out:
kfree(iucv_path_table);
iucv_path_table = NULL;
+ rc = -EIO;
goto unlock;
}
--
2.43.0
@@ -555,13 +555,16 @@ static int iucv_enable(void)
> if (cpumask_empty(&iucv_buffer_cpumask))
> /* No cpu could declare an iucv buffer. */
> goto out;
>+
>+ rc = 0;
>+unlock:
> cpus_read_unlock();
>- return 0;
>+ return rc;
>+
> out:
> kfree(iucv_path_table);
> iucv_path_table = NULL;
>- cpus_read_unlock();
>- return rc;
>+ goto unlock;
[Suman] This looks confusing. What is the issue with retaining the original change?
> }
>
> /*
>--
>2.43.0
>
> @@ -555,13 +555,16 @@ static int iucv_enable(void)
>> if (cpumask_empty(&iucv_buffer_cpumask))
>> /* No cpu could declare an iucv buffer. */
>> goto out;
>> +
>> + rc = 0;
>> +unlock:
>> cpus_read_unlock();
>> - return 0;
>> + return rc;
>> +
>> out:
>> kfree(iucv_path_table);
>> iucv_path_table = NULL;
>> - cpus_read_unlock();
>> - return rc;
>> + goto unlock;
> [Suman] This looks confusing. What is the issue with retaining the original change?
I propose to reduce the number of cpus_read_unlock() calls
(in the source code).
Regards,
Markus
>>> if (cpumask_empty(&iucv_buffer_cpumask))
>>> /* No cpu could declare an iucv buffer. */
>>> goto out;
>>> +
>>> + rc = 0;
>>> +unlock:
>>> cpus_read_unlock();
>>> - return 0;
>>> + return rc;
>>> +
>>> out:
>>> kfree(iucv_path_table);
>>> iucv_path_table = NULL;
>>> - cpus_read_unlock();
>>> - return rc;
>>> + goto unlock;
>> [Suman] This looks confusing. What is the issue with retaining the
>original change?
>
>I propose to reduce the number of cpus_read_unlock() calls (in the
>source code).
>
>Regards,
>Markus
[Suman] Then I think we should do something like this. Changing the code flow back-and-forth using "goto" does not seem correct.
static int iucv_enable(void)
{
size_t alloc_size;
int cpu, rc = 0;
cpus_read_lock();
alloc_size = iucv_max_pathid * sizeof(struct iucv_path);
iucv_path_table = kzalloc(alloc_size, GFP_KERNEL);
if (!iucv_path_table) {
rc = -ENOMEM;
goto out;
}
/* Declare per cpu buffers. */
for_each_online_cpu(cpu)
smp_call_function_single(cpu, iucv_declare_cpu, NULL, 1);
if (cpumask_empty(&iucv_buffer_cpumask))
/* No cpu could declare an iucv buffer. */
rc = -EIO;
out:
if (rc) {
kfree(iucv_path_table); //kfree is itself NULL protected. So, kzalloc failure should also be handled.
iucv_path_table = NULL;
}
cpus_read_unlock();
return rc;
}
On 02.01.24 09:27, Suman Ghosh wrote:
>>> [Suman] This looks confusing. What is the issue with retaining the
>> original change?
>>
>> I propose to reduce the number of cpus_read_unlock() calls (in the
>> source code).
>>
>> Regards,
>> Markus
> [Suman] Then I think we should do something like this. Changing the code flow back-and-forth using "goto" does not seem correct.
I share Suman's concern that jumping backwards goto is confusing.
But I think the Coccinelle finding of freeing a null-pointer should be addressed (see patch 2/2)
Thank you Markus for reporting it.
The allocation does require holding the cpus_read_lock.
For some reason Markus wants to reduce the number of cpus_read_unlock() calls (why?),
so what about something like this for both issues:
diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
index 0ed6e34d6edd..1030403b826b 100644
--- a/net/iucv/iucv.c
+++ b/net/iucv/iucv.c
@@ -542,24 +542,22 @@ static int iucv_enable(void)
size_t alloc_size;
int cpu, rc;
- cpus_read_lock();
- rc = -ENOMEM;
alloc_size = iucv_max_pathid * sizeof(struct iucv_path);
iucv_path_table = kzalloc(alloc_size, GFP_KERNEL);
if (!iucv_path_table)
- goto out;
+ return -ENOMEM;
/* Declare per cpu buffers. */
- rc = -EIO;
+ cpus_read_lock();
for_each_online_cpu(cpu)
smp_call_function_single(cpu, iucv_declare_cpu, NULL, 1);
- if (cpumask_empty(&iucv_buffer_cpumask))
+ if (cpumask_empty(&iucv_buffer_cpumask)) {
/* No cpu could declare an iucv buffer. */
- goto out;
- cpus_read_unlock();
- return 0;
-out:
- kfree(iucv_path_table);
- iucv_path_table = NULL;
+ kfree(iucv_path_table);
+ iucv_path_table = NULL;
+ rc = -EIO;
+ } else {
+ rc = 0;
+ }
cpus_read_unlock();
return rc;
}
> I share Suman's concern that jumping backwards goto is confusing.
> But I think the Coccinelle finding of freeing a null-pointer should be addressed (see patch 2/2)
> Thank you Markus for reporting it.
>
> The allocation does require holding the cpus_read_lock.
How does this information fit to your following suggestion to adjust the lock scope?
> For some reason Markus wants to reduce the number of cpus_read_unlock() calls (why?),
One cpus_read_unlock() call is required here.
Would you like to benefit more from a smaller executable code size?
> so what about something like this for both issues:
>
> diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
> index 0ed6e34d6edd..1030403b826b 100644
> --- a/net/iucv/iucv.c
> +++ b/net/iucv/iucv.c
> @@ -542,24 +542,22 @@ static int iucv_enable(void)
> size_t alloc_size;
> int cpu, rc;
>
> - cpus_read_lock();
> - rc = -ENOMEM;
> alloc_size = iucv_max_pathid * sizeof(struct iucv_path);
> iucv_path_table = kzalloc(alloc_size, GFP_KERNEL);
> if (!iucv_path_table)
> - goto out;
> + return -ENOMEM;
> /* Declare per cpu buffers. */
> - rc = -EIO;
> + cpus_read_lock();
> for_each_online_cpu(cpu)
> smp_call_function_single(cpu, iucv_declare_cpu, NULL, 1);
> - if (cpumask_empty(&iucv_buffer_cpumask))
> + if (cpumask_empty(&iucv_buffer_cpumask)) {
> /* No cpu could declare an iucv buffer. */
> - goto out;
> - cpus_read_unlock();
> - return 0;
> -out:
> - kfree(iucv_path_table);
> - iucv_path_table = NULL;
> + kfree(iucv_path_table);
> + iucv_path_table = NULL;
> + rc = -EIO;
> + } else {
> + rc = 0;
> + }
> cpus_read_unlock();
> return rc;
> }
I suggest to reconsider patch squashing a bit more.
Regards,
Markus