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=-4.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 59D56C433E0 for ; Tue, 23 Jun 2020 17:54:25 +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 203CA20702 for ; Tue, 23 Jun 2020 17:54:25 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="PKmL4WG3" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 203CA20702 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=xmission.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:Subject:MIME-Version:Message-ID:In-Reply-To:Date: References:To:From:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=F0bmtw86l0WmfhLDmL9zJkUMahEsmRPioSQYFqM0d7Q=; b=PKmL4WG3Xi1GkiHCvv7hPhn7Q e92pBZTLb29aY8ry1ZO4DB478mSzQgzCmPfjCHySV5f/rMfjuY8v/b0zAbWAybRAEePWcTKGbioHm flwIzVK2EpF5XCeSAdj1Izl1r/2T09Kkyv4S791xwwVQUZp8N0QkxszARwasEisMbrcbaGnB8ZRbI qOAqulZalrJyxf+56thVfgc/OhmzKevnLGL7svGcax5sdbo1kVwA8xhlqa/XmhyI8p2856MEAhdfo qzWL3MIZv2+YPuglcjd0waLo22OusCUKQEJmQC88Sv32DvSQtZYrOgN+PvMNenc6Vvo7hQr93Sj/0 rneS0kkHA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jnn5x-0001BK-Pa; Tue, 23 Jun 2020 17:52:29 +0000 Received: from out02.mta.xmission.com ([166.70.13.232]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jnn5u-000157-LH for linux-arm-kernel@lists.infradead.org; Tue, 23 Jun 2020 17:52:27 +0000 Received: from in01.mta.xmission.com ([166.70.13.51]) by out02.mta.xmission.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jnn5X-0002La-Qb; Tue, 23 Jun 2020 11:52:04 -0600 Received: from ip68-227-160-95.om.om.cox.net ([68.227.160.95] helo=x220.xmission.com) by in01.mta.xmission.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.87) (envelope-from ) id 1jnn5W-00021G-Jy; Tue, 23 Jun 2020 11:52:03 -0600 From: ebiederm@xmission.com (Eric W. Biederman) To: Dave Martin References: <20200623020134.16655-1-pcc@google.com> <87sgemrlgc.fsf@x220.int.ebiederm.org> <20200623143846.GX25945@arm.com> Date: Tue, 23 Jun 2020 12:47:38 -0500 In-Reply-To: <20200623143846.GX25945@arm.com> (Dave Martin's message of "Tue, 23 Jun 2020 15:38:47 +0100") Message-ID: <87d05pr7wl.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 X-XM-SPF: eid=1jnn5W-00021G-Jy; ; ; mid=<87d05pr7wl.fsf@x220.int.ebiederm.org>; ; ; hst=in01.mta.xmission.com; ; ; ip=68.227.160.95; ; ; frm=ebiederm@xmission.com; ; ; spf=neutral X-XM-AID: U2FsdGVkX1+arOXNflCMq4Gd5IEJHaqYsZtAqcetBZg= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH v8] arm64: Expose FAR_EL1 tag bits in siginfo X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) 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: Will Deacon , Catalin Marinas , Kevin Brodsky , Oleg Nesterov , Kostya Serebryany , Evgenii Stepanov , Andrey Konovalov , Vincenzo Frascino , Peter Collingbourne , 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 Dave Martin writes: > On Tue, Jun 23, 2020 at 07:54:59AM -0500, Eric W. Biederman wrote: >> Peter Collingbourne writes: >> >> > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c >> > index 47f651df781c..a8380a2b6361 100644 >> > --- a/arch/arm64/kernel/traps.c >> > +++ b/arch/arm64/kernel/traps.c >> > @@ -235,20 +235,41 @@ static void arm64_show_signal(int signo, const char *str) >> > } >> > >> > void arm64_force_sig_fault(int signo, int code, void __user *addr, >> > + unsigned long far, unsigned char far_tb_mask, >> > const char *str) >> > { >> > arm64_show_signal(signo, str); >> > - if (signo == SIGKILL) >> > + if (signo == SIGKILL) { >> > force_sig(SIGKILL); >> > - else >> > - force_sig_fault(signo, code, addr); >> > + } else { >> > + struct kernel_siginfo info; >> > + clear_siginfo(&info); >> > + info.si_signo = signo; >> > + info.si_errno = 0; >> > + info.si_code = code; >> > + info.si_addr = addr; >> > + info.si_addr_top_byte = (far >> 56) & far_tb_mask; >> > + info.si_addr_top_byte_mask = far_tb_mask; >> > + force_sig_info(&info); >> > + } >> > } >> > >> > void arm64_force_sig_mceerr(int code, void __user *addr, short lsb, >> > - const char *str) >> > + unsigned long far, const char *str) >> > { >> > + struct kernel_siginfo info; >> > + >> > arm64_show_signal(SIGBUS, str); >> > - force_sig_mceerr(code, addr, lsb); >> > + >> > + clear_siginfo(&info); >> > + info.si_signo = SIGBUS; >> > + info.si_errno = 0; >> > + info.si_code = code; >> > + info.si_addr = addr; >> > + info.si_addr_lsb = lsb; >> > + info.si_addr_top_byte = far >> 56; >> > + info.si_addr_top_byte_mask = 0xff; >> > + force_sig_info(&info); >> > } >> >> I have a real problem with this construction. force_sig_info is not an >> interface that should be used for anything except to define a wrapper >> that takes it's parameters. > > Can you elaborate? How would you do this king of thing. There are no other uses of force_sig_info in architecture code. I just removed them _all_ because they were almost all broken. In fact your mcerr case is broken because it uses two different union members simultantiously. So I am looking for something like force_sig_mcerr or force_sig_fault that includes your new information that then calls force_sig_info. I know of no other way to safely use the siginfo struct. > AIUI we absolutely need a forced signal here, we need to supply > metadata, and we don't have to open-code all that at every relevant > signal generation site... > >> It is not clear to me that if you have adapted siginfo_layout. > > Garbled sentence? Looks like. One of the pieces of code that needs to change when siginfo gets updated is siginfo_layout so that the structure can be properly decoded and made sense of. I am not seeing anything like that. >> > diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h >> > index cb3d6c267181..6dd82373eb2d 100644 >> > --- a/include/uapi/asm-generic/siginfo.h >> > +++ b/include/uapi/asm-generic/siginfo.h >> > @@ -91,6 +91,14 @@ union __sifields { >> > char _dummy_pkey[__ADDR_BND_PKEY_PAD]; >> > __u32 _pkey; >> > } _addr_pkey; >> > +#ifdef __aarch64__ >> > + /* used with all si_codes */ >> > + struct { >> > + short _dummy_top_byte; > > ^ What's this for? I don't have Eric's insight here. > >> > + unsigned char _top_byte; >> > + unsigned char _top_byte_mask; >> > + } _addr_top_byte; >> > +#endif >> > }; >> > } _sigfault; >> > >> >> Why the _dummy_top_byte? Oh I see it should be spelled "short _addr_lsb;". >> >> Please remove the "#ifdef __aarch64__". If at all possible we want to >> design this so any other architecture who has this challenge can use the >> code. The kind of code does not get enough attention/maintenance if it >> is built for a single architecture. > > Does this belong in the user-facing siginfo? It seems a bit strange, > when other closely-related information such as esr_context is in the > arch-specific signal frame. > > > If trying to make this reusable, I wonder if we should have some sort of > "address attributes" field. > > An alternative approach would be to add some opaque "arch_data" field, > that the arch code can go look at when delivering the signal. My point is arch specific hacks don't get looked at, and wind up being broken. So I am not encouraging anything that doesn't get looked at, and winds up being broken. > I think that's all we were trying to achieve here: tack some arch > private data onto the signal, to avoid having to stash the same info in > thread_info and pray that it doesn't get clobbered in between signal > generation and delivery. What makes it arch private data? Why isn't it just data that your arch happens to have that other architectures don't yet. > At signal delivery time, the arch signal delivery code could inspect > this data and emit it into the signal frame as appropriate for the > arch. Sorry this probably isn't what you mean but when I read that description I get the feeling that you are asking for code that won't be reviewed or looked at by anyone else. So inevitably that code will be broken. Frankly it is bad enough finding people to review and maintain the generic code of the kernel. With that said, and your desire for this data to go into the sigframe (despite it sounding a lot like generic data that only aarch64 has implemented yet) can you remind me why siginfo comes into the equation at all? Last I remember the discussion there were some issues and the plan was to simply solve the problem generically and use siginfo, and there would not need to be any sigframe changes. But if you want to deliver via sigframe force_sig_info and all it's variants will be delivered when the kernel returns back to userspace. So there should be no need to touch siginfo or anything else in that scenario. Eric _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel