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=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham 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 15F9DC4321A for ; Tue, 11 Jun 2019 05:15:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id DB6DA20896 for ; Tue, 11 Jun 2019 05:15:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2391210AbfFKFPV (ORCPT ); Tue, 11 Jun 2019 01:15:21 -0400 Received: from foss.arm.com ([217.140.110.172]:53006 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2390492AbfFKFPU (ORCPT ); Tue, 11 Jun 2019 01:15:20 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 55CCB344; Mon, 10 Jun 2019 22:15:19 -0700 (PDT) Received: from [10.162.43.135] (p8cg001049571a15.blr.arm.com [10.162.43.135]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 5A7FF3F73C; Mon, 10 Jun 2019 22:15:11 -0700 (PDT) Subject: Re: [RFC V3] mm: Generalize and rename notify_page_fault() as kprobe_page_fault() To: Christophe Leroy , linux-kernel@vger.kernel.org, linux-mm@kvack.org Cc: linux-arm-kernel@lists.infradead.org, linux-ia64@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org, linux-sh@vger.kernel.org, sparclinux@vger.kernel.org, x86@kernel.org, Andrew Morton , Michal Hocko , Matthew Wilcox , Mark Rutland , Stephen Rothwell , Andrey Konovalov , Michael Ellerman , Paul Mackerras , Russell King , Catalin Marinas , Will Deacon , Tony Luck , Fenghua Yu , Martin Schwidefsky , Heiko Carstens , Yoshinori Sato , "David S. Miller" , Thomas Gleixner , Peter Zijlstra , Ingo Molnar , Andy Lutomirski , Dave Hansen References: <1559903655-5609-1-git-send-email-anshuman.khandual@arm.com> <97e9c9b3-89c8-d378-4730-841a900e6800@arm.com> From: Anshuman Khandual Message-ID: <1875ab7a-204e-4150-c7cc-d282f69da724@arm.com> Date: Tue, 11 Jun 2019 10:45:30 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/11/2019 10:16 AM, Christophe Leroy wrote: > > > Le 10/06/2019 à 04:39, Anshuman Khandual a écrit : >> >> >> On 06/07/2019 09:01 PM, Christophe Leroy wrote: >>> >>> >>> Le 07/06/2019 à 12:34, Anshuman Khandual a écrit : >>>> Very similar definitions for notify_page_fault() are being used by multiple >>>> architectures duplicating much of the same code. This attempts to unify all >>>> of them into a generic implementation, rename it as kprobe_page_fault() and >>>> then move it to a common header. >>>> >>>> kprobes_built_in() can detect CONFIG_KPROBES, hence new kprobe_page_fault() >>>> need not be wrapped again within CONFIG_KPROBES. Trap number argument can >>>> now contain upto an 'unsigned int' accommodating all possible platforms. >>>> >>>> kprobe_page_fault() goes the x86 way while dealing with preemption context. >>>> As explained in these following commits the invoking context in itself must >>>> be non-preemptible for kprobes processing context irrespective of whether >>>> kprobe_running() or perhaps smp_processor_id() is safe or not. It does not >>>> make much sense to continue when original context is preemptible. Instead >>>> just bail out earlier. >>>> >>>> commit a980c0ef9f6d >>>> ("x86/kprobes: Refactor kprobes_fault() like kprobe_exceptions_notify()") >>>> >>>> commit b506a9d08bae ("x86: code clarification patch to Kprobes arch code") >>>> >>>> Cc: linux-arm-kernel@lists.infradead.org >>>> Cc: linux-ia64@vger.kernel.org >>>> Cc: linuxppc-dev@lists.ozlabs.org >>>> Cc: linux-s390@vger.kernel.org >>>> Cc: linux-sh@vger.kernel.org >>>> Cc: sparclinux@vger.kernel.org >>>> Cc: x86@kernel.org >>>> Cc: Andrew Morton >>>> Cc: Michal Hocko >>>> Cc: Matthew Wilcox >>>> Cc: Mark Rutland >>>> Cc: Christophe Leroy >>>> Cc: Stephen Rothwell >>>> Cc: Andrey Konovalov >>>> Cc: Michael Ellerman >>>> Cc: Paul Mackerras >>>> Cc: Russell King >>>> Cc: Catalin Marinas >>>> Cc: Will Deacon >>>> Cc: Tony Luck >>>> Cc: Fenghua Yu >>>> Cc: Martin Schwidefsky >>>> Cc: Heiko Carstens >>>> Cc: Yoshinori Sato >>>> Cc: "David S. Miller" >>>> Cc: Thomas Gleixner >>>> Cc: Peter Zijlstra >>>> Cc: Ingo Molnar >>>> Cc: Andy Lutomirski >>>> Cc: Dave Hansen >>>> >>>> Signed-off-by: Anshuman Khandual >>>> --- >>>> Testing: >>>> >>>> - Build and boot tested on arm64 and x86 >>>> - Build tested on some other archs (arm, sparc64, alpha, powerpc etc) >>>> >>>> Changes in RFC V3: >>>> >>>> - Updated the commit message with an explaination for new preemption behaviour >>>> - Moved notify_page_fault() to kprobes.h with 'static nokprobe_inline' per Matthew >>>> - Changed notify_page_fault() return type from int to bool per Michael Ellerman >>>> - Renamed notify_page_fault() as kprobe_page_fault() per Peterz >>>> >>>> Changes in RFC V2: (https://patchwork.kernel.org/patch/10974221/) >>>> >>>> - Changed generic notify_page_fault() per Mathew Wilcox >>>> - Changed x86 to use new generic notify_page_fault() >>>> - s/must not/need not/ in commit message per Matthew Wilcox >>>> >>>> Changes in RFC V1: (https://patchwork.kernel.org/patch/10968273/) >>>> >>>>    arch/arm/mm/fault.c      | 24 +----------------------- >>>>    arch/arm64/mm/fault.c    | 24 +----------------------- >>>>    arch/ia64/mm/fault.c     | 24 +----------------------- >>>>    arch/powerpc/mm/fault.c  | 23 ++--------------------- >>>>    arch/s390/mm/fault.c     | 16 +--------------- >>>>    arch/sh/mm/fault.c       | 18 ++---------------- >>>>    arch/sparc/mm/fault_64.c | 16 +--------------- >>>>    arch/x86/mm/fault.c      | 21 ++------------------- >>>>    include/linux/kprobes.h  | 16 ++++++++++++++++ >>>>    9 files changed, 27 insertions(+), 155 deletions(-) >>>> >>> >>> [...] >>> >>>> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h >>>> index 443d980..064dd15 100644 >>>> --- a/include/linux/kprobes.h >>>> +++ b/include/linux/kprobes.h >>>> @@ -458,4 +458,20 @@ static inline bool is_kprobe_optinsn_slot(unsigned long addr) >>>>    } >>>>    #endif >>>>    +static nokprobe_inline bool kprobe_page_fault(struct pt_regs *regs, >>>> +                          unsigned int trap) >>>> +{ >>>> +    int ret = 0; >>> >>> ret is pointless. >>> >>>> + >>>> +    /* >>>> +     * To be potentially processing a kprobe fault and to be allowed >>>> +     * to call kprobe_running(), we have to be non-preemptible. >>>> +     */ >>>> +    if (kprobes_built_in() && !preemptible() && !user_mode(regs)) { >>>> +        if (kprobe_running() && kprobe_fault_handler(regs, trap)) >>> >>> don't need an 'if A if B', can do 'if A && B' >> >> Which will make it a very lengthy condition check. > > Yes. But is that a problem at all ? Probably not. > > For me the following would be easier to read. > > if (kprobes_built_in() && !preemptible() && !user_mode(regs) && >     kprobe_running() && kprobe_fault_handler(regs, trap)) >     ret = 1; As mentioned before will stick with current x86 implementation.