On 27/11/2019 18:17, Andrew Cooper wrote: > On 21/11/2019 08:34, Jan Beulich wrote: >> On 20.11.2019 18:13, Andrew Cooper wrote: >>> On 20/11/2019 16:40, Jürgen Groß wrote: >>>> On 20.11.19 17:30, Jan Beulich wrote: >>>>> On 08.11.2019 12:18, Jan Beulich wrote: >>>>>> The .file assembler directives generated by the compiler do not include >>>>>> any path components (gcc) or just the ones specified on the command >>>>>> line >>>>>> (clang, at least version 5), and hence multiple identically named >>>>>> source >>>>>> files (in different directories) may produce identically named static >>>>>> symbols (in their kallsyms representation). The binary diffing >>>>>> algorithm >>>>>> used by xen-livepatch, however, depends on having unique symbols. >>>>>> >>>>>> Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build) >>>>>> behavior, and if enabled use objcopy to prepend the (relative to the >>>>>> xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note >>>>>> that this build option is made no longer depend on LIVEPATCH, but >>>>>> merely >>>>>> defaults to its setting now. >>>>>> >>>>>> Conditionalize explicit .file directive insertion in C files where it >>>>>> exists just to disambiguate names in a less generic manner; note that >>>>>> at the same time the redundant emission of STT_FILE symbols gets >>>>>> suppressed for clang. Assembler files as well as multiply compiled C >>>>>> ones using __OBJECT_FILE__ are left alone for the time being. >>>>>> >>>>>> Since we now expect there not to be any duplicates anymore, also don't >>>>>> force the selection of the option to 'n' anymore in allrandom.config. >>>>>> Similarly COVERAGE no longer suppresses duplicate symbol warnings if >>>>>> enforcement is in effect, which in turn allows >>>>>> SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on >>>>>> !ENFORCE_UNIQUE_SYMBOLS. >>>>>> >>>>>> Signed-off-by: Jan Beulich <jbeul...@suse.com> >>>>> I've got acks from Konrad and Wei, but still need an x86 and a release >>>>> one here. Andrew? Or alternatively - Jürgen, would you rather not see >>>>> this go in anymore? >>>> In case the needed x86 Ack is coming in before RC3 I'm fine to give my >>>> Release-ack, but I'm hesitant to take it later. >>> Has anyone actually tried building a livepatch with this change in place? >> Actually - what is your concern here? The exact spelling of symbols >> names should be of no interest to the tool. After all the compiler is >> free to invent all sorts of names for its local symbols, including >> the ones we would produce with this change in place. All the tool >> cares about is that the names be unambiguous. Hence any (theoretical) >> regression here would be a bug in the tools, which imo is no reason >> to delay this change any further. (Granted I should have got to it >> earlier, but it had been continuing to get deferred.) > > This might all be true (theoretically). > > The livepatch build tools are fragile and very sensitive to how the > object files are laid out. There is a very real risk that this change > accidentally breaks livepatching totally, even on GCC builds. > > Were this to happen, it would be yet another 4.13 regression. > > This is a change to fix a concrete livepatch issue with Clang. Sure - > it resolves the symbol uniqueness failures for the in-tree build, but > considering the risks to the area you are modifying, the fact you > haven't even done a dev test of a livepatch build on GCC means that the > patch as a whole has not had what I would consider a reasonable amount > of testing. > > Luckily for you, Ross and Sergey have agreed to smoke test this with > some livepatches. They will report on this thread with their findings.
Applying the patch didn't end up well for my test LP (from another thread): Perform full initial build with 8 CPU(s)... Reading special section data Apply patch and build with 8 CPU(s)... Unapply patch and build with 8 CPU(s)... Extracting new and modified ELF sections... Processing xen/arch/x86/mm/shadow/guest_2.o Processing xen/arch/x86/mm/shadow/guest_4.o Processing xen/arch/x86/mm/shadow/guest_3.o Processing xen/arch/x86/mm/guest_walk_3.o Processing xen/arch/x86/mm/hap/guest_walk_3level.o Processing xen/arch/x86/mm/hap/guest_walk_4level.o Processing xen/arch/x86/mm/hap/guest_walk_2level.o Processing xen/arch/x86/mm/guest_walk_2.o Processing xen/arch/x86/mm/guest_walk_4.o Processing xen/arch/x86/efi/efi/check.o Processing xen/arch/x86/pv/gpr_switch.o Processing xen/arch/x86/indirect-thunk.o Processing xen/arch/x86/boot/head.o Processing xen/arch/x86/x86_64/kexec_reloc.o Processing xen/arch/x86/x86_64/compat/entry.o Processing xen/arch/x86/x86_64/entry.o Processing xen/arch/x86/hvm/vmx/entry.o Processing xen/arch/x86/hvm/svm/entry.o Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.0s.o Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.0r.o Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.1s.o Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.1r.o ERROR: no functional changes found. So this looks like a regression. -- Thanks, Sergey _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
