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

[Will Deacon <will@kernel.org>]

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