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

[Vincenzo Frascino <vincenzo.frascino@arm.com>]

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