Re: [PATCH v3] livepatch: account for patch offset when applying NOP patch

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.03.2022 10:21, Roger Pau Monné wrote:
> On Thu, Mar 31, 2022 at 08:49:46AM +0200, Jan Beulich wrote:
>> While not triggered by the trivial xen_nop in-tree patch on
>> staging/master, that patch exposes a problem on the stable trees, where
>> all functions have ENDBR inserted. When NOP-ing out a range, we need to
>> account for this. Handle this right in livepatch_insn_len().
>>
>> This requires livepatch_insn_len() to be called _after_ ->patch_offset
>> was set.
>>
>> Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced 
>> functions")
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

As a note to the livepatch maintainers: I'm going to put this in
without further waiting for suitable acks. Just in case I'll put
it on the 4.16 branch only for starters, to see that it actually
helps there (it's unusual to put something on the stable
branches before it having passed the push gate to master).

> Albeit I don't think I understand how the in-place patching is done. I
> would expect the !func->new_addr branch of the if in
> arch_livepatch_apply to fill the insn buffer with the in-place
> replacement instructions, but I only see the buffer getting filled
> with nops. I'm likely missing something (not that this patch changes
> any of this).

Well, as per the v2 thread: There's no in-place patching except
to NOP out certain insns.

> I'm also having trouble figuring out how we assert that the len value
> (which is derived from new_size if !new_addr) is not greater than
> LIVEPATCH_OPAQUE_SIZE, which is the limit of the insn buffer. Maybe
> that's already checked elsewhere.

That's what my 3rd post-commit-message remark was (partly) about.

Jan