From: Markus Elfring <[email protected]>
Date: Sun, 16 Nov 2014 12:20:31 +0100
The module_put() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
kernel/kprobes.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 3995f54..f1e7d45 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1527,8 +1527,7 @@ int register_kprobe(struct kprobe *p)
out:
mutex_unlock(&kprobe_mutex);
- if (probed_mod)
- module_put(probed_mod);
+ module_put(probed_mod);
return ret;
}
--
2.1.3
On Sun, 16 Nov 2014, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Sun, 16 Nov 2014 12:20:31 +0100
>
> The module_put() function tests whether its argument is NULL and then
> returns immediately. Thus the test around the call is not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> kernel/kprobes.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 3995f54..f1e7d45 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1527,8 +1527,7 @@ int register_kprobe(struct kprobe *p)
> out:
> mutex_unlock(&kprobe_mutex);
>
> - if (probed_mod)
> - module_put(probed_mod);
> + module_put(probed_mod);
There is an out label, so please check whether the labels could not be
better positioned to avoid calling module_put when it is not needed.
julia
>
> return ret;
> }
> --
> 2.1.3
>
>
> _______________________________________________
> Cocci mailing list
> [email protected]
> https://systeme.lip6.fr/mailman/listinfo/cocci
>
>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> index 3995f54..f1e7d45 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
>> @@ -1527,8 +1527,7 @@ int register_kprobe(struct kprobe *p)
>> out:
>> mutex_unlock(&kprobe_mutex);
>>
>> - if (probed_mod)
>> - module_put(probed_mod);
>> + module_put(probed_mod);
>
> There is an out label, so please check whether the labels could not be
> better positioned to avoid calling module_put when it is not needed.
I do not see refactoring opportunities around jump labels in this use case
for the implementation of the register_kprobe() function so far because
the mutex_unlock() function must be called.
Would you like to suggest any other source code fine-tuning?
Regards,
Markus
On Sun, 16 Nov 2014, SF Markus Elfring wrote:
> >> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> >> index 3995f54..f1e7d45 100644
> >> --- a/kernel/kprobes.c
> >> +++ b/kernel/kprobes.c
> >> @@ -1527,8 +1527,7 @@ int register_kprobe(struct kprobe *p)
> >> out:
> >> mutex_unlock(&kprobe_mutex);
> >>
> >> - if (probed_mod)
> >> - module_put(probed_mod);
> >> + module_put(probed_mod);
> >
> > There is an out label, so please check whether the labels could not be
> > better positioned to avoid calling module_put when it is not needed.
>
> I do not see refactoring opportunities around jump labels in this use case
> for the implementation of the register_kprobe() function so far because
> the mutex_unlock() function must be called.
> Would you like to suggest any other source code fine-tuning?
OK. I don't think that removing the if is a good choice in this case.
The code ret = check_kprobe_address_safe(p, &probed_mod); is unusual, in
that it can fail to do anything in two ways. One is by setting ret, on
detecting an error, and the other is by returning 0 but still putting a
NULL value in probed_mod when there is nothing to do. Thus, in the
successful execution of the rest of the function, a probed module might or
might not exist. The if around the module_put is helpful to the reader to
understand that this possibility exists.
julia
> The if around the module_put is helpful to the reader to understand
> that this possibility exists.
I have got a different opinion. I would still prefer a small code clean-up there.
Regards,
Markus
(2014/11/16 21:34), SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Sun, 16 Nov 2014 12:20:31 +0100
>
> The module_put() function tests whether its argument is NULL and then
> returns immediately. Thus the test around the call is not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> kernel/kprobes.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 3995f54..f1e7d45 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1527,8 +1527,7 @@ int register_kprobe(struct kprobe *p)
> out:
> mutex_unlock(&kprobe_mutex);
>
> - if (probed_mod)
> - module_put(probed_mod);
> + module_put(probed_mod);
This is OK, but I you request a comment line over there so that
code reader can understand it is safe to pass a NULL pointer to
module_put().
# and of course don't touch jump label around that :)
Thank you,
>
> return ret;
> }
>
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
>> index 3995f54..f1e7d45 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
>> @@ -1527,8 +1527,7 @@ int register_kprobe(struct kprobe *p)
>> out:
>> mutex_unlock(&kprobe_mutex);
>>
>> - if (probed_mod)
>> - module_put(probed_mod);
>> + module_put(probed_mod);
>
> This is OK, but I you request a comment line over there so that
> code reader can understand it is safe to pass a NULL pointer to
> module_put().
Do you want that I replace the shown null pointer check by a short
comment which repeats an expectation for the affected function call?
Regards,
Markus
(2014/11/19 16:08), SF Markus Elfring wrote:
>>> index 3995f54..f1e7d45 100644
>>> --- a/kernel/kprobes.c
>>> +++ b/kernel/kprobes.c
>>> @@ -1527,8 +1527,7 @@ int register_kprobe(struct kprobe *p)
>>> out:
>>> mutex_unlock(&kprobe_mutex);
>>>
>>> - if (probed_mod)
>>> - module_put(probed_mod);
>>> + module_put(probed_mod);
>>
>> This is OK, but I you request a comment line over there so that
>> code reader can understand it is safe to pass a NULL pointer to
>> module_put().
>
> Do you want that I replace the shown null pointer check by a short
> comment which repeats an expectation for the affected function call?
No, not "want". IMHO, if try_module_get(mod) is done only when mod!=NULL,
we shouldn't call module_put(mod) when mod==NULL (even if it is possible),
because those get/put method must be used as a pair, for the better
understandings.
Thank you,
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
From: Markus Elfring <[email protected]>
Date: Sat, 4 Jul 2015 10:00:26 +0200
The module_put() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
kernel/kprobes.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index c90e417..52e3529 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1531,10 +1531,7 @@ int register_kprobe(struct kprobe *p)
out:
mutex_unlock(&kprobe_mutex);
-
- if (probed_mod)
- module_put(probed_mod);
-
+ module_put(probed_mod);
return ret;
}
EXPORT_SYMBOL_GPL(register_kprobe);
--
2.4.5
On 2015/07/04 17:10, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Sat, 4 Jul 2015 10:00:26 +0200
>
> The module_put() function tests whether its argument is NULL and then
> returns immediately. Thus the test around the call is not needed.
>
> This issue was detected by using the Coccinelle software.
>
OK, looks good to me.
Acked-by: Masami Hiramatsu <[email protected]>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> kernel/kprobes.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index c90e417..52e3529 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1531,10 +1531,7 @@ int register_kprobe(struct kprobe *p)
>
> out:
> mutex_unlock(&kprobe_mutex);
> -
> - if (probed_mod)
> - module_put(probed_mod);
> -
> + module_put(probed_mod);
> return ret;
> }
> EXPORT_SYMBOL_GPL(register_kprobe);
>
--
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: [email protected]