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=-10.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=unavailable 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 11EE4C433E1 for ; Sun, 26 Jul 2020 16:28:53 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id AE9C42065F for ; Sun, 26 Jul 2020 16:28:52 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="HMnGZGZ8" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org AE9C42065F Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id C86706B0005; Sun, 26 Jul 2020 12:28:51 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C10A76B0007; Sun, 26 Jul 2020 12:28:51 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id AD6BA6B000A; Sun, 26 Jul 2020 12:28:51 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0246.hostedemail.com [216.40.44.246]) by kanga.kvack.org (Postfix) with ESMTP id 927176B0005 for ; Sun, 26 Jul 2020 12:28:51 -0400 (EDT) Received: from smtpin11.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 3D87F181AEF0B for ; Sun, 26 Jul 2020 16:28:51 +0000 (UTC) X-FDA: 77080760862.11.chair56_160735f26f59 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin11.hostedemail.com (Postfix) with ESMTP id 5F7B6184C0532 for ; Sun, 26 Jul 2020 16:06:34 +0000 (UTC) X-HE-Tag: chair56_160735f26f59 X-Filterd-Recvd-Size: 7277 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by imf06.hostedemail.com (Postfix) with ESMTP for ; Sun, 26 Jul 2020 16:06:33 +0000 (UTC) Received: from mail-ot1-f52.google.com (mail-ot1-f52.google.com [209.85.210.52]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id B1CF020791 for ; Sun, 26 Jul 2020 16:06:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1595779592; bh=jOlSd937RAl4IikqYHXqep9iyrxRMosx470QDWX0F5o=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=HMnGZGZ8zFPNbKdaNkL5nkkSM8jRMIAHhiqCpnnztcLsnfOizEu4MyxHX3/PY48kQ OKSriRyt3KRRiQEY7MQd/UvJ5SEe1uW/4kpCx6/OlGtmxNSrIN7a+JuuZitRc8QarT 57nOy9ojB0zNJZSlav9+3RAnApxBHr7GhgYpBqNU= Received: by mail-ot1-f52.google.com with SMTP id a26so1774336otf.1 for ; Sun, 26 Jul 2020 09:06:32 -0700 (PDT) X-Gm-Message-State: AOAM530CZ0gW2qOSggfTuqvBjzJSyDPR5aLeK+CS1i+KD0SS+A2VuwYg ABJWf3bXLv+aHPXLmvfEDyOZ9m3CoJPNM70/WgU= X-Google-Smtp-Source: ABdhPJyLC8hHAzofnQu2h1FVAUiDveA5YPbaDCm/F/ft2ehbXBb05JTdDqOZFfW7sdjm1unxLObM5+FL1usYgX7CE+k= X-Received: by 2002:a05:6830:1094:: with SMTP id y20mr5572348oto.90.1595779591988; Sun, 26 Jul 2020 09:06:31 -0700 (PDT) MIME-Version: 1.0 References: <20200724050553.1724168-1-jarkko.sakkinen@linux.intel.com> <20200724050553.1724168-6-jarkko.sakkinen@linux.intel.com> <20200724092746.GD517988@gmail.com> <20200725031648.GG17052@linux.intel.com> <20200726081408.GB2927915@kernel.org> In-Reply-To: <20200726081408.GB2927915@kernel.org> From: Ard Biesheuvel Date: Sun, 26 Jul 2020 19:06:20 +0300 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v5 5/6] kprobes: Use text_alloc() and text_free() To: Mike Rapoport Cc: Jarkko Sakkinen , Ingo Molnar , Linux Kernel Mailing List , linux-mm@kvack.org, Andi Kleen , Masami Hiramatsu , Peter Zijlstra , "Naveen N. Rao" , Anil S Keshavamurthy , "David S. Miller" , Jessica Yu Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 5F7B6184C0532 X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam05 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Sun, 26 Jul 2020 at 11:14, Mike Rapoport wrote: > > On Sat, Jul 25, 2020 at 06:16:48AM +0300, Jarkko Sakkinen wrote: > > On Fri, Jul 24, 2020 at 11:27:46AM +0200, Ingo Molnar wrote: > > > > > > * Jarkko Sakkinen wrote: > > > > > > > Use text_alloc() and text_free() instead of module_alloc() and > > > > module_memfree() when an arch provides them. > > > > > > > > Cc: linux-mm@kvack.org > > > > Cc: Andi Kleen > > > > Cc: Masami Hiramatsu > > > > Cc: Peter Zijlstra > > > > Signed-off-by: Jarkko Sakkinen > > > > --- > > > > kernel/kprobes.c | 9 +++++++++ > > > > 1 file changed, 9 insertions(+) > > > > > > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > > > > index 4e46d96d4e16..611fcda9f6bf 100644 > > > > --- a/kernel/kprobes.c > > > > +++ b/kernel/kprobes.c > > > > @@ -40,6 +40,7 @@ > > > > #include > > > > #include > > > > #include > > > > +#include > > > > > > > > #define KPROBE_HASH_BITS 6 > > > > #define KPROBE_TABLE_SIZE (1 << KPROBE_HASH_BITS) > > > > @@ -111,12 +112,20 @@ enum kprobe_slot_state { > > > > > > > > void __weak *alloc_insn_page(void) > > > > { > > > > +#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC > > > > + return text_alloc(PAGE_SIZE); > > > > +#else > > > > return module_alloc(PAGE_SIZE); > > > > +#endif > > > > } > > > > > > > > void __weak free_insn_page(void *page) > > > > { > > > > +#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC > > > > + text_free(page); > > > > +#else > > > > module_memfree(page); > > > > +#endif > > > > } > > > > > > I've read the observations in the other threads, but this #ifdef > > > jungle is silly, it's a de-facto open coded text_alloc() with a > > > module_alloc() fallback... > > > > In the previous version I had: > > > > https://lore.kernel.org/lkml/20200717030422.679972-4-jarkko.sakkinen@linux.intel.com/ > > > > and I had just calls to text_alloc() and text_free() in corresponding > > snippet to the above. > > > > I got this feedback from Mike: > > > > https://lore.kernel.org/lkml/20200718162359.GA2919062@kernel.org/ > > > > I'm not still sure that I fully understand this feedback as I don't see > > any inherent and obvious difference to the v4. In that version fallbacks > > are to module_alloc() and module_memfree() and text_alloc() and > > text_memfree() can be overridden by arch. > > Let me try to elaborate. > > There are several subsystems that need to allocate memory for executable > text. As it happens, they use module_alloc() with some abilities for > architectures to override this behaviour. > > For many architectures, it would be enough to rename modules_alloc() to > text_alloc(), make it built-in and this way allow removing dependency on > MODULES. > > Yet, some architectures have different restrictions for code allocation > for different subsystems so it would make sense to have more than one > variant of text_alloc() and a single config option ARCH_HAS_TEXT_ALLOC > won't be sufficient. > > I liked Mark's suggestion to have text_alloc_() and proposed > a way to introduce text_alloc_kprobes() along with > HAVE_KPROBES_TEXT_ALLOC to enable arch overrides of this function. > > The major difference between your v4 and my suggestion is that I'm not > trying to impose a single ARCH_HAS_TEXT_ALLOC as an alternative to > MODULES but rather to use per subsystem config option, e.g. > HAVE_KPROBES_TEXT_ALLOC. > > Another thing, which might be worth doing regardless of the outcome of > this discussion is to rename alloc_insn_pages() to text_alloc_kprobes() > because the former is way too generic and does not emphasize that the > instruction page is actually used by kprobes only. > Masami or Peter should correct me if I am wrong, but it seems to me that the way kprobes uses these pages does not require them to be in relative branching range of the core kernel on any architecture, given that they are populated with individual instruction opcodes that are executed in single step mode, and relative branches are emulated (when needed) So for kprobes in particular, we should be able to come up with a generic sequence that does not involve module_alloc(), and therefore removes the kprobes dependency on module support entirely (with the exception of power which maps the vmalloc space nx when module support is disabled). Renaming alloc_insn_page() to something more descriptive makes sense imo, but is a separate issue.