linux-kernel - Re: [PATCH 3/3] livepatch: force transition process to finish

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:   Thu, 25 May 2017 18:03:07 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Miroslav Benes <mbenes@...e.cz>
Cc:     jpoimboe@...hat.com, jeyu@...hat.com, jikos@...nel.org,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] livepatch: force transition process to finish

On Thu 2017-05-25 14:59:55, Miroslav Benes wrote:
> 
> > > > In fact, I would suggest to take klp_mutex in force_store()
> > > > and do all actions synchronously, including the check
> > > > of klp_transition_patch.
> > > 
> > > I still think it is better not do it. klp_unmark_tasks() does nothing else 
> > > than tasks already do. They call klp_update_patch_state() by themselves 
> > > and they do not grab klp_mutex lock for doing that. klp_unmark_tasks() 
> > > only forces this action.
> > 
> > You have a point. But I am not convinced ;-) klp_update_patch_state()
> > was called very carefully only when it was safe. The forcing
> > intentionally breaks the consistency model. User should really know
> > what they are doing when they use this feature.
> > 
> > I think that we should actually taint the kernel. Developers should
> > know when users were pulling their legs.
> 
> We could do that. I can change pr_warn() to WARN_ON_ONCE(), which would of 
> course taint the kernel.

Sounds good to me.


> > > On the other hand, I do not see a problem in doing that. We already have a 
> > > relationship between klp_mutex and tasklist_lock defined elsewhere, so it 
> > > is safe.
> > 
> > Yup.
> > 
> > > It would only serialize things needlessly.
> > 
> > I do not agree. The speed is not important here. Also look
> > into klp_reverse_transition(). We explicitly clear all
> > TIF_PATCH_PENDING flags and call synchronize_rcu() just
> > to make the situation easier and reduce space for potential
> > mistakes.
> 
> Yes, because we had to do that. We ran into problems otherwise. We do not 
> have to do it here. It does not help anything in my opinion.

AFAIK, we did not have to do it, see
https://lkml.kernel.org/r/20161222143452.GK25166@pathway.suse.cz
and the comment starting with "It would still leave a small".

Just for record, the idea of disabling the TIF flags came from Josh
in another mail. I have just repeated it.

I think that the problem already is complex enough and the
serialization would reduce the space of potential races.
But it is possible that I see it just too complex here.

Best Regards,
Petr