From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S942612AbdEYQDN (ORCPT ); Thu, 25 May 2017 12:03:13 -0400 Received: from mx2.suse.de ([195.135.220.15]:41655 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1760052AbdEYQDJ (ORCPT ); Thu, 25 May 2017 12:03:09 -0400 Date: Thu, 25 May 2017 18:03:07 +0200 From: Petr Mladek To: Miroslav Benes Cc: jpoimboe@redhat.com, jeyu@redhat.com, jikos@kernel.org, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/3] livepatch: force transition process to finish Message-ID: <20170525160307.GI26699@pathway.suse.cz> References: <20170518120043.7205-1-mbenes@suse.cz> <20170518120043.7205-4-mbenes@suse.cz> <20170524130610.GK7297@pathway.suse.cz> <20170524150950.GC26699@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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