linux-kernel - Re: [PATCH] livepatch: Move error print out of lock protection in klp_enable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 2 Sep 2022 15:36:28 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     "Leizhen (ThunderTown)" <thunder.leizhen@...wei.com>
Cc:     Josh Poimboeuf <jpoimboe@...nel.org>,
        Jiri Kosina <jikos@...nel.org>,
        Miroslav Benes <mbenes@...e.cz>,
        Joe Lawrence <joe.lawrence@...hat.com>,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] livepatch: Move error print out of lock protection in
 klp_enable_patch()

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