Re: [PATCH] kernel: kprobe: move all *kretprobe* generic implementation to CONFIG_KRETPROBES enabled area

[Chen Gang <gang.chen.5i5j@gmail.com>]

On 02/05/2014 12:57 PM, Masami Hiramatsu wrote:
> (2014/02/05 12:08), Chen Gang wrote:
>>>>>>>>> Anyway, I don't think those inlined functions to be changed, because
>>>>>>>>> most of them are internal functions. If CONFIG_KRETPROBES=n, it just
>>>>>>>>> be ignored.
>>>>>>>>>
>>>>>>>>
>>>>>>>> In original implementation, if CONFIG_KRETPROBES=n, kretprobe_assert(),
>>>>>>>> disable_kretprobe(), and enable_kretprobe() are not ignored.
>>>>>>>
>>>>>>> Really? where are they called? I mean, those functions do not have
>>>>>>> any instance unless your module uses it (but that is not what the kernel
>>>>>>> itself should help).
>>>>>>>
>>>>>>
>>>>>> If what you said correct (I guess so), for me, we still need let them in
>>>>>> CONFIG_KRETPROBES area, and without any dummy outside, just like another
>>>>>> *kprobe* static inline functions have done in "include/linux/kprobes.h".
>>>>>
>>>>> kretprobe_assert() is only for the internal check. So we don't need to care
>>>>> about, and disable/enable_kretprobe() are anyway returns -EINVAL because
>>>>> kretprobe can not be registered. And all of them are inlined functions.
>>>>> In that case, we don't need to care about it.
>>>>
>>>> Hmm... it is related with code 'consistency':
>>>>
>>>>  - these static inline functions are kretprobe generic implementation,
>>>>    and we are trying to let all kretprobe generic implementation within
>>>>    CONFIG_KRETPROBES area.
>>>
>>> No, actually, kretprobe is just built on the kprobe. enable/disable_kretprobe
>>> just wrapped the kprobe methods. And kretprobe_assert() is just for kretprobe
>>> internal use. that is not an API. Moving only the kretprobe_assert() into the
>>> CONFIG_KRETPROBE area is not bad, but since it is just a static inline function,
>>> if there is no caller, it just be ignored, no side effect.
>>>
>>
>> OK, I can understand.
>>
>> And do you mean enable/disable_kretprobe() are API? if so, we have to
>> implement them whether CONFIG_KRETPROBES enabled or disabled.
>>
>> And when CONFIG_KRETPROBES=n, just like what you originally said: we
>> need returns -EINVAL directly (either, I am not quite sure whether the
>> input parameter will be NULL, in this case).
> 
> Both are API, and when implementing it I had also considered that, but
> I decided to stay it in inline-function wrapper. The reason why is,
> that enable/disable_k*probe require the registered k*probes. If the
> kernel hacker uses those functions, they must ensure registering his
> k*probes, otherwise it does not work correctly. If the CONFIG_KRETPROBES=n,
> register_kretprobe() always fails, this means that the code has
> no chance to call those functions (it must be).
>

OK, thank you for your explanations.


>>>>  - And original kprobe static inline functions have done like that,
>>>>    in same header file, if no obvious reasons, we can try to follow.
>>>
>>> It is no reasons to follow that too. Please keep your patch simple as much
>>> as possible.
>>>
>>
>> "keep our patch simple as much as posssible" sounds reasonable to me.
>> After skip "include/linux/kprobe.h", our patch's subject (include
>> comments) also need be changed (I will/should change it).
>>
>> For me, "include/linux/kprobe.h" can also be improved, but it can be
>> another patch for it (not only for kretprobe, but also for jpobe).
> 
> if that "improvement" means "simplify", it is acceptable. Now I don't like
> ifdefs of CONFIG_KPROBES and dummy functions, since if CONFIG_KPROBES=n,
> other kernel modules can also check the kconfig and decide what they do
> (or don't).
> Perhaps, what we've really needed is "just enough able to compile",
> not the fully covered dummy APIs.
> 

Hmm... for me, I still try to send a patch for "include/linux/kprobe.h".

For API (although it is kernel internal API), I have a hobby to try to
let it 'beautiful' as much as possible.


>>>>> I just concerned that it is a waste of memory if there are useless kretprobe
>>>>> related instances are built when CONFIG_KRETPROBES=n.
>>>>>
>>>>
>>>> Yeah, that is also one of reason (3rd reason).
>>>>
>>>>
>>>> And if necessary, please help check what we have done whether already
>>>> "let all kretprobe generic implementation within CONFIG_KRETPROBES area"
>>>> (exclude declaration, struct/union definition, and architecture
>>>> implementation).
>>>
>>> As I commented, your changes in kernel/kprobes.c are good to me except
>>> two functions. That's all what we need to fix :)
>>>
>>
>> I will send a patch for it (since subject changed, we need not mark
>> "patch v2"), thanks.  :-)
> 
> OK, I'll review that.
> 

Thanks.

> Thank you!
> 
> 

Thanks.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed