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.
