From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anshuman Khandual Date: Mon, 10 Jun 2019 02:51:11 +0000 Subject: Re: [RFC V3] mm: Generalize and rename notify_page_fault() as kprobe_page_fault() Message-Id: <97e9c9b3-89c8-d378-4730-841a900e6800@arm.com> List-Id: References: <1559903655-5609-1-git-send-email-anshuman.khandual@arm.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: Christophe Leroy , linux-kernel@vger.kernel.org, linux-mm@kvack.org Cc: Mark Rutland , Michal Hocko , linux-ia64@vger.kernel.org, linux-sh@vger.kernel.org, Peter Zijlstra , Catalin Marinas , Dave Hansen , Heiko Carstens , Paul Mackerras , Matthew Wilcox , sparclinux@vger.kernel.org, linux-s390@vger.kernel.org, Yoshinori Sato , Michael Ellerman , x86@kernel.org, Russell King , Will Deacon , Ingo Molnar , Fenghua Yu , Stephen Rothwell , Andrey Konovalov , Andy Lutomirski , Thomas Gleixner , linux-arm-kernel@lists.infradead.org, Tony Luck , Martin Schwidefsky , Andrew Morton , linuxppc-dev@lists.ozlabs.org, "David S. Miller" 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. > >> +            ret = 1; > > can do 'return true;' directly here > >> +    } >> +    return ret; > > And 'return false' here. Makes sense, will drop ret.