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.
