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 90806C47DDF for ; Thu, 1 Feb 2024 09:11:48 +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-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:Cc: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=W4Ss596NuKh2a7sPCfLDUDYefrJ/2JPTmWbIWoAndLw=; b=hL3hwetMDxiylqAEZ3LZwvVEH9 /7CPF5yrSKxJP6ozRwVdyFQZbAL9l+I2lv0LO396xpTC6sMdB7/171eheJMZGtWDbc5uZmAU/h219 dGLnM9qLeE6weL43QhPz/TmjtSWH9l5zpAY10bvAGGZiRowtem4I/hSSquH/orztOoDrgb3LaJIwV ZIH7uxHzy8tyA/NKvmnQ6Z9ZT2D1yT/LDcn9vdq0sQ8qONCx12ZH6jpFgtQCbSmgbH/Snba59lhOp GfScLrC+1Br8U7W2TPbHmUOF4kVyBGDm8rJfSna3/T37CQ7hlhcFPat/Q2T5tDqEcVTD5HrKGVXLq L9ZF3Jew==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rVT6d-00000007D02-1bio; Thu, 01 Feb 2024 09:11:35 +0000 Received: from mail-pl1-x635.google.com ([2607:f8b0:4864:20::635]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rVT6a-00000007Cyd-3lIj for linux-arm-kernel@lists.infradead.org; Thu, 01 Feb 2024 09:11:34 +0000 Received: by mail-pl1-x635.google.com with SMTP id d9443c01a7336-1d5ce88b51cso157535ad.0 for ; Thu, 01 Feb 2024 01:11:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1706778685; x=1707383485; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=zpmw7XD3NRYPmylcKUZIs1amCALm5ceYEkOK13mCobY=; b=CiQKrNFd7dqq2wtWFkc3DtO7mT8sYN+l8NT7ivWBB9tn9/iCj0+ApiLfI6IU7bgMYS SAYHoIprTSh3+qG2XjiOm8cBOfchLVw/Vt7hkI05wVz3RG2TBwx9OY+n6vnUUnJ3nY6+ 7nj7RzXrCPM29VQfuY0hYDC7w2IxgF/105Z7EpFVouOJNpHEUa5wlXqninrAye7QXQQE 0StVD75JaskdrxNJ3voFECiDH6zCQa+mzbaROqUK6gEQlpuiT8VTjPigwTAZ3cD868he YfXRrRdiKMpJsIzGuUrMn7M8Gzkx+gM5DYR33947mw6cVO73+9t0eJRk2e3meRx/IMbP PtWw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706778685; x=1707383485; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=zpmw7XD3NRYPmylcKUZIs1amCALm5ceYEkOK13mCobY=; b=Gyxv0sroEbIehQ/pDIPQREVEq9X8AfjXjKk7O0OTYtFBXLnxlAJywlTilLDRm0ZLg3 +EAEiYeeHeZljCQNo6ZSETHMrSqqAt5M4259NUTNkMRxwFJyUjLhbhWfVfdMfr58yxLZ Sx3oCLi4AnP9s1ih0IrTdkb7zHjMYO0QyBtkXw1kVoumhE03+RXutUehgrZLVK+XVQIz vJ3uoMe+iyL772/7m8j2wdR8L2h19Ua+ENQ9Now/uZkQ0pW2LKXDHCUbPtVYtqAYhKiw GJ4HD71BPx2B7S6CgRZ2FVNSB5/Uw2+MWp1vQ7HFhp+lJsZn0TR/DG13uc9qSHIapjd+ cUtQ== X-Gm-Message-State: AOJu0YwnNmE9o4fS7k5V3gyxJ3X6CFtG0OWW4baurwVRhS8JAKUJhcv7 ALdnmP7oRQ5GuObV/2p+IGGE8WeFW8KVOh1zxGxM4jrkeApQ4kEArP4jgO5naQ== X-Google-Smtp-Source: AGHT+IG06fzWWaeYjUr/vX4M7gLOO+61jy4fE2PhBPJ64FlHm7t/jb+OVF/nW0ijhlWnHwthdyMMFA== X-Received: by 2002:a17:902:ced1:b0:1d6:fb94:10d5 with SMTP id d17-20020a170902ced100b001d6fb9410d5mr139335plg.27.1706778685070; Thu, 01 Feb 2024 01:11:25 -0800 (PST) X-Forwarded-Encrypted: i=0; AJvYcCUrBRGPWguQv+hECDS45pjyaIr8WcoLrgzXUAgcAytmPpZrNB491a1QBS8h6aAZtuUn0nByGer8flJ9jGXMckawcVjlKBMKWkP/CQpCfPU9xE5+qGj1oxLnTwf3K9wBlb5C6+D0K4nlMvIduKOvkPEjfmy/qA0REshrupZR5iibOO+gT0i33Gy/1yGXoCHXhv3UXQFLpzG7gsZP0Y+8ikQ+P/9gGmfcQXi/Xjd7Ui8FQ6YEpJH1sSHbuUj3x1lssAJEKVcRw4pLQS07A0zf805ludaBv+JjRJxZvpY= Received: from google.com ([2620:15c:2d3:205:ce95:ce9d:3dd2:5053]) by smtp.gmail.com with ESMTPSA id l20-20020a170902e2d400b001d8d04a4fa8sm7699490plc.300.2024.02.01.01.11.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 01 Feb 2024 01:11:24 -0800 (PST) Date: Thu, 1 Feb 2024 01:11:20 -0800 From: Fangrui Song To: Ard Biesheuvel Cc: Dave Martin , Peter Smith , Catalin Marinas , Will Deacon , linux-arm-kernel@lists.infradead.org, Jisheng Zhang , llvm@lists.linux.dev, linux-kernel@vger.kernel.org Subject: Re: [PATCH] arm64: jump_label: use constraint "S" instead of "i" Message-ID: <20240201091120.pbgr7ng6t2c36fut@google.com> References: <20240131065322.1126831-1-maskray@google.com> <20240201045551.ajg4iqcajyowl2rh@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240201_011132_969595_A2030002 X-CRM114-Status: GOOD ( 44.44 ) 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 2024-02-01, Ard Biesheuvel wrote: >On Thu, 1 Feb 2024 at 05:55, Fangrui Song wrote: >> >> On 2024-01-31, Dave Martin wrote: >> >On Wed, Jan 31, 2024 at 08:16:04AM +0100, Ard Biesheuvel wrote: >> >> Hello Fangrui, >> >> >> >> On Wed, 31 Jan 2024 at 07:53, Fangrui Song wrote: >> >> > >> >> > The constraint "i" seems to be copied from x86 (and with a redundant >> >> > modifier "c"). It works with -fno-PIE but not with -fPIE/-fPIC in GCC's >> >> > aarch64 port. >> > >> >(I'm not sure of the exact history, but the "c" may be inherited from >> >arm, where an output modifier was needed to suppress the "#" that >> >prefixes immediates in the traditional asm syntax. This does not >> >actually seem to be required for AArch64: rather while a # is allowed >> >and still considered good style in handwritten asm code, the syntax >> >doesn't require it, and the compiler doesn't emit it for "i" arguments, >> >AFAICT.) >> >> The aarch64 one could be inherited from >> arch/arm/include/asm/jump_label.h (2012), which could in turn be >> inherited from x86 (2010). >> Both the constraint "i" and the modifier "c" are generic.. >> For -fno-pic this combination can be used for every arch. >> >> >> > The constraint "S", which denotes a symbol reference (e.g. function, >> >> > global variable) or label reference, is more appropriate, and has been >> >> > available in GCC since 2012 and in Clang since 7.0. >> >> > >> >> > Signed-off-by: Fangrui Song >> >> > Link: https://maskray.me/blog/2024-01-30-raw-symbol-names-in-inline-assembly >> >> > --- >> >> > 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 48ddc0f45d22..31862b3bb33d 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 * const key, >> >> > " .pushsection __jump_table, \"aw\" \n\t" >> >> > " .align 3 \n\t" >> >> > " .long 1b - ., %l[l_yes] - . \n\t" >> >> > - " .quad %c0 - . \n\t" >> >> > + " .quad %0 - . \n\t" >> >> > " .popsection \n\t" >> >> > - : : "i"(&((char *)key)[branch]) : : l_yes); >> >> > + : : "S"(&((char *)key)[branch]) : : l_yes); >> >> >> >> 'key' is not used as a raw symbol name. We should make this >> >> >> >> " .quad %0 + %1 - ." >> >> >> >> and >> >> >> >> :: "S"(key), "i"(branch) :: l_yes); >> >> >> >> if we want to really clean this up. >> > >> >This hides more logic in the asm so it's arguably more cryptic >> >(although the code is fairly cryptic to begin with -- I don't really >> >see why the argument wasn't written as the equivalent >> >(char *)key + branch...) >> >> I agree that using "S" and "i" would introduce complexity. >> Using just "S" as this patch does should be clear. >> >> All of "i" "s" "S" support a symbol or label reference and a constant offset (can be zero), >> (in object file, a symbol and an addend; in GCC's term, the sum of a SYMBOL_REF and a CONST_INT). >> > >Taken the address of a struct, cast it to char[] and then index it >using a boolean is rather disgusting, no? I agree with you. Hmm. Clang's constraint "S" implementation doesn't support a constant offset, so `static_key_false(&nf_hooks_needed[pf][hook])` in include/linux/netfilter.h:nf_hook would not compile with Clang <= 18. I have a patch https://github.com/llvm/llvm-project/pull/80255 , but even if it is accepted and cherry-picked into the 18.x release branch, if we still support older Clang, we cannot use "S" unconditionally. So we probably need the following to prepare for -fPIE support in the future: diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h index 48ddc0f45d22..b8af2f8b0c99 100644 --- a/arch/arm64/include/asm/jump_label.h +++ b/arch/arm64/include/asm/jump_label.h @@ -15,6 +15,16 @@ #define JUMP_LABEL_NOP_SIZE AARCH64_INSN_SIZE +/* + * Prefer "S" to support PIC. However, use "i" for Clang 18 and earlier as "S" + * on a symbol with a constant offset is not supported. + */ +#if defined(CONFIG_CC_IS_CLANG) && __clang_major__ <= 18 +#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "i" +#else +#define JUMP_LABEL_STATIC_KEY_CONSTRAINT "S" +#endif + static __always_inline bool arch_static_branch(struct static_key * const key, const bool branch) { @@ -23,9 +33,9 @@ static __always_inline bool arch_static_branch(struct static_key * const key, " .pushsection __jump_table, \"aw\" \n\t" " .align 3 \n\t" " .long 1b - ., %l[l_yes] - . \n\t" - " .quad %c0 - . \n\t" + " .quad %0 + %1 - . \n\t" " .popsection \n\t" - : : "i"(&((char *)key)[branch]) : : l_yes); + : : JUMP_LABEL_STATIC_KEY_CONSTRAINT(key), "i"(branch) : : l_yes); return false; l_yes: @@ -40,9 +50,9 @@ static __always_inline bool arch_static_branch_jump(struct static_key * const ke " .pushsection __jump_table, \"aw\" \n\t" " .align 3 \n\t" " .long 1b - ., %l[l_yes] - . \n\t" - " .quad %c0 - . \n\t" + " .quad %0 + %1 - . \n\t" " .popsection \n\t" - : : "i"(&((char *)key)[branch]) : : l_yes); + : : JUMP_LABEL_STATIC_KEY_CONSTRAINT(key), "i"(branch) : : l_yes); return false; l_yes: >> >Anyway, I don't think the "i" versys "S" distinction makes any >> >difference without -fpic or equivalent, so it is not really relevant >> >for the kernel (except that "S" breaks compatibility with older >> >compilers...) >> > >> > >> >I think the main advantage of "S" is that it stops you accidentally >> >emitting undesirable relocations from asm code that is not written for >> >the -fpic case. >> > >> >But just changing "i" to "S" is not sufficient to port asms to -fpic: >> >the asms still need to be reviewed. >> > >> > >> >So unless the asm has been reviewed for position-independence, it may >> >anyway be better to stick with "i" so that the compiler actually chokes >> >if someone tries to build the code with -fpic. >> > >The virtual address of the kernel is randomized by KASLR, which relies >on PIE linking, and this puts constraints on the permitted types of >relocations. > >IOW, we basically already build the kernel as PIC code, but without >relying on -fPIC, because that triggers some behaviors that only make >sense for shared objects in user space. > >> >Since we are not trying to run arbitraily many running kernels in a >> >common address space (and not likely to do that), I'm not sure that we >> >would ever build the kernel with -fpic except for a few special-case >> >bits like the EFI stub and vDSO... unless I've missed something? >> > > >Yes, KASLR. The number of kernels is not the point, the point is that >the virtual load address of the kernel is usually decided at boot, and >so the code needs to be generated to accommodate that. > >> >If there's another reason why "S" is advantageous though, I'm happy to >> >be corrected. >> >> I remember that Ard has an RFC >> https://lore.kernel.org/linux-arm-kernel/20220427171241.2426592-1-ardb@kernel.org/ >> "[RFC PATCH 0/2] arm64: use PIE code generation for KASLR kernel" >> and see some recent PIE codegen patches. >> >> > Building the KASLR kernel without -fpie but linking it with -pie works >> > in practice, but it is not something that is explicitly supported by the >> > toolchains - it happens to work because the default 'small' code model >> > used by both GCC and Clang relies mostly on ADRP+ADD/LDR to generate >> > symbol references. >> >> I agree that current -fno-PIE with -shared -Bsymbolic linking is a hack >> that works as a conincidence, not guaranteed by the toolchain. >> This jump_label improvement (with no object file difference) fixes an >> obstacle. > >If we can get the guaranteed behavior of #pragma GCC visibility >push(hidden) from a command line option, we should build the core >kernel with -fpie instead. (Modules are partially linked objects, so >they can be built non-PIC as before) > I believe we don't have such a GCC option, but the effect can be simulated by -include hidden.h ... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel