[PATCH] arm64/vdso: Remove unused makefile variable

[PATCH] arm64/vdso: Remove unused makefile variable

[Geoff Levand]

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(-)

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
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/vdso: Remove unused makefile variable

[Will Deacon]

[+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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/vdso: Remove unused makefile variable

[Vincenzo Frascino]

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
> 

-- 
Regards,
Vincenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/vdso: Remove unused makefile variable

[Ard Biesheuvel]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/vdso: Remove unused makefile variable

[Will Deacon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/vdso: Remove unused makefile variable

[Ard Biesheuvel]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/vdso: Remove unused makefile variable

[Will Deacon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/vdso: Remove unused makefile variable

[Vincenzo Frascino]

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
> 

-- 
Regards,
Vincenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel