| Message ID | d8baa0e5-0e20-9f58-5e79-34a272f86d1d@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/vdso: Remove unused makefile variable | expand |
[+Vincenzo] On Fri, Apr 24, 2020 at 08:58:49AM -0700, Geoff Levand wrote: > The vdso makefile variable VDSO_LDFLAGS is defined, but never used, > so remove it. > > Signed-off-by: Geoff Levand <geoff@infradead.org> > --- > > Hi, > > This seems to be left over from a code cleanup that missed it. While I agree that this isn't used, I'm wondering why '-Bsymbolic' is used to link the compat vDSO but not the native one. It seems weird to differ in this regard. Vincenzo? Looks like you added this unused variable in 28b1a824a4f44 ("arm64: vdso: Substitute gettimeofday() with C implementation"). Will
Hi Will, thank you for pointing this out. On 4/27/20 9:35 PM, Will Deacon wrote: > [+Vincenzo] > > On Fri, Apr 24, 2020 at 08:58:49AM -0700, Geoff Levand wrote: >> The vdso makefile variable VDSO_LDFLAGS is defined, but never used, >> so remove it. >> >> Signed-off-by: Geoff Levand <geoff@infradead.org> >> --- >> >> Hi, >> >> This seems to be left over from a code cleanup that missed it. > > While I agree that this isn't used, I'm wondering why '-Bsymbolic' is used > to link the compat vDSO but not the native one. It seems weird to differ > in this regard. > > Vincenzo? Looks like you added this unused variable in 28b1a824a4f44 > ("arm64: vdso: Substitute gettimeofday() with C implementation"). > My understanding is that "-Bsymbolic" is required by both compat and normal vdso because when the shared library is built it adds a flag in the dynamic section of the binary called DT_SYMBOLIC which alters the dynamic linker's symbol resolution algorithm to search for references for a symbol inside the library first and then into the executable. This becomes useful for example when an executable built with -fPIC is trying to call a public vDSO function from assembly (bl symbol). The issue here seems to be that I used VDSO_LDFLAGS instead of ldflags-y. I can post a patch and Cc stable. Adding Geoff as Reported-by. > Will >
On Tue, 28 Apr 2020 at 12:45, Vincenzo Frascino <vincenzo.frascino@arm.com> wrote: > > Hi Will, > > thank you for pointing this out. > > On 4/27/20 9:35 PM, Will Deacon wrote: > > [+Vincenzo] > > > > On Fri, Apr 24, 2020 at 08:58:49AM -0700, Geoff Levand wrote: > >> The vdso makefile variable VDSO_LDFLAGS is defined, but never used, > >> so remove it. > >> > >> Signed-off-by: Geoff Levand <geoff@infradead.org> > >> --- > >> > >> Hi, > >> > >> This seems to be left over from a code cleanup that missed it. > > > > While I agree that this isn't used, I'm wondering why '-Bsymbolic' is used > > to link the compat vDSO but not the native one. It seems weird to differ > > in this regard. > > > > Vincenzo? Looks like you added this unused variable in 28b1a824a4f44 > > ("arm64: vdso: Substitute gettimeofday() with C implementation"). > > > > My understanding is that "-Bsymbolic" is required by both compat and normal vdso > because when the shared library is built it adds a flag in the dynamic section > of the binary called DT_SYMBOLIC which alters the dynamic linker's symbol > resolution algorithm to search for references for a symbol inside the library > first and then into the executable. > DT_SYMBOLIC doesn't (or shouldn't) change the dynamic linking behavior. It informs the linker that ELF symbol preemption may not work, since the .so has bound internal references to its exported symbols to the internal versions directly, rather than allowing the application to supersede (i.e., 'preempt') them. This is an obscure feature that isn't really relevant for the VDSO, since we carefully control what we export from the .so anyway (via the linker script's VERSIONS section) > This becomes useful for example when an executable built with -fPIC is trying to > call a public vDSO function from assembly (bl symbol). > > The issue here seems to be that I used VDSO_LDFLAGS instead of ldflags-y. I can > post a patch and Cc stable. Adding Geoff as Reported-by. > I think it can be removed.
On Tue, Apr 28, 2020 at 01:52:55PM +0200, Ard Biesheuvel wrote: > On Tue, 28 Apr 2020 at 12:45, Vincenzo Frascino > <vincenzo.frascino@arm.com> wrote: > > On 4/27/20 9:35 PM, Will Deacon wrote: > > > [+Vincenzo] > > > > > > On Fri, Apr 24, 2020 at 08:58:49AM -0700, Geoff Levand wrote: > > >> The vdso makefile variable VDSO_LDFLAGS is defined, but never used, > > >> so remove it. > > >> > > >> Signed-off-by: Geoff Levand <geoff@infradead.org> > > >> --- > > >> > > >> Hi, > > >> > > >> This seems to be left over from a code cleanup that missed it. > > > > > > While I agree that this isn't used, I'm wondering why '-Bsymbolic' is used > > > to link the compat vDSO but not the native one. It seems weird to differ > > > in this regard. > > > > > > Vincenzo? Looks like you added this unused variable in 28b1a824a4f44 > > > ("arm64: vdso: Substitute gettimeofday() with C implementation"). > > > > > > > My understanding is that "-Bsymbolic" is required by both compat and normal vdso > > because when the shared library is built it adds a flag in the dynamic section > > of the binary called DT_SYMBOLIC which alters the dynamic linker's symbol > > resolution algorithm to search for references for a symbol inside the library > > first and then into the executable. > > > > DT_SYMBOLIC doesn't (or shouldn't) change the dynamic linking > behavior. It informs the linker that ELF symbol preemption may not > work, since the .so has bound internal references to its exported > symbols to the internal versions directly, rather than allowing the > application to supersede (i.e., 'preempt') them. This is an obscure > feature that isn't really relevant for the VDSO, since we carefully > control what we export from the .so anyway (via the linker script's > VERSIONS section) > > > This becomes useful for example when an executable built with -fPIC is trying to > > call a public vDSO function from assembly (bl symbol). > > > > The issue here seems to be that I used VDSO_LDFLAGS instead of ldflags-y. I can > > post a patch and Cc stable. Adding Geoff as Reported-by. > > > > I think it can be removed. Hmm, so I did a little bit more digging because -Bsymbolic is used to link the vDSO on arm, mips, sparc and x86. Commit 6f121e548f83 ("x86, vdso: Reimplement vdso.so preparation in build-time C") suggests that it's a good idea to prevent any unexpected dynamic relocations appearing in the vDSO object. Will
On Tue, 28 Apr 2020 at 14:43, Will Deacon <will@kernel.org> wrote: > > On Tue, Apr 28, 2020 at 01:52:55PM +0200, Ard Biesheuvel wrote: > > On Tue, 28 Apr 2020 at 12:45, Vincenzo Frascino > > <vincenzo.frascino@arm.com> wrote: > > > On 4/27/20 9:35 PM, Will Deacon wrote: > > > > [+Vincenzo] > > > > > > > > On Fri, Apr 24, 2020 at 08:58:49AM -0700, Geoff Levand wrote: > > > >> The vdso makefile variable VDSO_LDFLAGS is defined, but never used, > > > >> so remove it. > > > >> > > > >> Signed-off-by: Geoff Levand <geoff@infradead.org> > > > >> --- > > > >> > > > >> Hi, > > > >> > > > >> This seems to be left over from a code cleanup that missed it. > > > > > > > > While I agree that this isn't used, I'm wondering why '-Bsymbolic' is used > > > > to link the compat vDSO but not the native one. It seems weird to differ > > > > in this regard. > > > > > > > > Vincenzo? Looks like you added this unused variable in 28b1a824a4f44 > > > > ("arm64: vdso: Substitute gettimeofday() with C implementation"). > > > > > > > > > > My understanding is that "-Bsymbolic" is required by both compat and normal vdso > > > because when the shared library is built it adds a flag in the dynamic section > > > of the binary called DT_SYMBOLIC which alters the dynamic linker's symbol > > > resolution algorithm to search for references for a symbol inside the library > > > first and then into the executable. > > > > > > > DT_SYMBOLIC doesn't (or shouldn't) change the dynamic linking > > behavior. It informs the linker that ELF symbol preemption may not > > work, since the .so has bound internal references to its exported > > symbols to the internal versions directly, rather than allowing the > > application to supersede (i.e., 'preempt') them. This is an obscure > > feature that isn't really relevant for the VDSO, since we carefully > > control what we export from the .so anyway (via the linker script's > > VERSIONS section) > > > > > This becomes useful for example when an executable built with -fPIC is trying to > > > call a public vDSO function from assembly (bl symbol). > > > > > > The issue here seems to be that I used VDSO_LDFLAGS instead of ldflags-y. I can > > > post a patch and Cc stable. Adding Geoff as Reported-by. > > > > > > > I think it can be removed. > > Hmm, so I did a little bit more digging because -Bsymbolic is used to link > the vDSO on arm, mips, sparc and x86. Commit 6f121e548f83 ("x86, vdso: > Reimplement vdso.so preparation in build-time C") suggests that it's a good > idea to prevent any unexpected dynamic relocations appearing in the vDSO > object. > In the x86 case, there are internal calls to the exported routines, and without Bsymbolic, those are routed via a GOT/PLT so that the application can override those symbols. For instance, under the normal ELF symbol preemption rules, the x86 VDSO should use the application's version of __kernel_vsyscall() if it exists as a global symbol, and so the interposable dynamic relocation is made to point to the application's version of the symbol. That is why x86 needs -Bsymbolic. That issue does not exist on arm64, as far as I can tell. It doesn't really hurt either to have the option, but it would be good to perhaps annotate why we are keeping it.
On Tue, Apr 28, 2020 at 03:02:43PM +0200, Ard Biesheuvel wrote: > On Tue, 28 Apr 2020 at 14:43, Will Deacon <will@kernel.org> wrote: > > > > On Tue, Apr 28, 2020 at 01:52:55PM +0200, Ard Biesheuvel wrote: > > > On Tue, 28 Apr 2020 at 12:45, Vincenzo Frascino > > > <vincenzo.frascino@arm.com> wrote: > > > > On 4/27/20 9:35 PM, Will Deacon wrote: > > > > > [+Vincenzo] > > > > > > > > > > On Fri, Apr 24, 2020 at 08:58:49AM -0700, Geoff Levand wrote: > > > > >> The vdso makefile variable VDSO_LDFLAGS is defined, but never used, > > > > >> so remove it. > > > > >> > > > > >> Signed-off-by: Geoff Levand <geoff@infradead.org> > > > > >> --- > > > > >> > > > > >> Hi, > > > > >> > > > > >> This seems to be left over from a code cleanup that missed it. > > > > > > > > > > While I agree that this isn't used, I'm wondering why '-Bsymbolic' is used > > > > > to link the compat vDSO but not the native one. It seems weird to differ > > > > > in this regard. > > > > > > > > > > Vincenzo? Looks like you added this unused variable in 28b1a824a4f44 > > > > > ("arm64: vdso: Substitute gettimeofday() with C implementation"). > > > > > > > > > > > > > My understanding is that "-Bsymbolic" is required by both compat and normal vdso > > > > because when the shared library is built it adds a flag in the dynamic section > > > > of the binary called DT_SYMBOLIC which alters the dynamic linker's symbol > > > > resolution algorithm to search for references for a symbol inside the library > > > > first and then into the executable. > > > > > > > > > > DT_SYMBOLIC doesn't (or shouldn't) change the dynamic linking > > > behavior. It informs the linker that ELF symbol preemption may not > > > work, since the .so has bound internal references to its exported > > > symbols to the internal versions directly, rather than allowing the > > > application to supersede (i.e., 'preempt') them. This is an obscure > > > feature that isn't really relevant for the VDSO, since we carefully > > > control what we export from the .so anyway (via the linker script's > > > VERSIONS section) > > > > > > > This becomes useful for example when an executable built with -fPIC is trying to > > > > call a public vDSO function from assembly (bl symbol). > > > > > > > > The issue here seems to be that I used VDSO_LDFLAGS instead of ldflags-y. I can > > > > post a patch and Cc stable. Adding Geoff as Reported-by. > > > > > > > > > > I think it can be removed. > > > > Hmm, so I did a little bit more digging because -Bsymbolic is used to link > > the vDSO on arm, mips, sparc and x86. Commit 6f121e548f83 ("x86, vdso: > > Reimplement vdso.so preparation in build-time C") suggests that it's a good > > idea to prevent any unexpected dynamic relocations appearing in the vDSO > > object. > > > > In the x86 case, there are internal calls to the exported routines, > and without Bsymbolic, those are routed via a GOT/PLT so that the > application can override those symbols. For instance, under the normal > ELF symbol preemption rules, the x86 VDSO should use the application's > version of __kernel_vsyscall() if it exists as a global symbol, and so > the interposable dynamic relocation is made to point to the > application's version of the symbol. That is why x86 needs -Bsymbolic. > > That issue does not exist on arm64, as far as I can tell. It doesn't > really hurt either to have the option, but it would be good to perhaps > annotate why we are keeping it. Yes, so I think we either remove it for arm, arm64 compat and arm64 native or we add it to arm64 native for consistency/over-zealous future-proofing. In either case, we need to document it somewhere so we don't run into this again in future. Vincenzo -- are you able to send a patch, please? Will
Hi Will, On 4/28/20 2:16 PM, Will Deacon wrote: > On Tue, Apr 28, 2020 at 03:02:43PM +0200, Ard Biesheuvel wrote: >> On Tue, 28 Apr 2020 at 14:43, Will Deacon <will@kernel.org> wrote: >>> >>> On Tue, Apr 28, 2020 at 01:52:55PM +0200, Ard Biesheuvel wrote: >>>> On Tue, 28 Apr 2020 at 12:45, Vincenzo Frascino >>>> <vincenzo.frascino@arm.com> wrote: >>>>> On 4/27/20 9:35 PM, Will Deacon wrote: >>>>>> [+Vincenzo] >>>>>> >>>>>> On Fri, Apr 24, 2020 at 08:58:49AM -0700, Geoff Levand wrote: >>>>>>> The vdso makefile variable VDSO_LDFLAGS is defined, but never used, >>>>>>> so remove it. >>>>>>> >>>>>>> Signed-off-by: Geoff Levand <geoff@infradead.org> >>>>>>> --- >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> This seems to be left over from a code cleanup that missed it. >>>>>> >>>>>> While I agree that this isn't used, I'm wondering why '-Bsymbolic' is used >>>>>> to link the compat vDSO but not the native one. It seems weird to differ >>>>>> in this regard. >>>>>> >>>>>> Vincenzo? Looks like you added this unused variable in 28b1a824a4f44 >>>>>> ("arm64: vdso: Substitute gettimeofday() with C implementation"). >>>>>> >>>>> >>>>> My understanding is that "-Bsymbolic" is required by both compat and normal vdso >>>>> because when the shared library is built it adds a flag in the dynamic section >>>>> of the binary called DT_SYMBOLIC which alters the dynamic linker's symbol >>>>> resolution algorithm to search for references for a symbol inside the library >>>>> first and then into the executable. >>>>> >>>> >>>> DT_SYMBOLIC doesn't (or shouldn't) change the dynamic linking >>>> behavior. It informs the linker that ELF symbol preemption may not >>>> work, since the .so has bound internal references to its exported >>>> symbols to the internal versions directly, rather than allowing the >>>> application to supersede (i.e., 'preempt') them. This is an obscure >>>> feature that isn't really relevant for the VDSO, since we carefully >>>> control what we export from the .so anyway (via the linker script's >>>> VERSIONS section) >>>> >>>>> This becomes useful for example when an executable built with -fPIC is trying to >>>>> call a public vDSO function from assembly (bl symbol). >>>>> >>>>> The issue here seems to be that I used VDSO_LDFLAGS instead of ldflags-y. I can >>>>> post a patch and Cc stable. Adding Geoff as Reported-by. >>>>> >>>> >>>> I think it can be removed. >>> >>> Hmm, so I did a little bit more digging because -Bsymbolic is used to link >>> the vDSO on arm, mips, sparc and x86. Commit 6f121e548f83 ("x86, vdso: >>> Reimplement vdso.so preparation in build-time C") suggests that it's a good >>> idea to prevent any unexpected dynamic relocations appearing in the vDSO >>> object. >>> >> >> In the x86 case, there are internal calls to the exported routines, >> and without Bsymbolic, those are routed via a GOT/PLT so that the >> application can override those symbols. For instance, under the normal >> ELF symbol preemption rules, the x86 VDSO should use the application's >> version of __kernel_vsyscall() if it exists as a global symbol, and so >> the interposable dynamic relocation is made to point to the >> application's version of the symbol. That is why x86 needs -Bsymbolic. >> >> That issue does not exist on arm64, as far as I can tell. It doesn't >> really hurt either to have the option, but it would be good to perhaps >> annotate why we are keeping it. > > Yes, so I think we either remove it for arm, arm64 compat and arm64 native > or we add it to arm64 native for consistency/over-zealous future-proofing. > In either case, we need to document it somewhere so we don't run into this > again in future. > > Vincenzo -- are you able to send a patch, please? > Already working on it. My preference is to keep it, I am adding documentation to the patch notes and in the Makefile. > Will >
diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile index dd2514bb1511..6f6b55c12029 100644 --- a/arch/arm64/kernel/vdso/Makefile +++ b/arch/arm64/kernel/vdso/Makefile @@ -23,8 +23,6 @@ ldflags-y := -shared -nostdlib -soname=linux-vdso.so.1 --hash-style=sysv \ ccflags-y := -fno-common -fno-builtin -fno-stack-protector -ffixed-x18 ccflags-y += -DDISABLE_BRANCH_PROFILING -VDSO_LDFLAGS := -Bsymbolic - CFLAGS_REMOVE_vgettimeofday.o = $(CC_FLAGS_FTRACE) -Os KBUILD_CFLAGS += $(DISABLE_LTO) KASAN_SANITIZE := n
The vdso makefile variable VDSO_LDFLAGS is defined, but never used, so remove it. Signed-off-by: Geoff Levand <geoff@infradead.org> --- Hi, This seems to be left over from a code cleanup that missed it. -Geoff arch/arm64/kernel/vdso/Makefile | 2 -- 1 file changed, 2 deletions(-)