Re: [PATCH v4 1/2] Introduce klp_ops into klp_func structure

[zhang warden]

Hi, Miroslav & Petr

> On Sep 6, 2024, at 17:44, zhang warden <zhangwarden@xxxxxxxxx> wrote:
> 
>> 
>>>> 
>>>>> +       struct list_head func_stack;
>>>>> +       struct ftrace_ops fops;
>>>>> +};
>>>>> +
>>>>> 
>>>>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>>>>> index 52426665eecc..e4572bf34316 100644
>>>>> --- a/kernel/livepatch/core.c
>>>>> +++ b/kernel/livepatch/core.c
>>>>> @@ -760,6 +760,8 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>>>>> if (!func->old_name)
>>>>> return -EINVAL;
>>>>> 
>>>>> + func->ops = NULL;
>>>>> +
>>>> 
>>>> Any reason why it is not added a couple of lines later alongside the rest 
>>>> of the initialization?
>>> 
>>> Do you mean I should add couple of lines after 'return -EINVAL' ?
>> 
>> No, I am asking if there is a reason why you added 'func->ops = NULL;' 
>> here and not right after the rest of func initializations
>> 
>>       INIT_LIST_HEAD(&func->stack_node);
>>       func->patched = false;
>>       func->transition = false;
>> 
> 

I think I found a bug in my patch.

I move struct klp_ops to klp_func. But every time, klp_func
will init klp_func->ops to NULL. Which will make the test branch
 `if(! ops)` always true in function klp_patch_func.

An alternative solution should be something like
 (as Peter suggested before) 

https://lore.kernel.org/live-patching/20240805064656.40017-1-zhangyongde.zyd@xxxxxxxxxxxxxxx/T/#t


 static struct klp_ops *klp_find_ops(void *old_func)
 {
       struct klp_patch *patch;
       struct klp_object *obj;
       struct klp_func *func;
       struct klp_ops *ops;
 
        klp_for_each_patch(patch)
                klp_for_each_object(patch, obj)
                        klp_for_each_func(obj, func)
                               if(func->old_func == old_func)
                                        return func->ops;
        return NULL;
 }

and  klp_ops should be initialize in klp_init_func:

        func->patched = false;
        func->transition = false;
+       func->ops = klp_find_ops(func->old_func);

Here, func->ops should not init as NULL, it should be initialize with 
the existed ops (if klp_find_ops returns NULL, this patch is the first
time to be patched).

Regards.
Wardenjohn.