Received: by 2002:ac0:a5a6:0:0:0:0:0 with SMTP id m35-v6csp62992imm; Wed, 29 Aug 2018 14:01:38 -0700 (PDT) X-Google-Smtp-Source: ANB0VdbAcHtwwIX9/g1ryMnPv54ANEajciadUQtygqd+lg+ranLw8I8GgFi+SbuI03TxwGYmTv5D X-Received: by 2002:a62:b40c:: with SMTP id h12-v6mr7492993pfn.18.1535576498585; Wed, 29 Aug 2018 14:01:38 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1535576498; cv=none; d=google.com; s=arc-20160816; b=teYgQTUoxEYjqgMcNZg6yJItAOFtE0Cii6XJPi4OjRRGWEBb6BvHk89EaOQvKKDtdG FgQ3mu7GEIr4UFV4tE+QOFt71pw59ghAkez0trptAyBgtfCfQ2jHe7RcoFtkbu5q7hdY 50GZnV0FqT4ARMoePERdQgHk6yx5ItLIaYlWpNjhNtbMrbeYaLuLNHCp2sFk4rjeBIZM r+ooq6diM9fxdUPdgwI2LAi+6rlME58neZQ8uZ5bCX6Poey4AqiEiaUd1bWaIMuDZl52 alf1TpBnEXDpjdhlY4UbBP3+1nFo08m1ImleHtWNoYH+4EXIHKlk8LE24K3sdY/GuXuN Um0g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-transfer-encoding:content-disposition:mime-version :references:message-id:subject:cc:to:from:date :arc-authentication-results; bh=Ed315EqKGspL1QArksnt7rgH8wINi3ZIAkxmq/Fm/xM=; b=XEspgX8Z5+zjMWu/F0ovleYmBQH566jjaehKtTBfoBW0nHsfTij3u3EN/ybKnrBaay Zntf+BzebbDZBaVA15pM4dQkvZZfvhL2/YTVi6U+Jn+oV9+Q5mCeC1qmAzD1t4Sr8AkB wkGXkh4dP5+2lUEZdRC5dMlDGrS2daTT09COnDwR39HnDX37oI4gpdfchoKJbC/2B6ZG fG30PVxGEJMqtI+U7ZOBg7mZpbmtdNWvfNcoOoiMUaUxxQ56FHVn4QOE2qZvzXGleIPC v9M9fAuac4ZoD4sdYcg5QYSvd1O2YjCdlH8Afm2lf1MN+XaySlrPZF2EWTHVLH7oTnHV QtIw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id l3-v6si4588770pld.501.2018.08.29.14.01.23; Wed, 29 Aug 2018 14:01:38 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728830AbeH3A6w (ORCPT + 99 others); Wed, 29 Aug 2018 20:58:52 -0400 Received: from mga14.intel.com ([192.55.52.115]:62100 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727595AbeH3A6v (ORCPT ); Wed, 29 Aug 2018 20:58:51 -0400 X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Aug 2018 14:00:12 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,304,1531810800"; d="scan'208";a="81314336" Received: from sjchrist-coffee.jf.intel.com (HELO linux.intel.com) ([10.54.74.20]) by fmsmga002.fm.intel.com with ESMTP; 29 Aug 2018 14:00:06 -0700 Date: Wed, 29 Aug 2018 14:00:06 -0700 From: Sean Christopherson To: Nadav Amit Cc: Masami Hiramatsu , Thomas Gleixner , LKML , Ingo Molnar , X86 ML , Arnd Bergmann , linux-arch , Andy Lutomirski , Kees Cook Subject: Re: [RFC PATCH 1/6] x86/alternative: assert text_mutex is taken Message-ID: <20180829210006.GA7166@linux.intel.com> References: <20180829081147.184610-1-namit@vmware.com> <20180829081147.184610-2-namit@vmware.com> <20180829175936.fb27b3bf13da819a9a971f07@kernel.org> <1F547CEE-B5D9-42A0-8093-2C5555BACE26@vmware.com> <2694AE6F-2212-46C6-A570-6BAF265364FB@vmware.com> <20180829201309.GA7142@linux.intel.com> <58345C1F-8FF3-4F49-AF2F-B4789DF50CC7@vmware.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <58345C1F-8FF3-4F49-AF2F-B4789DF50CC7@vmware.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Aug 29, 2018 at 08:44:47PM +0000, Nadav Amit wrote: > at 1:13 PM, Sean Christopherson wrote: > > > On Wed, Aug 29, 2018 at 07:36:22PM +0000, Nadav Amit wrote: > >> at 10:11 AM, Nadav Amit wrote: > >> > >>> at 1:59 AM, Masami Hiramatsu wrote: > >>> > >>>> On Wed, 29 Aug 2018 01:11:42 -0700 > >>>> Nadav Amit wrote: > >>>> > >>>>> Use lockdep to ensure that text_mutex is taken when text_poke() is > >>>>> called. > >>>>> > >>>>> Actually it is not always taken, specifically when it is called by kgdb, > >>>>> so take the lock in these cases. > >>>> > >>>> Can we really take a mutex in kgdb context? > >>>> > >>>> kgdb_arch_remove_breakpoint > >>>> <- dbg_deactivate_sw_breakpoints > >>>> <- kgdb_reenter_check > >>>> <- kgdb_handle_exception > >>>> <- __kgdb_notify > >>>> <- kgdb_ll_trap > >>>> <- do_int3 > >>>> <- kgdb_notify > >>>> <- die notifier > >>>> > >>>> kgdb_arch_set_breakpoint > >>>> <- dbg_activate_sw_breakpoints > >>>> <- kgdb_reenter_check > >>>> <- kgdb_handle_exception > >>>> ... > >>>> > >>>> Both seems called in exception context, so we can not take a mutex lock. > >>>> I think kgdb needs a special path. > >>> > >>> You are correct, but I don’t want a special path. Presumably text_mutex is > >>> guaranteed not to be taken according to the code. > >>> > >>> So I guess the only concern is lockdep. Do you see any problem if I change > >>> mutex_lock() into mutex_trylock()? It should always succeed, and I can add a > >>> warning and a failure path if it fails for some reason. > >> > >> Err.. This will not work. I think I will drop this patch, since I cannot > >> find a proper yet simple assertion. Creating special path just for the > >> assertion seems wrong. > > > > It's probably worth expanding the comment for text_poke() to call out > > the kgdb case and reference kgdb_arch_{set,remove}_breakpoint(), whose > > code and comments make it explicitly clear why its safe for them to > > call text_poke() without acquiring the lock. Might prevent someone > > from going down this path again in the future. > > I thought that the whole point of the patch was to avoid comments, and > instead enforce the right behavior. I don’t understand well enough kgdb > code, so I cannot attest it does the right thing. What happens if > kgdb_do_roundup==0? As is, the comment is wrong because there are obviously cases where text_poke() is called without text_mutex being held. I can't attest to the kgdb code either. My thought was to document the exception so that if someone does want to try and enforce the right behavior they can dive right into the problem instead of having to learn of the kgdb gotcha the hard way. Maybe a FIXME is the right approach?