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=-8.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 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 0B48CC2D0A3 for ; Tue, 3 Nov 2020 11:44:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A286A216C4 for ; Tue, 3 Nov 2020 11:44:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728598AbgKCLoP (ORCPT ); Tue, 3 Nov 2020 06:44:15 -0500 Received: from foss.arm.com ([217.140.110.172]:47464 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727988AbgKCLoO (ORCPT ); Tue, 3 Nov 2020 06:44:14 -0500 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 98CE1101E; Tue, 3 Nov 2020 03:44:13 -0800 (PST) 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 9C1B63F66E; Tue, 3 Nov 2020 03:44:11 -0800 (PST) Date: Tue, 3 Nov 2020 11:44:08 +0000 From: Dave Martin To: Peter Collingbourne Cc: Catalin Marinas , Evgenii Stepanov , Kostya Serebryany , Vincenzo Frascino , Will Deacon , Oleg Nesterov , "Eric W. Biederman" , "James E.J. Bottomley" , Andrey Konovalov , Helge Deller , Kevin Brodsky , Linux API , David Spickett , Linux ARM , Richard Henderson Subject: Re: [PATCH v12 7/8] signal: define the field siginfo.si_xflags Message-ID: <20201103114407.GK6882@arm.com> References: <7cc72abf960871135bc6e7fb11c8fc747401957b.1602892799.git.pcc@google.com> <20201102173751.GE6882@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Precedence: bulk List-ID: X-Mailing-List: linux-api@vger.kernel.org On Mon, Nov 02, 2020 at 08:10:57PM -0800, Peter Collingbourne wrote: > On Mon, Nov 2, 2020 at 9:38 AM Dave Martin wrote: > > > > On Fri, Oct 16, 2020 at 05:12:32PM -0700, Peter Collingbourne wrote: > > > This field will contain flags that may be used by signal handlers to > > > determine whether other fields in the _sigfault portion of siginfo are > > > valid. An example use case is the following patch, which introduces > > > the si_addr_tag_bits{,_mask} fields. > > > > > > A new sigcontext flag, SA_XFLAGS, is introduced in order to allow > > > a signal handler to require the kernel to set the field (but note > > > that the field will be set anyway if the kernel supports the flag, > > > regardless of its value). In combination with the previous patches, > > > this allows a userspace program to determine whether the kernel will > > > set the field. > > > > Apologies for this coming rather late: > > > > It occurs to me that we might want a more specific name, since this only > > applies to fault signals -- say, SA_FAULTFLAGS. > > > > If we end up wanting to add flags fields for other signal types, then we > > might end up needing a SA_ flag for each, which would be a bit annoying. > > > > So, alternatively. I wonder whether it's worth preemptively adding an > > extra flags to every kind of kernel-generated siginfo. If so, then > > having a single SA_XFLAGS would be fine. > > > > > > If added flags fields all over the place is considered overkill, then I > > guess it's sufficient to rename this flag. > > > > If renaming, the actual flags field in siginfo should also be renamed to > > match. > > I'd prefer not to add flags fields to every union member at this > point. I agree that faultflags is a better name, and I guess it's one > more reason not to try and reuse the ia64 field. Renamed in v13. Ack -- I thought I should make the point, but we've got enough spare sa_flags bits for now to make this one SIL_FAULT-specific, providing the SA_foo name looks equally specific -- so just renaming that should be OK. If we end up adding a flags field to another siginfo union member in the future, it's probably worth adding all the rest at the same time ... but it may never happen. [...] > > > diff --git a/kernel/ptrace.c b/kernel/ptrace.c > > > index 43d6179508d6..85b5b4e38661 100644 > > > --- a/kernel/ptrace.c > > > +++ b/kernel/ptrace.c > > > @@ -687,18 +687,32 @@ static int ptrace_getsiginfo(struct task_struct *child, kernel_siginfo_t *info) > > > return error; > > > } > > > > > > -static int ptrace_setsiginfo(struct task_struct *child, const kernel_siginfo_t *info) > > > +static int ptrace_setsiginfo(struct task_struct *child, unsigned long flags, > > > + kernel_siginfo_t *info) > > > { > > > - unsigned long flags; > > > + unsigned long lock_flags; > > > int error = -ESRCH; > > > > > > - if (lock_task_sighand(child, &flags)) { > > > + if (flags & ~PTRACE_SIGINFO_XFLAGS) { > > > + return -EINVAL; > > > + } > > > + > > > + /* > > > + * If the caller is unaware of si_xflags and we're using a layout that > > > + * requires it, set it to 0 which means "no fields are available". > > > + */ > > > + if (!(flags & PTRACE_SIGINFO_XFLAGS) && > > > + siginfo_layout_is_fault( > > > + siginfo_layout(info->si_signo, info->si_code))) > > > + info->si_xflags = 0; > > > + > > > + if (lock_task_sighand(child, &lock_flags)) { > > > error = -EINVAL; > > > if (likely(child->last_siginfo != NULL)) { > > > copy_siginfo(child->last_siginfo, info); > > > error = 0; > > > } > > > - unlock_task_sighand(child, &flags); > > > + unlock_task_sighand(child, &lock_flags); > > > } > > > return error; > > > } > > > @@ -1038,9 +1052,12 @@ int ptrace_request(struct task_struct *child, long request, > > > break; > > > > > > case PTRACE_SETSIGINFO: > > > + addr = 0; > > > > If this is intended to fall through, please add a > > > > /* fall through */ > > > > comment here (newer GCC has warnings to catch this; not sure about > > clang, but I'd be surprised if no version warns). > > Yes, clang has this warning, but it looks like it is currently > disabled in clang due to differences between the compilers [1] so I > didn't see it. > > It looks like the kernel is moving towards using the fallthrough > macro/attribute defined in include/linux/compiler_attributes.h (and to > me this personally seems better than relying on parsing comments), so > I've used that macro in v13. Ah, I wasn't aware of that. Sounds better! Cheers ---Dave 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=-8.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,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 91B63C2D0A3 for ; Tue, 3 Nov 2020 11:46:11 +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 0ED4E206C0 for ; Tue, 3 Nov 2020 11:46:11 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="hErBWXbY" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0ED4E206C0 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=VDs1XXCdtbxmyeLpmhnmEu55dRB+cputZdSsikilCl8=; b=hErBWXbYHyLaNrOVLZMmf1MNM w5Yk4tIc0cx/XiIftZcDY4ccHdHpCM5SzoDzILqiW1Mm4NiJ4HhOSrLnKVceZq34/JANLxObQJ9gk aXJqpZdUviBpvLH+76TwoecxDAn3iQGLWAyxI16N+EaYMXg67KBYg3kEzxrfl/icsZLpD2V8o6+YM xZQCGMiJW9kLZKWGG+tTQzwGDKCcuX5WX5ji52lk2Pe+OfNoypLlWZ9Y0uqHczl+l3V2ZufhQ5GjM VV/RElJ94I0lDkoYZyLLE0L+dGbS/0yLxpWgK69eieYzWmjzPlK8DlzM2+ZaC/pAmdfUzc9mVbSmv 5+pGF5TLA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kZujb-0003Vk-FU; Tue, 03 Nov 2020 11:44:19 +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 1kZujY-0003Un-Fu for linux-arm-kernel@lists.infradead.org; Tue, 03 Nov 2020 11:44:17 +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 98CE1101E; Tue, 3 Nov 2020 03:44:13 -0800 (PST) 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 9C1B63F66E; Tue, 3 Nov 2020 03:44:11 -0800 (PST) Date: Tue, 3 Nov 2020 11:44:08 +0000 From: Dave Martin To: Peter Collingbourne Subject: Re: [PATCH v12 7/8] signal: define the field siginfo.si_xflags Message-ID: <20201103114407.GK6882@arm.com> References: <7cc72abf960871135bc6e7fb11c8fc747401957b.1602892799.git.pcc@google.com> <20201102173751.GE6882@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-20201103_064416_690889_832203E7 X-CRM114-Status: GOOD ( 41.67 ) 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: Linux ARM , Catalin Marinas , Helge Deller , Kevin Brodsky , Oleg Nesterov , Linux API , "James E.J. Bottomley" , Kostya Serebryany , "Eric W. Biederman" , Andrey Konovalov , David Spickett , Vincenzo Frascino , Will Deacon , Evgenii Stepanov , 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, Nov 02, 2020 at 08:10:57PM -0800, Peter Collingbourne wrote: > On Mon, Nov 2, 2020 at 9:38 AM Dave Martin wrote: > > > > On Fri, Oct 16, 2020 at 05:12:32PM -0700, Peter Collingbourne wrote: > > > This field will contain flags that may be used by signal handlers to > > > determine whether other fields in the _sigfault portion of siginfo are > > > valid. An example use case is the following patch, which introduces > > > the si_addr_tag_bits{,_mask} fields. > > > > > > A new sigcontext flag, SA_XFLAGS, is introduced in order to allow > > > a signal handler to require the kernel to set the field (but note > > > that the field will be set anyway if the kernel supports the flag, > > > regardless of its value). In combination with the previous patches, > > > this allows a userspace program to determine whether the kernel will > > > set the field. > > > > Apologies for this coming rather late: > > > > It occurs to me that we might want a more specific name, since this only > > applies to fault signals -- say, SA_FAULTFLAGS. > > > > If we end up wanting to add flags fields for other signal types, then we > > might end up needing a SA_ flag for each, which would be a bit annoying. > > > > So, alternatively. I wonder whether it's worth preemptively adding an > > extra flags to every kind of kernel-generated siginfo. If so, then > > having a single SA_XFLAGS would be fine. > > > > > > If added flags fields all over the place is considered overkill, then I > > guess it's sufficient to rename this flag. > > > > If renaming, the actual flags field in siginfo should also be renamed to > > match. > > I'd prefer not to add flags fields to every union member at this > point. I agree that faultflags is a better name, and I guess it's one > more reason not to try and reuse the ia64 field. Renamed in v13. Ack -- I thought I should make the point, but we've got enough spare sa_flags bits for now to make this one SIL_FAULT-specific, providing the SA_foo name looks equally specific -- so just renaming that should be OK. If we end up adding a flags field to another siginfo union member in the future, it's probably worth adding all the rest at the same time ... but it may never happen. [...] > > > diff --git a/kernel/ptrace.c b/kernel/ptrace.c > > > index 43d6179508d6..85b5b4e38661 100644 > > > --- a/kernel/ptrace.c > > > +++ b/kernel/ptrace.c > > > @@ -687,18 +687,32 @@ static int ptrace_getsiginfo(struct task_struct *child, kernel_siginfo_t *info) > > > return error; > > > } > > > > > > -static int ptrace_setsiginfo(struct task_struct *child, const kernel_siginfo_t *info) > > > +static int ptrace_setsiginfo(struct task_struct *child, unsigned long flags, > > > + kernel_siginfo_t *info) > > > { > > > - unsigned long flags; > > > + unsigned long lock_flags; > > > int error = -ESRCH; > > > > > > - if (lock_task_sighand(child, &flags)) { > > > + if (flags & ~PTRACE_SIGINFO_XFLAGS) { > > > + return -EINVAL; > > > + } > > > + > > > + /* > > > + * If the caller is unaware of si_xflags and we're using a layout that > > > + * requires it, set it to 0 which means "no fields are available". > > > + */ > > > + if (!(flags & PTRACE_SIGINFO_XFLAGS) && > > > + siginfo_layout_is_fault( > > > + siginfo_layout(info->si_signo, info->si_code))) > > > + info->si_xflags = 0; > > > + > > > + if (lock_task_sighand(child, &lock_flags)) { > > > error = -EINVAL; > > > if (likely(child->last_siginfo != NULL)) { > > > copy_siginfo(child->last_siginfo, info); > > > error = 0; > > > } > > > - unlock_task_sighand(child, &flags); > > > + unlock_task_sighand(child, &lock_flags); > > > } > > > return error; > > > } > > > @@ -1038,9 +1052,12 @@ int ptrace_request(struct task_struct *child, long request, > > > break; > > > > > > case PTRACE_SETSIGINFO: > > > + addr = 0; > > > > If this is intended to fall through, please add a > > > > /* fall through */ > > > > comment here (newer GCC has warnings to catch this; not sure about > > clang, but I'd be surprised if no version warns). > > Yes, clang has this warning, but it looks like it is currently > disabled in clang due to differences between the compilers [1] so I > didn't see it. > > It looks like the kernel is moving towards using the fallthrough > macro/attribute defined in include/linux/compiler_attributes.h (and to > me this personally seems better than relying on parsing comments), so > I've used that macro in v13. Ah, I wasn't aware of that. Sounds better! Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel