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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5822AC433EF for ; Thu, 28 Apr 2022 09:38:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=I8szsg2ocw8agmx0U7a+ZwvOoPbCqJiZp1vPxK9X8GY=; b=x6KvPAwmJzZ/mA zasJZ8a8vPuXi4GRouJ9Og6a4ySplBzycmpogzHSfjqqnAB5/eDDN42Z1CbADse3vEV3VE3VSEHvJ g7mzNJ03QHIxMHUuTQP1PD6FuM60wFw7CaZtSNvaZfwDyWc1UgdgQpVPdg+EjPbnAM8Qoiy/3cpAG vDJ1LhEfWioHjOTOR3uA3gv7Dj/erp+EHE6TxopZ1r0M6sKd8Lwf2Dsz/pA/zeK/NoIru3LUikXjU N3KtL7o+RPZUrTMGJV4iSJY8xTpR33v++Rq9T5XjH1LMt0SJuJUUhWZI9wjvz8asjOJrYnMMPRUyL NDg7O7AYkovLsVCWEwTA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nk0ab-005lsp-RN; Thu, 28 Apr 2022 09:37:34 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nk0ZB-005lIH-Km for linux-arm-kernel@lists.infradead.org; Thu, 28 Apr 2022 09:36:09 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id E7B0661999 for ; Thu, 28 Apr 2022 09:36:04 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2ACC7C385AC for ; Thu, 28 Apr 2022 09:36:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1651138564; bh=TC5OcEQItwF+flx2o+zOio7lGQFb1AmBamQHYEb6C0E=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=hIzQdLCulyApTfYNzAVBv5Y6bt87OFopYh85b7eRWRpfWZiHPqzYN7TLtvu7OJ9DQ Ktq7jKIO1hOBqziBmItuuRbBF0kpGOZ1sZK2HAYjVBp9SbbCf+EJUsUbTAPj1ieDcM ZZM49kMUMCjnft/aiYrb27V4BCimPhpPXXeJSci+dZjcIzqkCzwLza4BgoKspNXpEX 9MtFUve8ncdtGTmQXYFMl4xiTceDDT7cWDQmoAjSBINeO1PVAEAJlbRHMj7hrcCQrD OGIulruHnXfwSDlhQrcW8P0bpqyrsTbpak06/pj9i1Os/zolN8dF3gGNqixHanGjxD 6ZIJG/rwiys2w== Received: by mail-ot1-f50.google.com with SMTP id w23-20020a056830111700b00603c6d1ce73so2775695otq.9 for ; Thu, 28 Apr 2022 02:36:04 -0700 (PDT) X-Gm-Message-State: AOAM533NLxIlnzSOgYId2AD+BhKjfC9BPH3Stq3GegQxQU7J6KHa62CH RCNS+1mIHHf6cxuYCarDeh5khWwtGXoS7clci+k= X-Google-Smtp-Source: ABdhPJzgIatg3WbOH4wNy3TpHDmAKSmIsRBjgVKhaa0HVAX9RwCox9jOgv0onNTcpMYa/dWOOXGn6SGGVNg845VEOM4= X-Received: by 2002:a05:6830:33eb:b0:5f8:d36d:3831 with SMTP id i11-20020a05683033eb00b005f8d36d3831mr11451226otu.265.1651138563294; Thu, 28 Apr 2022 02:36:03 -0700 (PDT) MIME-Version: 1.0 References: <20220427171241.2426592-1-ardb@kernel.org> <20220427171241.2426592-2-ardb@kernel.org> In-Reply-To: From: Ard Biesheuvel Date: Thu, 28 Apr 2022 11:35:51 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC PATCH 1/2] arm64: jump_label: use more precise asm constraints To: Nick Desaulniers Cc: Linux ARM , clang-built-linux , Will Deacon , Catalin Marinas , Kees Cook , Mark Rutland , Nathan Chancellor , Sami Tolvanen X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220428_023605_838057_9F0B327E X-CRM114-Status: GOOD ( 43.38 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 Wed, 27 Apr 2022 at 23:50, Ard Biesheuvel wrote: > > Hi Nick, > > On Wed, 27 Apr 2022 at 20:59, Nick Desaulniers wrote: > > > > On Wed, Apr 27, 2022 at 10:13 AM Ard Biesheuvel wrote: > > > > > > In order to set bit #0 of the struct static_key pointer in the the jump > > > label struct, we currently cast the pointer to char[], and take the > > > address of either the 0th or 1st array member, depending on the value of > > > 'branch'. This works but creates problems with -fpie code generation: > > > GCC complains about the constraint being unsatisfiable, and Clang > > > miscompiles the code in a way that causes stability issues (immediate > > > panic on 'attempt to kill init') > > > > Any more info on the "miscompile?" Perhaps worth an upstream bug report? > > > > I made very little progress trying to narrow it down: a segfault in > user space caused by who knows what. I'd file a bug if I knew how to > reproduce it. > ... and now, I cannot even reproduce it anymore, so I'll drop this mention altogether. > > > > > > So instead, pass the struct static_key reference and the 'branch' > > > immediate individually, in a way that satisfies both GCC and Clang (GCC > > > wants the 'S' constraint, whereas Clang wants the 'i' constraint for > > > argument %0) > > > > Anything we can do to improve Clang's handling of S constraints? > > https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html#Machine-Constraints > > >> An absolute symbolic address or a label reference > > > > "S" seems more appropriate than "i" for a symbol reference, and GCC > rejects "i" when using -fpie. But the actual literal being emitted is > a relative reference, not an absolute one. This likely needs more > discussion, but using "i" in the way we do now is definitely dodgy. > I have tried to create a reproducer for this issue, but the code below, which does essentially the same thing as jump_label.h, works fine with Clang and GCC with or without -fpie. extern struct s { struct { } m; } ss; static inline __attribute__((always_inline)) int inner(struct s *s) { asm goto("adrp xzr, %c0" : : "S"(&s->m) : : l); return 0; l:return 1; } int outer(void) { return inner(&ss); } So what's tricky here is that arch_static_branch() [like inner() above] does not refer to the global directly, which is either struct static_key_true or struct static_key_false, but to its 'key' member, which is the first member of either former type. However, Clang does not seem to care in case of this example, but when building the kernel, it errors out with /home/ard/linux/arch/arm64/include/asm/jump_label.h:22:3: error: invalid operand for inline asm constraint 'S' Another thing I hate about this code is that it needs optimizations enabled, or it won't compile. > > > > > > Signed-off-by: Ard Biesheuvel > > > --- > > > arch/arm64/include/asm/jump_label.h | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h > > > index cea441b6aa5d..f741bbacf175 100644 > > > --- a/arch/arm64/include/asm/jump_label.h > > > +++ b/arch/arm64/include/asm/jump_label.h > > > @@ -23,9 +23,9 @@ static __always_inline bool arch_static_branch(struct static_key *key, > > > " .pushsection __jump_table, \"aw\" \n\t" > > > " .align 3 \n\t" > > > " .long 1b - ., %l[l_yes] - . \n\t" > > > - " .quad %c0 - . \n\t" > > > + " .quad %c0 - . + %1 \n\t" > > > > If %1 is "i" constrained, then I think we can use the %c output > > template for it as well? > > https://gcc.gnu.org/onlinedocs/gccint/Output-Template.html > > > > Is %c what prevents the leading # from being emitted? I'm not sure > whether that matters here, as AArch64 asm does not require it (and the > code builds fine with either compiler). But if it reflects the use > more precisely, I agree we should add it. > > > Is the expression clearer if we keep the `- .` at the end of each expression? > > > > I don't think so. The add sets the enabled bit, so I'd even be > inclined to write this as (%c0 - .) + %c1 to emphasize that this is a > relative reference with bit #0 set separately. > > > > " .popsection \n\t" > > > - : : "i"(&((char *)key)[branch]) : : l_yes); > > > + : : "Si"(key), "i"(branch) : : l_yes); > > > > > > return false; > > > l_yes: > > > @@ -40,9 +40,9 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, > > > " .pushsection __jump_table, \"aw\" \n\t" > > > " .align 3 \n\t" > > > " .long 1b - ., %l[l_yes] - . \n\t" > > > - " .quad %c0 - . \n\t" > > > + " .quad %c0 - . + %1 \n\t" > > > " .popsection \n\t" > > > - : : "i"(&((char *)key)[branch]) : : l_yes); > > > + : : "Si"(key), "i"(branch) : : l_yes); > > > > > > return false; > > > l_yes: > > > -- > > > 2.30.2 > > > > > > > > > -- > > Thanks, > > ~Nick Desaulniers _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel