From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x631.google.com (mail-pl1-x631.google.com [IPv6:2607:f8b0:4864:20::631]) by sourceware.org (Postfix) with ESMTPS id 1B9A3385843B for ; Tue, 7 Sep 2021 17:44:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1B9A3385843B Received: by mail-pl1-x631.google.com with SMTP id u1so6290812plq.5 for ; Tue, 07 Sep 2021 10:44:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=GMf2FtqKG8JQD4HOOfwdRHm3XlbZqalRK1HrRA6NcFo=; b=G4L+N9DJULIcJNdMEKwgSWHPRI4GypKJIG9J3QuGrSYj/lo70VXWwBK5faGzl7W00F KrE+RybmPImTUzzR/lCed6S4U2hGWkMXYYU7hAm/KFlvU6d9Bf1E0M567dXgh2gL/Ufm tbMew7JAQSHV0eg4RgViczhLpu/kFn2UGLxl3070OZgqFbrnz90rbESC/F/hyrbwoW+h 87ZqeyvN/Qn48vRhII1VNCDnBmBwCo7Q1Px1eCBShOyj8krsdePYr3F7AgAgUEznEuLt GlJkpAJXy87069DVq9BddaT2/P+c4NCQTm3jNBbL/XwrmOncraI39yA/2pqM7NVWtKEN VRWQ== X-Gm-Message-State: AOAM5303oZKjp5jHp3HgK9yrG7H7OKDHZOBoV8Wt2L8QrlITmgvq0UAL i1IivON6urE4fRB957PPWixtxQ== X-Google-Smtp-Source: ABdhPJw0DBuxCaHYjznzdClLxP8befOKLykOZQ9ONqOhvEj5MNaiOkJvR5+vTU1xDvjzr5JmZ7IAMA== X-Received: by 2002:a17:90a:6f24:: with SMTP id d33mr5770828pjk.239.1631036661938; Tue, 07 Sep 2021 10:44:21 -0700 (PDT) Received: from google.com ([2620:15c:2ce:200:5e58:ee8:7bca:ad2e]) by smtp.gmail.com with ESMTPSA id g3sm13824460pgj.66.2021.09.07.10.44.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Sep 2021 10:44:21 -0700 (PDT) Date: Tue, 7 Sep 2021 10:44:17 -0700 From: Fangrui Song To: Lukasz Majewski Cc: Adhemerval Zanella , Florian Weimer , Joseph Myers , Carlos O'Donell , libc-alpha Subject: Re: [PATCH] dl: Use "adr" assembler command to get proper load address Message-ID: <20210907174417.sctsswphsyae4mpc@google.com> References: <20210907131616.23472-1-lukma@denx.de> <20210907164906.yt6nonvfyhvbrx6p@google.com> <20210907193227.6047f9cc@ktm> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20210907193227.6047f9cc@ktm> X-Spam-Status: No, score=-27.2 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Sep 2021 17:44:24 -0000 On 2021-09-07, Lukasz Majewski wrote: >Hi Fangrui, > >> On 2021-09-07, Lukasz Majewski wrote: >> >This change is a partial revert of commit >> >bca0f5cbc9257c13322b99e55235c4f21ba0bd82 which imposed usage of >> >__ehdr_start linker variable to get the address of loaded program. >> > >> >The elf_machine_load_address() function is declared in the >> >sysdeps/arm/dl-machine.h header. It is called from _dl_start() entry >> >point for the program. It shall return the load address of the >> >dynamic linker program. >> >> Yes. >> >> >With this revert the 'adr' assembler instruction is used instead of a >> >place holder: >> > >> >arm-poky-linux-gnueabi-objdump -t ld-linux-armhf.so.3 | grep ehdr >> >00000000 l .note.gnu.build-id 00000000 __ehdr_start >> > >> >which shall be pre-set by binutils. >> >> I don't understand this. The sh_addr field of the binutils defined >> __ehdr_start is 0. >> >> Declararing __ehdr_start as hidden and accessing it with PC-relative >> addressing generates computes the runtime address of __ehdr_start, >> which equals the load base. > >Maybe I don't get it - but shouldn't this be filled in by binutils >(linker ?) when the program is assembled before run? The linker just sets its link-time address (sh_addr): 0. The address is computed at runtime. >Or is the __ehdr_start used just as a relative offset to get the proper >position of ld-linux-armh.so when called? It's a relative offset. % cat a.c __attribute__((visibility("hidden"))) extern char __ehdr_start[]; char *load_address() { return __ehdr_start; } % arm-linux-gnueabi-gcc -O1 -fpic a.c -nostdlib % arm-linux-gnueabi-objdump -dr a.out a.out: file format elf32-littlearm Disassembly of section .text: 00000198 : 198: e59f0004 ldr r0, [pc, #4] ; 1a4 19c: e08f0000 add r0, pc, r0 1a0: e12fff1e bx lr 1a4: fffffe5c .word 0xfffffe5c At runtime, `load_address` will compute the load address. Note that there is no runtime relocation. >> >> What's wrong with it? The objdump -t line doesn't tell why this change >> is good. > >I'm also puzzled why this causes the QEMU versalite board to break. >(/sbin/init dies with SIGSEGFAULT = 0xb [*]) > >It looks like the &__ehdr_start is not equal in the result to 'adr' >assembler code in QEMU. > >I also thought that the issue is caused by fs/binfmt_elf.c changes in >the Linux kernel (as mine is 5.1), so I've tested it with 5.10 and >5.14. No change. The kernel change must be unrelated. The kernel doesn't even know __ehdr_start as a symbol exists. >The QEMU is pretty recent - it is 6.0.0 version for ARM. > >> Can you compare objdump -dr output and show the incorrect >> code sequences with C assessing __ehdr_start? > >Ok. I will explicitly compare _dl_start function with and without this >patch. In case it helps, perhaps replace always_inline with noinline and check why the function is incorrect under your qemu setup. >> >> >This is crucial in the QEMU ARM environment for which (when >> >/sbin/init is executed) values set in __ehdr_start symbol are wrong. >> >This causes the program to crash very early - when the >> >/lib/ld-linux-armhf.so.3 is executed as a prerequisite to /sbin/init >> >execution. >> >> >The kernel's fs/binfmt_elf.c is though responsible for setting >> >up execution environment, not binutils. >> >> Whether the symbol entry __ehdr_start exists doesn't matter. >> The hidden definition does not have any associated relocation. > >I'm also wondering if similar issue is visible with "normal" - i.e. not >QEMU ARM run init. > >Maybe the "rough" environment in which /sbin/init is run is the culprit? > I had tested running ld.so and elf/ldconfig which covered the usage. I don't know. It needs some debugging from you:) >> >> >It looks like the only robust way to obtain the _dl_start offset is >> >to use assembler instruction - not rely on values provided by >> >binutils. >> > >> >HW: >> >Hardware name: ARM-Versatile Express (Run with QEMU) >> >Tested (affected) kernels v5.1.12, v5.10.62 and v5.14.1 >> > >> >When the /sbin/init is setup for run from Linux kernel's very small >> >environment with LD_DEBUG=all the __ehdr_start is not shown at all. >> > >> >Fixes: BZ #28293 >> >--- >> > sysdeps/arm/dl-machine.h | 28 +++++++++++++++++++++++++--- >> > 1 file changed, 25 insertions(+), 3 deletions(-) >> > >> >diff --git a/sysdeps/arm/dl-machine.h b/sysdeps/arm/dl-machine.h >> >index eb13cb8b57..58ebef6ecd 100644 >> >--- a/sysdeps/arm/dl-machine.h >> >+++ b/sysdeps/arm/dl-machine.h >> >@@ -38,11 +38,33 @@ elf_machine_matches_host (const Elf32_Ehdr *ehdr) >> > } >> > >> > /* Return the run-time load address of the shared object. */ >> >-static inline ElfW(Addr) __attribute__ ((unused)) >> >+static inline Elf32_Addr __attribute__ ((unused)) >> > elf_machine_load_address (void) >> > { >> >- extern const ElfW(Ehdr) __ehdr_start attribute_hidden; >> >- return (ElfW(Addr)) &__ehdr_start; >> >+ Elf32_Addr pcrel_addr; >> >+#ifdef SHARED >> >+ extern Elf32_Addr __dl_start (void *) asm ("_dl_start"); >> >+ Elf32_Addr got_addr = (Elf32_Addr) &__dl_start; >> >+ asm ("adr %0, _dl_start" : "=r" (pcrel_addr)); >> >+#else >> >+ extern Elf32_Addr __dl_relocate_static_pie (void *) >> >+ asm ("_dl_relocate_static_pie") attribute_hidden; >> >+ Elf32_Addr got_addr = (Elf32_Addr) &__dl_relocate_static_pie; >> >+ asm ("adr %0, _dl_relocate_static_pie" : "=r" (pcrel_addr)); >> >+#endif >> >+#ifdef __thumb__ >> >+ /* Clear the low bit of the function address. >> >+ >> >+ NOTE: got_addr is from GOT table whose lsb is always set by >> >linker if it's >> >+ Thumb function address. PCREL_ADDR comes from PC-relative >> >calculation >> >+ which will finish during assembling. GAS assembler before the >> >fix for >> >+ PR gas/21458 was not setting the lsb but does after that. >> >Always do the >> >+ strip for both, so the code works with various combinations of >> >glibc and >> >+ Binutils. */ >> >+ got_addr &= ~(Elf32_Addr) 1; >> >+ pcrel_addr &= ~(Elf32_Addr) 1; >> >+#endif >> >+ return pcrel_addr - got_addr; >> > } >> > >> > /* Return the link-time address of _DYNAMIC. */ >> >-- >> >2.20.1 >> > > >Note: > >[*] - the QEMU debugging is pretty difficult with /sbin/init. Qemu has >good support for kernel debugging (-s -S switches). Then for user space >program one could use the "on-board" gdb or gdbserver. > >However, /sbin/init needs to be debugged from linux and scheduler >context switches needs to be taken into account to review how the code >is executed. And such debugging scenario has issue with QEMU-arm. > > >Best regards, > >Lukasz Majewski > >-- > >DENX Software Engineering GmbH, Managing Director: Wolfgang Denk >HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de