On Fri 2022-09-02 09:28:59, Leizhen (ThunderTown) wrote:
> >> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> >> index 42f7e716d56bf72..cb7abc821a50584 100644
> >> --- a/kernel/livepatch/core.c
> >> +++ b/kernel/livepatch/core.c
> >> @@ -1041,9 +1041,9 @@ int klp_enable_patch(struct klp_patch *patch)
> >> mutex_lock(&klp_mutex);
> >>
> >> if (!klp_is_patch_compatible(patch)) {
> >> + mutex_unlock(&klp_mutex);
> >> pr_err("Livepatch patch (%s) is not compatible with the already installed livepatches.\n",
> >> patch->mod->name);
> >> - mutex_unlock(&klp_mutex);
> >
> > I do not see how this change could reliably reduce the code size.
> >
> > As Joe wrote, it looks like a random effect that is specific to a
> > particular compiler version, compilation options, and architecture.
> >
> > I am against these kind of random microptimizations. It is just a call
> > for problems. If you move printk() outside of a lock, you need to make
> > sure that the information is not racy.
>
> OK.
>
> mutex_lock(&klp_mutex);
> if (!klp_is_patch_compatible(patch)) {
> mutex_unlock(&klp_mutex);
> <--------- Do you mean the incompatible patches maybe disabled at this point?
This particular change is safe in the current design.
klp_enable_patch() is called from the module_init() callback
where patch->mod->name is defined. So it can't change.
The problem is that it is not obvious that it is safe. One has to
think about it. Also it might become dangerous when someone
tries to call klp_enable_livepatch() for another livepatch module.
> pr_err("Livepatch patch (%s) ...\n", patch->mod->name);
> return -EINVAL;
> }
>
> >
> > It might be safe in this particular case. But it is a bad practice.
> > It adds an extra work. It is error-prone with questionable gain.
> >
> > I am sorry but I NACK this patch. There must be better ways to
>
> OK
Thanks for understanding.
Best Regards,
Petr
