Re: [PATCH 2/3] x86/kprobes: Fix __recover_optprobed_insn check optimizing logic

[Yang Jihong <yangjihong1@huawei.com>]

Hello Masami,

On 2023/2/15 23:08, Masami Hiramatsu (Google) wrote:
> On Wed, 15 Feb 2023 19:54:29 +0800
> Yang Jihong <yangjihong1@huawei.com> wrote:
> 
>> Since the following commit:
>>
>>    commit f66c0447cca1 ("kprobes: Set unoptimized flag after unoptimizing code")
>>
>> modified the update timing of the KPROBE_FLAG_OPTIMIZED, a optimized_kprobe
>> may be in the optimizing or unoptimizing state when op.kp->flags
>> has KPROBE_FLAG_OPTIMIZED and op->list is not empty.
>>
>> The __recover_optprobed_insn check logic is incorrect, a kprobe in the
>> unoptimizing state may be incorrectly determined as unoptimizing.
>> As a result, incorrect instructions are copied.
> 
> Ah, good catch!
> 
>>
>> The optprobe_queued_unopt function needs to be exported for invoking in
>> arch directory.
>>
>> Fixes: f66c0447cca1 ("kprobes: Set unoptimized flag after unoptimizing code")
> 
> Cc: stable@vger.kernel.org

OK, will add in next version.

> 
>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>> ---
>>   arch/x86/kernel/kprobes/opt.c | 4 ++--
>>   include/linux/kprobes.h       | 1 +
>>   kernel/kprobes.c              | 2 +-
>>   3 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
>> index e57e07b0edb6..3718d6863555 100644
>> --- a/arch/x86/kernel/kprobes/opt.c
>> +++ b/arch/x86/kernel/kprobes/opt.c
>> @@ -46,8 +46,8 @@ unsigned long __recover_optprobed_insn(kprobe_opcode_t *buf, unsigned long addr)
>>   		/* This function only handles jump-optimized kprobe */
>>   		if (kp && kprobe_optimized(kp)) {
>>   			op = container_of(kp, struct optimized_kprobe, kp);
>> -			/* If op->list is not empty, op is under optimizing */
>> -			if (list_empty(&op->list))
>> +			/* If op is [un]optimized or under unoptimizing */
> 
> Hmm, this is a bit confusing comment. If it is unoptimized, the kprobe_optimized() returns false.
> Thus the comment should be /* If op is optimized or under unoptimizing */.
> 

OK, will fix in next version.

Thanks,
Yang.