From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=0.6 required=3.0 tests=DKIM_ADSP_ALL,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 817A7C3A5A1 for ; Thu, 22 Aug 2019 15:43:15 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 4A507233FD for ; Thu, 22 Aug 2019 15:43:15 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=amazon.de header.i=@amazon.de header.b="cV3qeK6x" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4A507233FD Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=amazon.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1i0pEq-0008RV-Cr; Thu, 22 Aug 2019 15:43:00 +0000 Received: from all-amaz-eas1.inumbo.com ([34.197.232.57] helo=us1-amaz-eas2.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1i0pEp-0008RL-FE for xen-devel@lists.xenproject.org; Thu, 22 Aug 2019 15:42:59 +0000 X-Inumbo-ID: 833cb9e3-c4f3-11e9-adda-12813bfff9fa Received: from smtp-fw-9102.amazon.com (unknown [207.171.184.29]) by us1-amaz-eas2.inumbo.com (Halon) with ESMTPS id 833cb9e3-c4f3-11e9-adda-12813bfff9fa; Thu, 22 Aug 2019 15:42:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amazon.de; i=@amazon.de; q=dns/txt; s=amazon201209; t=1566488577; x=1598024577; h=from:to:cc:subject:date:message-id:references: in-reply-to:mime-version; bh=Ai/ATEVNpL4IatvswiOIvK6oBSkpMR1WxyRuejALFpY=; b=cV3qeK6xOlAOB0gYqCBD4A2gbuS2GoGZySlK7/wz6vKqABvd+MN9WoVv 0RVJcN504X2QWQ8DGhmqYHkYOPMBTNsxDaAhN9MjUcdjmw/fczYU6hS/s ihta+B6s8AR7H2qikVJ0ZUMhkaCYXQsgF2eOWzLUVhcUWZrO19bK41iBT k=; X-IronPort-AV: E=Sophos;i="5.64,417,1559520000"; d="scan'208,217";a="696508207" Received: from sea3-co-svc-lb6-vlan3.sea.amazon.com (HELO email-inbound-relay-1d-5dd976cd.us-east-1.amazon.com) ([10.47.22.38]) by smtp-border-fw-out-9102.sea19.amazon.com with ESMTP; 22 Aug 2019 15:42:54 +0000 Received: from EX13MTAUEA001.ant.amazon.com (iad55-ws-svc-p15-lb9-vlan3.iad.amazon.com [10.40.159.166]) by email-inbound-relay-1d-5dd976cd.us-east-1.amazon.com (Postfix) with ESMTPS id 29AA6A2AFF; Thu, 22 Aug 2019 15:42:53 +0000 (UTC) Received: from EX13D05EUB001.ant.amazon.com (10.43.166.87) by EX13MTAUEA001.ant.amazon.com (10.43.61.82) with Microsoft SMTP Server (TLS) id 15.0.1367.3; Thu, 22 Aug 2019 15:42:52 +0000 Received: from EX13D05EUB004.ant.amazon.com (10.43.166.115) by EX13D05EUB001.ant.amazon.com (10.43.166.87) with Microsoft SMTP Server (TLS) id 15.0.1367.3; Thu, 22 Aug 2019 15:42:51 +0000 Received: from EX13D05EUB004.ant.amazon.com ([10.43.166.115]) by EX13D05EUB004.ant.amazon.com ([10.43.166.115]) with mapi id 15.00.1367.000; Thu, 22 Aug 2019 15:42:51 +0000 From: "Wieczorkiewicz, Pawel" To: Julien Grall Thread-Topic: [PATCH 09/14] livepatch: Add per-function applied/reverted state tracking marker Thread-Index: AQHVV/lI+OhiRe5NgkKqw74UV0kjMKcG+QKAgAAJKoCAAEr8AIAAA2cA Date: Thu, 22 Aug 2019 15:42:51 +0000 Message-ID: References: <20190821081931.90887-1-wipawel@amazon.de> <20190821081931.90887-10-wipawel@amazon.de> <9a2a6d52-f247-ff18-0cce-593162e33755@arm.com> In-Reply-To: <9a2a6d52-f247-ff18-0cce-593162e33755@arm.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.43.165.55] MIME-Version: 1.0 Precedence: Bulk Subject: Re: [Xen-devel] [PATCH 09/14] livepatch: Add per-function applied/reverted state tracking marker X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Tim Deegan , Stefano Stabellini , Wei Liu , Konrad Rzeszutek Wilk , George Dunlap , Andrew Cooper , Ross Lagerwall , Ian Jackson , xen-devel , "Pohlack, Martin" , "Wieczorkiewicz, Pawel" , Jan Beulich , xen-devel , =?iso-8859-1?Q?Roger_Pau_Monn=E9?= Content-Type: multipart/mixed; boundary="===============3717982362155098943==" Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" --===============3717982362155098943== Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_FA06107F58B84D1F9D1A15908E049F57amazoncom_" --_000_FA06107F58B84D1F9D1A15908E049F57amazoncom_ Content-Type: text/plain; charset="iso-8859-1" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable On 22. Aug 2019, at 17:30, Julien Grall > wrote: Hi Pawel, On 22/08/2019 12:02, Wieczorkiewicz, Pawel wrote: On 22. Aug 2019, at 12:29, Julien Grall > wrote: On 21/08/2019 09:19, Pawel Wieczorkiewicz wrote: More generally, I am not very comfortable to see panic() in the middle of t= he code. Could you explain why panic is the best solution over reverting th= e work? Yes. Production-ready hotpatches must not contain inconsistent hooks or fun= ctions-to-be-applied/reverted. The goal here is to detect such hotpatches and fail hard immediately highli= ghting the fact that such hotpatch is broken. Aside the len =3D 0 that you are going to fix. How would this condition hap= pen? Are you going to add code that will potentially trigger the panic? The default livepatch code path is very conservative (to the extent it does= not even need this check and panic). But, with the changes of this series, one can potentially construct a hotpa= tch that upon load would trigger the panic (or without the panic, would silently leave the Xen code in memory in an in= consistent state). This obviously must not happen in production, so the new livepatch feature should be used with care= .The changes make livepatch more flexible and universal, yet when using new features, more care is needed. However, when someone is developing a hotpatch or is using the new feature = for debugging or for whatever non-production-related reason, it is very helpful to detect immediately any= sort of undefined state. I have been there myself when I was working on the changes presented here, = and thought that would be a good idea to add this checks permanently. Also, when something changes in the future in the livepatch implementation,= those checks could potentially highlight any inconsistencies. The inconsistent application of a hotpatch (some function applied, some rev= erted while other left behined) leaves the system in a very bad state. I think the best what we could do here is p= anic() to enable post-mortem analysis of what went wrong and avoid leaking such system into production. Thank you for the explanation here (and on IRC). May I ask some documentati= on regarding the panic in at least commit message? Ideally, this would expl= ain why the panic the most sensible solution. Yes, certainly. I will add that. Cheers, -- Julien Grall Best Regards, Pawel Wieczorkiewicz Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879 --_000_FA06107F58B84D1F9D1A15908E049F57amazoncom_ Content-Type: text/html; charset="iso-8859-1" Content-ID: MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable
On 22. Aug 2019, at 17:30, Julien Grall <julien.grall@arm.com> wrote:
Hi Pawel,

