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=-12.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 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 0531AC4363C for ; Wed, 7 Oct 2020 08:57:33 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 91A0B20797 for ; Wed, 7 Oct 2020 08:57:32 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="qqPY9wGD" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 91A0B20797 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=QUbgCrf9AYEeCu11Q4YC9I6QKqvDugx4LdKVG28JHVM=; b=qqPY9wGDx6VMh61YjswxHMFo8 a5N0HbNa+g+gOQorYXljnli2z/Yh1nVn4M3s6iEF/xoSgiLiWam0PFMq0H/zkGx5wdcD7qoCEx9Ho A6ROJ7DdtHmsmk/OCAeJrby6sRrqhm8I8cKZxwI9HdUahYBziySH1O+cIsr7q47HrEhNjuiaXukjr 67jZCrz7g64nCbQz6LvtVasvv6VdU/8CfH3n+V3C1hUoz8O9kQ7BdId5J1+RXuyndqqq/8bGG3krK LgPq2Qk8tUmBDf4wJoF2BzFx4kQoTSYcl6N4M8GXsO3e1woVOjci3luqpGuknbB+MdkAEooZyxl8i B6jz1CCYA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kQ5F9-0001Qm-FK; Wed, 07 Oct 2020 08:56:15 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kQ5F6-0001Ps-Cn for linux-arm-kernel@lists.infradead.org; Wed, 07 Oct 2020 08:56:13 +0000 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 CC75E113E; Wed, 7 Oct 2020 01:56:10 -0700 (PDT) Received: from arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E803D3F71F; Wed, 7 Oct 2020 01:56:08 -0700 (PDT) Date: Wed, 7 Oct 2020 09:56:06 +0100 From: Dave Martin To: Peter Collingbourne Subject: Re: [PATCH v10 5/7] signal: deduplicate code dealing with common _sigfault fields Message-ID: <20201007085605.GD6642@arm.com> References: <20200908151326.GV6642@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201007_045612_524750_6C146C12 X-CRM114-Status: GOOD ( 41.52 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Parisc List , Catalin Marinas , Kevin Brodsky , Oleg Nesterov , Evgenii Stepanov , "James E.J. Bottomley" , Kostya Serebryany , "Eric W. Biederman" , Andrey Konovalov , David Spickett , Vincenzo Frascino , Will Deacon , Linux ARM , Richard Henderson Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, Oct 05, 2020 at 10:07:01PM -0700, Peter Collingbourne wrote: > On Tue, Sep 8, 2020 at 8:13 AM Dave Martin wrote: > > > > On Fri, Aug 21, 2020 at 10:10:15PM -0700, Peter Collingbourne wrote: > > > We're about to add more common _sigfault fields, so deduplicate the > > > existing code for initializing _sigfault fields in {send,force}_sig_*, > > > and for copying _sigfault fields in copy_siginfo_to_external32 and > > > post_copy_siginfo_from_user32, to reduce the number of places that > > > will need to be updated by upcoming changes. > > > > > > Signed-off-by: Peter Collingbourne > > > --- > > > View this change in Gerrit: https://linux-review.googlesource.com/q/I4f56174e1b7b2bf4a3c8139e6879cbfd52750a24 > > > > > > include/linux/signal.h | 13 ++++++ > > > kernel/signal.c | 101 ++++++++++++++++------------------------- > > > 2 files changed, 53 insertions(+), 61 deletions(-) > > > > > > diff --git a/include/linux/signal.h b/include/linux/signal.h > > > index 6bb1a3f0258c..3edbf54493ee 100644 > > > --- a/include/linux/signal.h > > > +++ b/include/linux/signal.h > > > @@ -50,6 +50,19 @@ enum siginfo_layout { > > > > > > enum siginfo_layout siginfo_layout(unsigned sig, int si_code); > > > > > > +static inline bool siginfo_layout_is_fault(enum siginfo_layout layout) > > > +{ > > > + switch (layout) { > > > + case SIL_FAULT: > > > + case SIL_FAULT_MCEERR: > > > + case SIL_FAULT_BNDERR: > > > + case SIL_FAULT_PKUERR: > > > + return true; > > > + default: > > > + return false; > > > + } > > > +} > > > + > > > /* > > > * Define some primitives to manipulate sigset_t. > > > */ > > > diff --git a/kernel/signal.c b/kernel/signal.c > > > index c80e70bde11d..4ee9dc03f20f 100644 > > > --- a/kernel/signal.c > > > +++ b/kernel/signal.c > > > @@ -1649,6 +1649,15 @@ void force_sigsegv(int sig) > > > force_sig(SIGSEGV); > > > } > > > > > > +static void set_sigfault_common_fields(struct kernel_siginfo *info, int sig, > > > + int code, void __user *addr) > > > +{ > > > + info->si_signo = sig; > > > + info->si_errno = 0; > > > + info->si_code = code; > > > + info->si_addr = addr; > > > +} > > > + > > > int force_sig_fault_to_task(int sig, int code, void __user *addr > > > ___ARCH_SI_TRAPNO(int trapno) > > > ___ARCH_SI_IA64(int imm, unsigned int flags, unsigned long isr) > > > @@ -1657,10 +1666,7 @@ int force_sig_fault_to_task(int sig, int code, void __user *addr > > > struct kernel_siginfo info; > > > > > > clear_siginfo(&info); > > > - info.si_signo = sig; > > > - info.si_errno = 0; > > > - info.si_code = code; > > > - info.si_addr = addr; > > > + set_sigfault_common_fields(&info, sig, code, addr); > > > #ifdef __ARCH_SI_TRAPNO > > > info.si_trapno = trapno; > > > #endif > > > @@ -1689,10 +1695,7 @@ int send_sig_fault(int sig, int code, void __user *addr > > > struct kernel_siginfo info; > > > > > > clear_siginfo(&info); > > > - info.si_signo = sig; > > > - info.si_errno = 0; > > > - info.si_code = code; > > > - info.si_addr = addr; > > > + set_sigfault_common_fields(&info, sig, code, addr); > > > #ifdef __ARCH_SI_TRAPNO > > > info.si_trapno = trapno; > > > #endif > > > @@ -1710,10 +1713,7 @@ int force_sig_mceerr(int code, void __user *addr, short lsb) > > > > > > WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR)); > > > clear_siginfo(&info); > > > - info.si_signo = SIGBUS; > > > - info.si_errno = 0; > > > - info.si_code = code; > > > - info.si_addr = addr; > > > + set_sigfault_common_fields(&info, SIGBUS, code, addr); > > > info.si_addr_lsb = lsb; > > > return force_sig_info(&info); > > > } > > > @@ -1724,10 +1724,7 @@ int send_sig_mceerr(int code, void __user *addr, short lsb, struct task_struct * > > > > > > WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR)); > > > clear_siginfo(&info); > > > - info.si_signo = SIGBUS; > > > - info.si_errno = 0; > > > - info.si_code = code; > > > - info.si_addr = addr; > > > + set_sigfault_common_fields(&info, SIGBUS, code, addr); > > > info.si_addr_lsb = lsb; > > > return send_sig_info(info.si_signo, &info, t); > > > } > > > @@ -1738,10 +1735,7 @@ int force_sig_bnderr(void __user *addr, void __user *lower, void __user *upper) > > > struct kernel_siginfo info; > > > > > > clear_siginfo(&info); > > > - info.si_signo = SIGSEGV; > > > - info.si_errno = 0; > > > - info.si_code = SEGV_BNDERR; > > > - info.si_addr = addr; > > > + set_sigfault_common_fields(&info, SIGSEGV, SEGV_BNDERR, addr); > > > info.si_lower = lower; > > > info.si_upper = upper; > > > return force_sig_info(&info); > > > @@ -1753,10 +1747,7 @@ int force_sig_pkuerr(void __user *addr, u32 pkey) > > > struct kernel_siginfo info; > > > > > > clear_siginfo(&info); > > > - info.si_signo = SIGSEGV; > > > - info.si_errno = 0; > > > - info.si_code = SEGV_PKUERR; > > > - info.si_addr = addr; > > > + set_sigfault_common_fields(&info, SIGSEGV, SEGV_PKUERR, addr); > > > info.si_pkey = pkey; > > > return force_sig_info(&info); > > > } > > > @@ -1770,10 +1761,8 @@ int force_sig_ptrace_errno_trap(int errno, void __user *addr) > > > struct kernel_siginfo info; > > > > > > clear_siginfo(&info); > > > - info.si_signo = SIGTRAP; > > > + set_sigfault_common_fields(&info, SIGTRAP, TRAP_HWBKPT, addr); > > > info.si_errno = errno; > > > - info.si_code = TRAP_HWBKPT; > > > - info.si_addr = addr; > > > return force_sig_info(&info); > > > } > > > > > > @@ -3266,12 +3255,23 @@ int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from) > > > void copy_siginfo_to_external32(struct compat_siginfo *to, > > > const struct kernel_siginfo *from) > > > { > > > + enum siginfo_layout layout = > > > + siginfo_layout(from->si_signo, from->si_code); > > > + > > > memset(to, 0, sizeof(*to)); > > > > > > to->si_signo = from->si_signo; > > > to->si_errno = from->si_errno; > > > to->si_code = from->si_code; > > > - switch(siginfo_layout(from->si_signo, from->si_code)) { > > > + > > > + if (siginfo_layout_is_fault(layout)) { > > > + to->si_addr = ptr_to_compat(from->si_addr); > > > +#ifdef __ARCH_SI_TRAPNO > > > + to->si_trapno = from->si_trapno; > > > +#endif > > > + } > > > + > > > + switch (layout) { > > > > I find the code flow slightly awkward with this change, because the > > fault signal fields are populated partly in the if() above and partly > > in the switch(). Previously, only the universal stuff was done outside. > > > > Would this be easier on future maintainers if we pulled the common > > stuff out into a helper and then called it from the appropriate switch > > cases? The compiler will probably output some duplicated code in that > > case (depending on how clever it is at undoing the duplication), but > > the amount of affected code is small. > > I'm not sure about that. One advantage of the current code structure > is that we end up with siginfo_layout_is_fault containing our single > canonical list of layouts that use the sigfault union member. With > your proposed code structure, the only caller of > siginfo_layout_is_fault would be the code that the next patch adds to > kernel/ptrace.c, which needs to know which layouts use sigfault so > that it can clear si_xflags, and that could relatively easily get out > of sync by accident since it's dealing with a less common case. > > That being said, perhaps we could say that a caller of > ptrace(PTRACE_SETSIGINFO) is by definition old, so it wouldn't be > using that API to send signals with new siginfo layouts. That would > preclude a client from upgrading its knowledge of si_xflags > independently of its knowledge of new layouts though, and there would > be nothing preventing a new caller of siginfo_layout_is_fault being > added that would be exposed to new user code. OK, I guess I don't have a strong view on this for now. I suppose that clear_siginfo() could be moved into set_sigfault_common_fields() (perhaps warranting a rename to init_sigfault() or similar). But that's just one line per case, so probably not worth getting excited about. There is also some virtue in keeping the clear_siginfo() explicit everywhere, so that people are reminded about the need for it. Anyway, that's not essential. [...] Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel