On 31.03.2022 10:27, Roger Pau Monné wrote: > On Thu, Mar 31, 2022 at 10:13:06AM +0200, Jan Beulich wrote: >> On 31.03.2022 10:01, Roger Pau Monné wrote: >>> On Thu, Mar 31, 2022 at 08:42:47AM +0200, Jan Beulich wrote: >>>> On 30.03.2022 19:04, Roger Pau Monné wrote: >>>>> On Wed, Mar 30, 2022 at 01:05:31PM +0200,>> --- a/xen/arch/x86/livepatch.c >>>>>> +++ b/xen/arch/x86/livepatch.c >>>>>> @@ -157,9 +157,15 @@ void noinline arch_livepatch_apply(struc >>>>>> * loaded hotpatch (to avoid racing against other fixups >>>>>> adding/removing >>>>>> * ENDBR64 or similar instructions). >>>>>> */ >>>>>> - if ( is_endbr64(old_ptr) || is_endbr64_poison(func->old_addr) ) >>>>>> + if ( len >= ENDBR64_LEN && >>>>> >>>>> Sorry, didn't realize before, but shouldn't this check be using >>>>> old_size instead of len (which is based on new_size)? >>>> >>>> Yes and no: In principle yes, but with len == func->new_size in the NOP >>>> case, and with arch_livepatch_verify_func() guaranteeing new_size <= >>>> old_size, the check is still fine for that case. Plus: If new_size was >>>> less than 4 _but_ there's an ENDBR64 at the start, what would we do? I >>>> think there's more that needs fixing in this regard. So I guess I'll >>>> make a v3 with this extra fix dropped and with the livepatch_insn_len() >>>> invocation simply moved. After all the primary goal is to get the >>>> stable trees unstuck. >>> >>> Right, I agree to try and get the stable trees unblocked ASAP. >>> >>> I think the check for ENDBR is only relevant when we are patching the >>> function with a jump, otherwise the new replacement code should >>> contain the ENDBR instruction already? >> >> No, the "otherwise" case is when we're NOP-ing out code, i.e. when >> there's no replacement code at all. In this case we need to leave >> intact the ENDBR, and new_size being less than 4 is bogus afaict in >> case there actually is an ENDBR. > > Hm, so we never do in-place replacement of code, and we either > introduce a jump to the new code or otherwise the function is not to > be called anymore and hence we fill it with no-ops?
If it wasn't to be called anymore, it would be better to fill the space with INT3, not NOP. I think the purpose isn't really to nop out entire functions; it's just that the NOP testcase in the tree does so. > Shouldn't in the no-op filling case the call to add_nops be bounded by > old_size and salso the memcpy to old_addr? > > I'm not sure I understand why we use new_size when memcpy'ing into > old_addr, or when filling the insn buffer. I was wondering too - it would have seemed more natural to either require old_size == new_size in this case, or to demand new_size == 0 matching new_addr == NULL. I'm afraid I have to rely on the livepatch maintainers to answer your questions. Jan