On 22/08/2019 12:02, Wieczorkiewicz, Pawel wrote:
On 22. Aug 2019, at 12:29, Julien Gral= l <julien.grall@arm.c= om <mailto:julien= .grall@arm.com>> wrote:
On 21/08/2019 09:19, Pawel Wieczorkiewicz wrote:
More generally, I am not very comfortable to see panic() in the middle of t= he code. Could you explain why panic is the best solution over reverting th= e work?

Yes. Production-ready hotpatches must not contain inconsistent hooks or fun= ctions-to-be-applied/reverted.
The goal here is to detect such hotpatches and fail hard immediately highli= ghting the fact that such hotpatch
is broken.

Aside the len =3D 0 that you are going to fix. How would this condition hap= pen? Are you going to add code that will potentially trigger the panic?


The default livepatch code path is very conservative (to the extent it= does not even need this check and panic).
But, with the changes of this series, one can potentially construct a = hotpatch that upon load would trigger the panic
(or without the panic, would silently leave the Xen code in memory in = an inconsistent state). This obviously must not
happen in production, so the new livepatch feature should be used with= care.The changes make livepatch more
flexible and universal, yet when using new features, more care is need= ed.

However, when someone is developing a hotpatch or is using the new fea= ture for debugging or for whatever
non-production-related reason, it is very helpful to detect immediatel= y any sort of undefined state.
I have been there myself when I was working on the changes presented h= ere, and thought that would be a good idea
to add this checks permanently.
Also, when something changes in the future in the livepatch implementa= tion, those checks could potentially highlight
any inconsistencies. 

The inconsistent application of a hotp= atch (some function applied, some reverted while other left behined) leaves=
the system in a very bad state. I think the best what we could do here is p= anic() to enable post-mortem analysis
of what went wrong and avoid leaking such system into production.

Thank you for the explanation here (and on IRC). May I ask some documentati= on regarding the panic in at least commit message? Ideally, this would expl= ain why the panic the most sensible solution.


Yes, certainly. I will add that.

Cheers,

--
Julien Grall

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


--_000_FA06107F58B84D1F9D1A15908E049F57amazoncom_-- --===============3717982362155098943== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVucHJvamVjdC5vcmcKaHR0cHM6Ly9saXN0 cy54ZW5wcm9qZWN0Lm9yZy9tYWlsbWFuL2xpc3RpbmZvL3hlbi1kZXZlbA== --===============3717982362155098943==--