Re: [PATCH v2] ARM: module: Fix function kallsyms on Thumb-2

[Jessica Yu]

+++ Vincent Whitchurch [09/11/18 14:53 +0100]:
> On Fri, Nov 02, 2018 at 02:53:22PM +0100, Jessica Yu wrote:
> > +++ Vincent Whitchurch [01/11/18 16:29 +0100]:
> > > On Wed, Oct 31, 2018 at 04:53:41PM +0100, Jessica Yu wrote:
> > > > Could this be done in modpost? I'm guessing the answer is no as some
> > > > relocations may rely on that bit being set in st_value, right?
> > > > Therefore we can only clear the bit _after_ relocations to the module
> > > > are applied at runtime, correct?
> > >
> > > Yes, that's correct, it needs to be done after the relocations.
> > >
> > > > I'm not against having an arch-specific kallsyms fixup function, my
> > > > only concern would be if this would interfere with the delayed
> > > > relocations livepatching does, if there are plans in the future to
> > > > have livepatching on arm (IIRC there was an attempt at a port in
> > > > 2016). If there exists some Thumb-2 relocation types that rely on that
> > > > lowest bit in st_value being set in order to be applied, and we clear
> > > > it permanently from the symtab, then livepatching wouldn't be able to
> > > > apply those types of relocations anymore. If this is a legitimate
> > > > concern, then perhaps an alternative solution would be to have an
> > > > arch-specific kallsyms symbol-value-fixup function for accesses to
> > > > sym.st_value, without modifying the module symtab.
> > >
> > > I'm not familiar with livepatching, but yes, if it needs to do
> > > relocations later I guess we should preserve the original value.
> >
> > Yeah, I think the symtab needs to remain unmodified for livepatch to
> > be able to do delayed relocations after module load.
> >
> > > I gave the alternative solution a go and it seems to work.
> > > add_kallsyms() currently overwrites st_info so I had to move the
> > > elf_type to the unused st_other field instead to preserve st_info:
> >
> > I think that's fine, I think the only usages of st_other I've found
> > are during relocations, and the field is big enough for a char, so it
> > should be fine to reuse it afterwards.
>
> If livepatch can do relocations later, doesn't that mean that we _can't_
> reuse st_other for storing the elf_type?  OTOH, the current code
> overwrites st_info, and I see that used from various archs' relocation
> functions too, so perhaps I'm missing something.

D'oh, I just contradicted myself. Yes, you are correct. AFAIK the only
reason we haven't run into issues yet in livepatch is because the only
arches it currently supports (x86, s390, powerpc) don't depend on
st_info for module relocations. But yes, that's an outstanding problem
that will need to be fixed if livepatch gains support on more arches,
we shouldn't be overwriting that field. And after grepping around I do
see that st_other is used in powerpc, alpha, sh for module
relocations. I suggested in my other reply to your v3 patch that
reusing st_size instead of st_info might work, since it's not used in
the module loader/kallsyms, and arches don't seem to use it for module
relocs. Maybe that would work?

Jessica