From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751837AbdH3P3l (ORCPT ); Wed, 30 Aug 2017 11:29:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35350 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751748AbdH3P3k (ORCPT ); Wed, 30 Aug 2017 11:29:40 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C2A867EA9A Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=jpoimboe@redhat.com Date: Wed, 30 Aug 2017 10:29:38 -0500 From: Josh Poimboeuf To: Miroslav Benes Cc: Pavel Machek , jeyu@kernel.org, jikos@kernel.org, pmladek@suse.com, lpechacek@suse.cz, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 3/3] livepatch: force transition process to finish Message-ID: <20170830152938.vx5dbuef5g7onoq5@treble> References: <20170810104815.14727-1-mbenes@suse.cz> <20170810104815.14727-4-mbenes@suse.cz> <20170830072436.GA14796@amd> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Wed, 30 Aug 2017 15:29:39 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Aug 30, 2017 at 02:48:24PM +0200, Miroslav Benes wrote: > On Wed, 30 Aug 2017, Pavel Machek wrote: > > > On Thu 2017-08-10 12:48:15, Miroslav Benes wrote: > > > If a task sleeps in a set of patched functions uninterruptibly, it could > > > block the whole transition process indefinitely. Thus it may be useful > > > to clear its TIF_PATCH_PENDING to allow the process to finish. > > > > > > Admin can do that now by writing to force sysfs attribute in livepatch > > > sysfs directory. TIF_PATCH_PENDING is then cleared for all tasks and the > > > transition can finish successfully. > > > > > > Important note! Use wisely. Admin must be sure that it is safe to > > > execute such action. This means that it must be checked that by doing so > > > the consistency model guarantees are not violated. > > > > Yes, that's what admins are good for. Magically determining what state > > their machine is in, and deciding if all the processes are in the sane > > state and what the consequences are. They have all the tools they need > > to do that, like JTAG connection to the CPU and about 10 years of > > time... do they? > > It is not that bad in this case fortunately. The decision depends on > > 1. which processes are not yet migrated and why (which functions they're > sleeping on) > > AND > > 2. what functions are patched and what is the nature of a fix. > > Admin knows 1. and he needs input from a livepatch author about 2. There > is no magic there, but cooperation between those two is definitely > required. As we said before, we have to trust the admin knows what they're doing. If the vendor didn't tell them to force the patch, they shouldn't be forcing it. Just like they shouldn't be doing "sudo rm -rf /". And the last time I checked, there's no taint for logging in as root. > > This should taint the kernel at the very least. > > That's what we discussed in v1. In cases where the patch does not need the > consistency model at all, the taint would be too much. But I have no > problem to transform pr_warn to WARN_ON_ONCE. As I mentioned before, false warnings can have unintended negative consequences as well. But anyway, if you do decide to make it a warning, I think the ONCE wouldn't be helpful. It should just be WARN_ON. But I still vote against it. > > It should also require capabilities beyond "normal root", because it > > allows malicious admin to do "bad things (tm)" to the kernel. What exactly do you consider to be beyond "normal root"??? > I think that malicious admin can do pretty bad (and more serious) things > even without this. That is not an excuse, but it is general problem not > specific to this patch. Right. A malicious admin with root access can do a *lot* worse than force loading a patch. A hacker who just obtained root access isn't going to say "Finally! I can force load that stalled CVE fix!" -- Josh